-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The dialogue routes need to be renamed #19
Comments
I've fixed the oauth flow so that users who have already trusted the client application will not need to again. It's ready to be tested on stage. To further clarify:
|
I haven't had a chance to look into this, I was going to review how github worked by dumping all the information i get back in a small example https://github.com/jaredhanson/passport-github project. Key things to observe are:
I would rather follow the lead of an existing implementation in this case. |
Is this still a pertinent issue / enhancement? |
I have done a review of the github oauth2 provider and it will reissue the same token for a given application between sessions. At the moment we are generating a new token per login to the application, this is less than ideal with some users already at 15 access tokens just doing some testing. So workflow should be:
This will avoid us ending up with an endless churn of access tokens. |
I've fixed it so that access-tokens already issued for that user / application combination are returned. Mark, I've deployed the fix to stage, could you test and close this ticket if you're happy with the fix? |
Yeah I will do some testing tomorrow as we are doing some more integration with our existing services. Thanks |
In the oauth2 routes section you have two routes see https://github.com/ninjablocks/douitsu/blob/master/lib/oauth2-routes.js#L63-L64
Github has the following URL https://github.com/login/oauth/authorize
The thing here is that our current route name implies that only dialogues will be shown by this route, which is not the case if the user has already trusted the application see #11.
The text was updated successfully, but these errors were encountered: