-
Couldn't load subscription status.
- Fork 23
Allow wildcard for redirect_urls in OAuth2 apps #702
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
Allow wildcard for redirect_urls in OAuth2 apps #702
Conversation
bdb380e to
24611cb
Compare
2aaafb6 to
ca88f5e
Compare
ca88f5e to
9be812b
Compare
66521d6 to
0eb0c40
Compare
0eb0c40 to
69b52c4
Compare
1f823eb to
8ca575a
Compare
8ca575a to
ee4afee
Compare
ee4afee to
5a413ed
Compare
| """ | ||
|
|
||
|
|
||
| default_app_config = ( |
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.
Do we need this? It looks like default_app_config was deprecated and then removed from django.
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.
You're right. I just copied from the URL in the comment and that's the only reason that I included it here.
I found there was a PR on removing default_app_config from django-oauth-toolkit as well. I will remove that line.
| def _uri_is_allowed(allowed_uri, uri): | ||
| """Check that the URI conforms to these rules.""" | ||
| schemes_match = allowed_uri.scheme == uri.scheme | ||
| netloc_matches_pattern = re.fullmatch(allowed_uri.netloc, uri.netloc) |
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.
While I know we discussed that we would be very careful with our regexs that we specify in our applications, should we also consider enforcing e.g. that the top level domain is an exact match, and only the subdomain can have a wildcard? I came across this wildcard_redirect support in okta that takes this approach and seems to help limit the risk: https://developer.okta.com/docs/reference/api/apps/#details
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.
The current implementation accepts general expressions, but if we put limitations like Okta's implementation, we should just support "wildcard" ('*') even though we will use regex in internal processing. I will re-design this part in that way.
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.
I have implemented a new design in the latest commit:
- Instead of accepting a given string for allowed URI as a regex pattern, allow only wildcards (asterisk
*) in a redirect URI - A wildcard matches any (0 or more) characters except for slashes (
/). - In a netloc part, wildcards should appear only in the subdomain, not in the root domain.
- Matching with wildcards is not performed against IP addresses. For example, even if an allowed URI is set to
http://*.*.0.1:8080/callback,http://123.123.0.1:8080/callbackdoes not match to it.
| } | ||
|
|
||
| # | ||
| # We need to run 'manage.py migrate' before adding our own OAuth2 application model. |
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 we be seeing a migration file in this PR for this new application? Assuming yes, I'm not entirely clear on the order of operations for getting everything set up correctly.
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.
No, we will not have a new migration file with this PR. The new "wildcard" OAuth app will swap the default app as it is defined as "swappable" and the same db table will be used as the default one,
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 awesome work and I'm excited to get it in! Sent you a few comments and questions. Thanks!
|
@robinbobbitt I have addressed to your comments in the latest commit. Would you take a look? Thank you! |
|
For anybody else testing this PR, Jeff's test script here is handy: #84 (comment) |
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.
Thanks @TamiTakamiya ! This looks great. I tested with valid and invalid redirect uris in the Application configuration and in the authorize flow and all looks good.
For AAP-18031