Fix server error if not authenticated

[lekoala]

For context : silverstripe/silverstripe-framework#9978

Here, my proposal is to return a consistent json response

[GuySartorelli]

I'd hesitate to just catch all Exceptions - if something does actually fail, this will now mean there's no log of the failure other than a context-less 500 response in access logs.

Have you identified what is specifically causing the Exception to be thrown? It may be that a more targeted approach is appropriate.

[lekoala]

I can improve the pr to catch the specific message if needed

[GuySartorelli]

The exception being caught is from SilverStripe\GraphQL\Controller::getRequestUser():

silverstripe-graphql/src/Controller.php

Lines 439 to 447 in b51ce72

	if (!$member) {
	throw new Exception("Authentication required");
	}
	// Check authorisation for this member
	$allowed = Permission::checkMember($member, $permissions);
	if (!$allowed) {
	throw new Exception("Not authorised");
	}

The exceptions were originally added to index() in #57.

We should probably also treat the "Not authorised" exception message in the same way.

In general I'm okay with this. If you're happy to make a change to check for the "Not authorised" message, and change the response code (401 for "Authentication required" and 403 for "Not authorised"), I'll be happy to approve it. I'll probably ask someone more familiar with GraphQL to review it before I'd be comfortable merging.

Note that while @unclecheese did say it shouldn't require authentication at all in the issue, I don't have enough background on why the authentication checks were added in the first place, so I'm more comfortable with this approach.

[lekoala]

We should probably also treat the "Not authorised" exception message in the same way.

I don't think that's strictly necessary. I don't mind having logged in users get that kind of error page. My issue is that public endpoints can be visited by anyone (bots for example) and trigger errors that get logged for instance.

I think the current design works ok as long as the public use case is covered.

I've changed the error code as you requested.

[GuySartorelli]

Sorry it's taken me so long to get back to this.
Please treat the "Not authorised" exception message the same way for consistency.
Please also target this to 3.8 so it can be released as a patch.

Once that's done I'll be happy to merge.

Alternatively if you don't plan on continuing with this PR please close it.

[lekoala]

@GuySartorelli done

[GuySartorelli]

Thanks for that. Unfortunately as mentioned in #447 (review) the two scenarios news to return differen HTTP code (specifically 401 for "Authentication required" and 403 for "Not authorised")

Can you please make that change?

[lekoala]

sorry i missed that one
done