-
Notifications
You must be signed in to change notification settings - Fork 117
Conversation
router.replace( | ||
`/object/${response.oAuthRequest.appId}?requestId=${requestId}&appOption=${response.oAuthRequest.appOption}` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit WIP until we have an app to authenticate against, but it will be something like this
</Col> | ||
</Row> | ||
|
||
<Modal show={showModal} onHide={() => setShowModal(false)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case you have more than one identity, we show a modal so you can pick the right one
// TODO: How can we test this without making real oAuth requests? | ||
test.todo("an oAuth token and identities can be retrieved"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like in the telemetry app, we need to come up with a pattern to test a full oAuth flow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality is looking great so far. I left a few comments
7e45a2f
to
733c31a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think we should make sure that the requestIds can't be used multiple times to sign in, maybe this means doing something to the OAuthRequest on successful sign in
core/src/actions/session.ts
Outdated
const oauthRequest = await OAuthRequest.findOne({ | ||
where: { id: params.requestId }, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this matches we should also delete the OAuthRequest
or set something on it so it can't be used again. At the moment the requestId
can be reused (until it's swept)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Love it. Thank you for reading all this code so carefully! Updated in e2ad6c6
320d204
to
e21ede5
Compare
Ok @pedroslopez I've got login reuse fixed up, and telemetry deployed. I think this is ready to go! |
Milestone! The production |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woo!
throw new Errors.AuthenticationError("password does not match"); | ||
} else { | ||
const oauthRequest = await OAuthRequest.findOne({ | ||
where: { id: params.requestId, consumed: false }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a little odd that the identities are still returned to the user for a consumed oauth request (going to /session/sign-in?requestId=req_ABCDE), but it's probably ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea! 857d687 prevents showing identities for consumed oAuth requests
b86fe74
to
857d687
Compare
Users can sign into Grouparoo instances with oAuth
oAuth.mov
Checklists
Development
Impact
Please explain any security, performance, migration, or other impacts if relevant:
Code review