Allow loopback redirect URIs using ports as described in RFC8252

[pauldekkers]

Description of the Change

Allow loopback redirect URIs with ports using http scheme and localhost addresses (127.0.0.1 or ::1),
as described in RFC8252 (section 7.3).

Checklist

• PR only contains one change (considered splitting up PR)
• unit-test added
• documentation updated
• CHANGELOG.md updated (only for user relevant changes)
• author name in AUTHORS

[codecov]

Codecov Report

Merging #953 (947a83d) into master (b4f418b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #953   +/-   ##
=======================================
  Coverage   96.61%   96.62%           
=======================================
  Files          31       31           
  Lines        1713     1716    +3     
=======================================
+ Hits         1655     1658    +3     
  Misses         58       58           

Impacted Files	Coverage Δ
oauth2_provider/models.py	98.67% <100.00%> (+0.01%)	⬆️

---

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 b4f418b...947a83d. Read the comment docs.

[auvipy]

thanks for your PR. can you check the PR checklist and update as appripriate? changelog, entry to the author, and possibly unit test?

[pauldekkers]

I've modified the patch slightly to only accept random redirect_uri ports when there is no explicit port configured in the Application. I've added a line in the documentation, updated the CHANGELOG and AUTHORS.
I'm struggling a bit with the unit test as there is no existing test and these tests are a bit more complex than the suggested change. Would you accept without?

[rtpg]

While reading over this back and forth I took the opportunity to just add in some tests and factor out this logic to make it easier to unit test. Thanks for doing all the legwork on this first though!

I think that this loopback special casing in redirect code (pretty sensitive area, securitywise) warranted a nice chunky comment, but my tests left me pretty happy that this is the "right" way of going forward.

@auvipy since I made a change I feel a bit uncomfortable approving the PR, please look over what I pushed in.

[rtpg]

at first it looks like both of these branches could be merged! And for scheme and path it can, but netloc is actually inclusive of the port (it's roughly login info + hostname + port) , and for loopback IPs we don't want to be requiring the port to be the same (the whole point is wanting to support ephemeral IPs)

[pauldekkers]

@rtpg Thanks for your modifications and adding those tests, I can now tick the unit-test checkbox too ;-)

FWIW, I've tested your changes locally and of course it works just as well.

[pauldekkers]

Hi; it looks like this PR is stuck on "changes requested" while I think they were actually applied @auvipy ? I kind of think @rtpg made this PR even better 👍 but I'm not sure where we stand.

Review comments were handled

[rtpg]

alright a bit convoluted but given that @pauldekkers and I have been going back and forth on this I think we can consider this to be reviewed sufficiently

[n2ygk]

@rtpg @pauldekkers - Hey welcome to the project. Thanks for your PR. @MattBlack85 and I are trying to be good project leads and stay on top of changes and releases and will try to slot this in to a future release after we review it.

Notably @rtpg I do not see your name in AUTHORS. Please submit a PR to correct that.

[kargaj]

Hi, when you are planning the next release and adding this feature?