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

Unify handling of bearer token security scheme. #515

Closed
wants to merge 1 commit into from

Conversation

ngnpope
Copy link
Contributor

@ngnpope ngnpope commented Sep 14, 2021

Fixes JWTScheme to behave similar to the implementation of TokenScheme and SimpleJWTScheme. Also adds additional tests.

This follows on from #508 (comment).

@codecov
Copy link

codecov bot commented Sep 14, 2021

Codecov Report

Merging #515 (3e3479d) into master (93935c7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #515   +/-   ##
=======================================
  Coverage   98.62%   98.62%           
=======================================
  Files          58       59    +1     
  Lines        5951     5980   +29     
=======================================
+ Hits         5869     5898   +29     
  Misses         82       82           
Impacted Files Coverage Δ
drf_spectacular/authentication.py 100.00% <100.00%> (ø)
drf_spectacular/contrib/rest_framework_jwt.py 100.00% <100.00%> (ø)
...rf_spectacular/contrib/rest_framework_simplejwt.py 91.66% <100.00%> (ø)
drf_spectacular/mixins.py 100.00% <100.00%> (ø)
tests/contrib/test_drf_jwt.py 100.00% <100.00%> (ø)
tests/contrib/test_simplejwt.py 100.00% <100.00%> (ø)

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 93935c7...3e3479d. Read the comment docs.

@tfranzel
Copy link
Owner

Hey @ngnpope, thanks for the PR, but i am leaning against it for the following reasons:

  • the Extension base classes are kept minimal by design. this use-case is not compelling enough for me to break that consistency.
  • TokenAuthentication does not need the extra functionality.
  • the change adds very little overall and on top of that the diff is messy (especially the tests)

Fixes `JWTScheme` to behave similar to the implementation of
`TokenScheme` and `SimpleJWTScheme`. Also adds additional tests.
@ngnpope
Copy link
Contributor Author

ngnpope commented Sep 16, 2021

  • the Extension base classes are kept minimal by design. this use-case is not compelling enough for me to break that consistency.

I understand. I was looking at consistency from a different angle w.r.t. the output generated. JWTScheme needs the same fix as SimpleJWTScheme and I was trying to avoid copying and pasting.

I pushed an updated version that uses a separate mixin for composition instead of inheritance, de-cluttering the extension base class. I also updated the tests to reduce the churn significantly. I don't know whether it's more acceptable...

@tfranzel
Copy link
Owner

tfranzel commented Sep 17, 2021

apologies @ngnpope, i was too quick to discard this. I missed that this is actually used 3 times.

your change is fine, but the placement, namings, and partially the tests did not sit 100% right with me. i took the liberty to rewrite this a bit differently. hope this is fine with you. thank you for being persistent here.

@ngnpope
Copy link
Contributor Author

ngnpope commented Sep 17, 2021

apologies @ngnpope, i was too quick to discard this. I missed that this is actually used 3 times.

Thanks 🙂 Yes, I was hoping that this approach would help avoid it diverging again in the future.

your change is fine, but the placement, namings, and partially the tests did not sit 100% right with me. i took the liberty to rewrite this a bit differently. hope this is fine with you. thank you for being persistent here.

No worries. You know how your project is structured best so 👍🏻.

@ngnpope ngnpope closed this Sep 17, 2021
@ngnpope ngnpope deleted the fix-jwt-auth branch September 17, 2021 17:26
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

2 participants