Skip to content
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

Normalize discovered URIs before comparing them #41

Closed
wants to merge 1 commit into from

Conversation

cjwatson
Copy link

@cjwatson cjwatson commented Jun 4, 2020

python-openid2's urinorm is stricter than that in the old Python-2-only
version: in particular, it always normalizes the reserved character '+'
to '%2B'. The discovery process normalizes URIs, so
GenericConsumer._verifyDiscoverySingle could end up comparing normalized
and unnormalized identifiers.

In particular, this broke with the Ubuntu single-sign-on system
(login.ubuntu.com / login.launchpad.net) which includes a '+' character
in the path part of its identifiers.

It makes sense to normalize both expected and discovered identifiers
before comparing them, so do that according to the rules in OpenID
Authentication 2.0 section 7.2.

@cjwatson
Copy link
Author

cjwatson commented Jun 4, 2020

#42 fixes the CI failures here. I plan to rebase this once that PR lands.

python-openid2's urinorm is stricter than that in the old Python-2-only
version: in particular, it always normalizes the reserved character '+'
to '%2B'.  The discovery process normalizes URIs, so
GenericConsumer._verifyDiscoverySingle could end up comparing normalized
and unnormalized identifiers.

In particular, this broke with the Ubuntu single-sign-on system
(login.ubuntu.com / login.launchpad.net) which includes a '+' character
in the path part of its identifiers.

It makes sense to normalize both expected and discovered identifiers
before comparing them, so do that according to the rules in OpenID
Authentication 2.0 section 7.2.
@cjwatson cjwatson force-pushed the normalize-discovered-uris branch from 726ed18 to 9b980fd Compare Jun 8, 2020
@cjwatson
Copy link
Author

cjwatson commented Jun 8, 2020

Rebased now, so CI should be happy shortly.

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2020

Codecov Report

Merging #41 into master will increase coverage by 0.00%.
The diff coverage is 92.75%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #41   +/-   ##
=======================================
  Coverage   89.48%   89.49%           
=======================================
  Files          93       93           
  Lines       12015    12058   +43     
  Branches     1084     1087    +3     
=======================================
+ Hits        10752    10791   +39     
- Misses       1094     1096    +2     
- Partials      169      171    +2     
Impacted Files Coverage Δ
openid/consumer/consumer.py 95.00% <82.14%> (-0.48%) ⬇️
openid/test/test_verifydisco.py 98.97% <100.00%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b0cbbf...9b980fd. Read the comment docs.

@cjwatson
Copy link
Author

cjwatson commented Jul 2, 2020

@ziima @tpazderka Would there be any chance of getting a review of this, please? I have I think three other projects so far that are blocked on this as part of their Python 3 upgrades.

@ziima
Copy link
Owner

ziima commented Jul 2, 2020

Sorry about the delay and thanks for poking us :-)

I believe the cause is in the urinorm itself, so I prepared a fix only for that in #43. I'd appreciate if you could check that it really helps with this issue.

@cjwatson
Copy link
Author

cjwatson commented Jul 2, 2020

@ziima #43 seems to fix my problem just as well, so I'm happy with that. Thanks!

@ziima
Copy link
Owner

ziima commented Jul 8, 2020

Closing, since #43 fixed the issue.

@ziima ziima closed this Jul 8, 2020
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.

None yet

3 participants