Address remaining "authentication" tasks #20
Conversation
Also, return a default identity from the test server's LocalIdentityProvider unless there is an explicit entry for the given token in the map. This makes it easier to set up tests for logic that is unrelated to authentication and identities.
There was a problem hiding this comment.
This is coming along nicely, @as-cii. I've noted a few questions in the review below. I hope this helps.
test/identity-provider.test.js
Outdated
| test('throws an error when GitHub API is inaccessible', async () => { | ||
| const request = { | ||
| get: async function (url, {headers}) { | ||
| const body = JSON.stringify({message: 'Bad credentials'}) |
There was a problem hiding this comment.
Do we need this line, or can we remove it?
test/identity-provider.test.js
Outdated
| const request = { | ||
| get: async function (url, {headers}) { | ||
| const body = JSON.stringify({message: 'Bad credentials'}) | ||
| throw new RequestError('a request error') |
There was a problem hiding this comment.
If we can't reach the GitHub API, will request-promise-core throw a RequestError?
lib/controller-layer.js
Outdated
| const identity = await identityProvider.identityForToken(oauthToken) | ||
| res.send(identity) | ||
| } catch (error) { | ||
| res.status(401).send({message: 'No identity found for OAuth token'}) |
There was a problem hiding this comment.
It looks like we're translating any GitHub API failure response into a 401. I wonder if we want to capture more details about the underlying failure that we got from the GitHub API. For example, if the user has exceeded their GitHub API rate limit, we'll get a 403 response from the GitHub API. In that case, the token is valid, but the user just needs to chill out for a bit. It might confuse the user if we say that there was "No identity found for the OAuth token." What do you think?
There was a problem hiding this comment.
Yeah, we definitely need to have more specific errors and audit downstream code paths for handling those errors in a specific way. I'd like to revisit our approach to handling server errors in general, possibly throwing appropriate exceptions from the rest gateway.
Signed-off-by: Antonio Scandurra <as-cii@github.com>
This is a follow-up to #4 and takes care of the remaining tasks to provide an authentication facility for real-time-client and, as a result, for the real-time package as well.
@jasonrudolph: I think this addresses all the TODOs you originally added, but I'd ❤️ to have your feedback on this.
/cc: @nathansobo