Bug 1518137 - better libraries/api error handling, fix InsufficientScopes #54
Conversation
…opes The important thing here is not losing track of the message template, so users still see the explanation of their InsufficientScopes error. But this also factors out the translation of AuthorizationError -> InsufficientScopes and AuthenticationError -> AuthenticationFailed.
| throw new ErrorReply({ | ||
| code: 'AuthenticationFailed', | ||
| message: result.message, | ||
| details: result, |
There was a problem hiding this comment.
So that result object - does it have the information about which scopes are missing anywhere in it? Should we be showing that information at all in the authorization case?
There was a problem hiding this comment.
This error is for bad Hawk credentials, not bad scopes - so no, not in this case.
| @@ -319,11 +317,6 @@ const remoteAuthentication = ({signatureValidator, entry}) => { | |||
| next(); | |||
| } | |||
| } catch (err) { | |||
| if (err.code === 'AuthorizationError') { | |||
| return next(new ErrorReply({code: 'InsufficientScopes', message: err.messageTemplate, details: err.details})); | |||
There was a problem hiding this comment.
I'm quite puzzled. The error that Simon reported lacked details field completely (or at least it wasn't showing up in the logs). Why would that be?
There was a problem hiding this comment.
The original bug was, indeed, right here -- we passed err.messageTemplate (which is undefined) instead of err.message (which contains the message simon expected). The minimal fix to that bug was just messageTemplate -> message, but I took the chance to clean up some messiness in handling errors while I was at it.
There was a problem hiding this comment.
Hm, hold on. The code says: if (err.code === 'AuthorizationError') - the code of the offending error was InsufficientScopes (although the message was, indeed, Authorization failed). And the template was absent from the err.message, and details field was missing from the error object altogether
There was a problem hiding this comment.
Ok, I am quite confused by this piece of code (as I was confused by the error object) - it's good it's gone in your PR 😆
There was a problem hiding this comment.
Yeah, it was a bit of leftovers from the refactor that @arshadkazmi42 worked on.
| @@ -253,12 +253,11 @@ const remoteAuthentication = ({signatureValidator, entry}) => { | |||
| // If authentication failed | |||
| if (result.status === 'auth-failed') { | |||
| res.set('www-authenticate', 'hawk'); | |||
| const err = new Error('Authentication failed'); // This way instead of subclassing due to babel/babel#3083 | |||
|
deploying now |
…ter-taskcluster-clients-client-go-v21-21.x Update module taskcluster/taskcluster/clients/client-go/v21 to v21.2.0
Prevent zombie nodes being created from filesystem creation error
Prevent zombie nodes being created from filesystem creation error
The important thing here is not losing track of the message template, so
users still see the explanation of their InsufficientScopes error. But
this also factors out the translation of AuthorizationError ->
InsufficientScopes and AuthenticationError -> AuthenticationFailed.