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

refactor(auth): Use self.authenticate_header() in authenticate() method to get auth header prefix #329

Merged
merged 1 commit into from
Jan 28, 2024

Conversation

luqmansen
Copy link
Contributor

@luqmansen luqmansen commented Jan 26, 2024

Description

As titled, seems like the TokenAuthentication class already defined a method to retrieve the header, yet not utilized within the class itself.

ref: the related method

def authenticate_header(self, request):
    return knox_settings.AUTH_HEADER_PREFIX

Similar technique has been done in rest_framework Token Authentication (though, using class attribute)

Impact

These changes allows the auth header to be overridden from the subclass without having to override the authenticate() method (just for small auth prefix header changes) or meddling with global settings for knox_settings.AUTH_HEADER_PREFIX (because it might be used somewhere else within the project)

example usage:

class TokenAuthWithCustomHeader(knox.TokenAuthentication):
    def authenticate_header(self, request):
        return "MyCustomAuthPrefix"

without these changes, the subclass needs to:

class TokenAuthWithCustomHeader(knox.TokenAuthentication):
    def authenticate(self, request):
        prefix =  "MyCustomAuthPrefix"
        # re-implement the exact same logic
        ...
                 

@luqmansen luqmansen changed the title refactor(auth): Use self.authenticate_header() in authenticate() method to get auth header prefix refactor(auth): Customizable auth header prefix in authenticate() Jan 26, 2024
@luqmansen luqmansen changed the title refactor(auth): Customizable auth header prefix in authenticate() refactor(auth): customizable auth header prefix in authenticate() Jan 26, 2024
@luqmansen luqmansen marked this pull request as draft January 26, 2024 08:16
@luqmansen luqmansen marked this pull request as ready for review January 26, 2024 09:40
@luqmansen luqmansen changed the title refactor(auth): customizable auth header prefix in authenticate() refactor(auth): refactor(auth): Use self.authenticate_header() in authenticate() method to get auth header prefix Jan 26, 2024
@luqmansen luqmansen changed the title refactor(auth): refactor(auth): Use self.authenticate_header() in authenticate() method to get auth header prefix refactor(auth): Use self.authenticate_header() in authenticate() method to get auth header prefix Jan 26, 2024
Copy link
Collaborator

@johnraz johnraz left a comment

Choose a reason for hiding this comment

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

if you can get rid of the 2 first commits I will approve !

Thanks for the contribution 👌🏻

Copy link

codecov bot commented Jan 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dd7b062) 91.70% compared to head (0a5b184) 91.70%.
Report is 2 commits behind head on develop.

❗ Current head 0a5b184 differs from pull request most recent head 4015993. Consider uploading reports for the commit 4015993 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #329   +/-   ##
========================================
  Coverage    91.70%   91.70%           
========================================
  Files            9        9           
  Lines          229      229           
  Branches        35       35           
========================================
  Hits           210      210           
  Misses          16       16           
  Partials         3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@giovannicimolin
Copy link
Contributor

@luqmansen Thank you for your contribution! 😁

@johnraz I think we can use GitHub's Squash and Merge feature to merge these changes in. 😉

@johnraz
Copy link
Collaborator

johnraz commented Jan 27, 2024

@giovannicimolin indeed that would give the same result, we just need to make sure the commit message is proper 😊

@luqmansen
Copy link
Contributor Author

Hi @johnraz, I have dropped the unneeded commit, thank you for reviewing :adore:

Copy link
Contributor

@giovannicimolin giovannicimolin left a comment

Choose a reason for hiding this comment

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

👍

Looks good! LGTM.

@giovannicimolin giovannicimolin merged commit 271179a into jazzband:develop Jan 28, 2024
17 checks passed
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