jazzband / django-rest-knox

Authentication Module for django rest auth
MIT License
1.15k stars 211 forks source link

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

Closed luqmansen closed 8 months ago

luqmansen commented 8 months ago

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
        ...
codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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 ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

giovannicimolin commented 8 months ago

@luqmansen Thank you for your contribution! :grin:

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

johnraz commented 8 months ago

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

luqmansen commented 8 months ago

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