-
Notifications
You must be signed in to change notification settings - Fork 8
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
Don't try to set cookie from hash during OAuth login #190
base: master
Are you sure you want to change the base?
Conversation
I don't know if this is the right solution either, but I like it better than calling the hash thing every time. There is no way to solve the cross-origin OAuth problem except for hash string hacking. The only real choice we're offering here is the abillity to pick the prefix and suffix. It's messy, but even in cases where Vue-Router is being used, everything after the hash will be ignored regardless of router mode. On the flip side, this is a scenario where someone without a ton of Girder or even web programming experience can pull it off the shelf and have it do the right thing. You and I will remember how to do do this now that we've talked about it, but the next intern or new hire will have to go rooting around in Girder's auth code or ping the slack channel after realizing that they can't get access to girderToken after OAuth same as I did. Maybe others will catch on quicker than me :P I'd settle for a bit of documentation about how to make this work and leaving I'd be happy to modify this PR with those changes. Also, as-is, this will break the gwc.girder.org demo app. |
To be precise, there's no way to do it without putting the token somewhere in the URL. Different apps may wish to put it in different parts of the URL.
I agree that this is mostly a documentation issue. We need some kind of cookbook, and this ought to be a recipe inside it titled "Enabling OAuth login in a CORS environment". Where I deviate here is that I am disinclined to provide any help in the form of code in this library to support that, as I believe it's out of scope; dealing with URLs and routing should IMO be completely in the domain of the application rather than this library. The documentation message here should be that using the OAuth features of this component will require downstreams to bring their own URL, but we'll provide a sensible default ( Curious to hear others' thoughts on this since it is possibly controversial. @matthewma7 @danlamanna @brianhelba |
Just wanted to bump this thread. I'm still in favor of this library not making any assumptions about or modifying the browser's URL. |
I endorse removing it from girderRest, but I still think we ought to provide it as a utility. I don't know that we have enough recipes to warrant a cookbook, and I'd rather not maintain another package just for this. Even if we don't provide it from the library, this code has to go in this repo somewhere because the demo app needs it. |
Fixes #188
This is a breaking change as it removes public symbols intentionally.