Skip to content

Conversation

RodneyRichardson
Copy link
Contributor

As per #154.

Updated validate_uris, added ALLOWED_REDIRECT_URI_SCHEMES settings, and
HttpResponseUriRedirect class.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 469833f on RodneyRichardson:allow-custom-uri-schemes into 5ed4f50 on evonove:master.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not intimately familiar with this function, but is there a reason for using a custom regex instead of a custom validator based on urlparse.urlparse?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not my code - that's already in the master (I just fixed the scheme to match the RFC standard).

The call seems to handle the case where the network location has been internationalized (although there are no tests for that), so that may have something to do with it.

We DO need a RedirectURIValidator (which inherits from this), because the standard says that Redirect URIs cannot contain fragments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at urlparse documentation, I can't see why this (or the safe equivalent for Python 2 and 3 compatibility) couldn't be used instead of a RegexValidator. Perhaps you might add this as a separate issue?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, urlparse is what I used in my implementation (https://github.com/Locu/djoauth2/blob/9df7c3661e0a4c3585d3a333a63d5ed74472083c/djoauth2/authorization.py#L185):

if urlparse(redirect_uri).fragment:
  raise InvalidRequest('"redirect_uri" must not contain a fragment')

I'll take a look at this later today / tomorrow when I have the time.

@rudemateo
Copy link

It would be great to get this merged and pushed to a release on pypi! Thanks!

@zopieux
Copy link
Contributor

zopieux commented Jan 14, 2015

👍
I started working on my own PR before realizing there was already some efforts being made for this issue (and related ones). Thanks! If this could be merged I could take care of refactoring this regex nightmare into something based solely on urlparse, as long as there is no specific reason to use that(?).

@synasius synasius merged commit 469833f into django-oauth:master Jan 15, 2015
@RodneyRichardson RodneyRichardson deleted the allow-custom-uri-schemes branch September 13, 2016 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants