-
Notifications
You must be signed in to change notification settings - Fork 6
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
Another go at an OAuth module #24
Conversation
c3441d5
to
699e1d7
Compare
699e1d7
to
6b444a4
Compare
This is ready to go now. The API is as follows:
Fix #13 |
…ssion check before getting current user
e563e2b
to
d97cbc1
Compare
|
||
console.log('Using OAuth (implicit grant) for login'); | ||
console.info('Setting app ID to', appID); | ||
ls.get(LOCAL_STORAGE_PREFIX + 'clientAppId', appID); |
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.
Should this be ls.set
?
@brian-c are your comments blocking the merge? If so, I'm not sure when @rogerhutchings will be able to address them. I'd be happy to have a look too. |
I still haven't been able to get this to work. Will run through it again today. |
Cool thanks, let me know how it goes please. |
Workin' fine now. Will look into figuring out a nice way to test automate testing this. |
Published in v2.1.0. |
👍 |
I rewrote my previous implementation largely from scratch, and I think it's a lot better for it.
This version will eventually do all token handling itself, and request new tokens via an iFrame, so no page redirects are needed outside of the initial auth. That's currently blocked by zooniverse/panoptes#1673
It'll also have a much simpler API -
init
to enable URL watching etc, thencheckCurrent
,signIn
andsignOut
.Currently a work in progress, please do not merge.