Refactor OAuth provider code and switch from Flask-OAuthlib to Authlib #4798
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ThiefMaster
force-pushed
the
oauth
branch
4 times, most recently
from
3274acd to
de35086
Compare
It was never used, and the idea of using the session user if there is any error with the oauth token (missing, invalid, whatever) is extremely obscure and prone to errors.
The implicit flow has been replaced with the more modern and in case of mobile apps more secure code+pkce flow.
It's less secure and we do not really care about clients that cannot create a SHA256 hash...
The PKCE flow can be used in browsers, so they should be able to exchange the code for a token via CORS
If a test results in a templte being rendered this usually looks up some webpack bundles which we don't care about during tests and don't have during CI.
The old name was just poor naming on the flask-oauthlib side, and never had anything to do with default scopes.
Also refactor how user app authorizations are stored; instead of checking for a token, we now have a separate model holind the authorization and granted scopes, so if an app revokes its own token, the scopes will remain authorized in any future authorization flow the user does.
Deleting is fine, our tokens are unique so there is basically no risk of regenerating the same token that has been revoked before.
Not sure why it now works without that ugly hack...
upstream removed it as well
Apps that are not targeting SPA/native apps do not need to allow flows without a client secret.
While not passwords, they are sensitive and there is no need to store plaintext versions of them.
This also adds support for passing the token insecurely in the query string, but also ONLY for that app. Like this we are not under any pressure to update the app, and we can simply do it at some point after a proper Indico v3 release.
javfg
approved these changes
Feb 24, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
/.well-known/oauth-authorization-server)maybe group by scope tokeep the total number of tokens still limited? grouping is no longer possible due to hashing, keeping up to 50 tokens for now)Regarding the breaking change, this needs an update in the checkin app which I think is the only client using the implicit flow right now. An alternative, especially since hello.js which we use in that app does not support PKCE yet (see MrSwitch/hello.js#633), could be enabling the implicit flow but allowing it only for the checkin app so we can remove it once everyone is on 3.0 and there is no need to support older Indico versions in the checkin app. (Update: The checkin system app is allowed to use the implicit grant and token-in-querystring for now)
closes #4685