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

Easily Retrieve the Raw API Key from a View #102

Closed
wants to merge 2 commits into from

Conversation

dstarner
Copy link

@dstarner dstarner commented Jan 30, 2020

Figured that this would be an easy one to get my feet wet.

Solves #98 as suggested in the issue.

This turns get_key on the BaseHasAPIKey permission class into a @classmethod which will allow a view or viewset to access the raw API key using HasAPIKey.get_key(request).

This will not break backwards compatibility if someone has done HasAPIKey().get_key(request) because Python will automatically convert the instance and pass the appropriate cls type.

Compat Annotation

In order for scripts/check to run, I had to add the -> Generator to the nullcontext context manager. Not sure if this is a standalone issue or what, but it can't hurt others....I hope.

Testing

Note: the following text will be removed before merging the PR.

I am trying to understand the test fixtures, but I cannot figure out how to have the create_request fixture set up the request correctly for my view to parse the API key.

The following is what I'm attempting to do. The HasAPIKey.get_key(request) though is either returning None or the entire header. What's the correct argument to create_request for it to pass my custom key without mangling it?

def test_retrieving_raw_api_key_within_view(create_request, key_header_config):
    expected_key = 'thisismyapikey'
    expected_status = 200

    class View(generics.GenericAPIView):
        permission_classes = []

        def get(self, request):
            raw_key = HasAPIKey.get_key(request)
            status_code = expected_status if raw_key == expected_key else expected_status + 1
            return Response(status=status_code)

    view = View.as_view()
    request = create_request(authenticated=True, authorization='Api-Key %s' % expected_key)
    response = view(request)
    assert response.status_code == expected_status

@codecov-io
Copy link

codecov-io commented Jan 30, 2020

Codecov Report

Merging #102 into master will increase coverage by 0.44%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   96.37%   96.81%   +0.44%     
==========================================
  Files          21       21              
  Lines         469      471       +2     
==========================================
+ Hits          452      456       +4     
+ Misses         17       15       -2
Impacted Files Coverage Δ
src/rest_framework_api_key/permissions.py 94.73% <100%> (+0.14%) ⬆️
tests/compat.py 100% <100%> (+14.28%) ⬆️
src/rest_framework_api_key/models.py 98.75% <0%> (+1.25%) ⬆️

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 851aec1...5b9fb0b. Read the comment docs.

@florimondmanca
Copy link
Owner

florimondmanca commented Feb 2, 2020

Hey, thanks for tackling this one :)

Re: testing...

Useful to know that testing fixtures are hard to grasp... They sort of date back from two years ago in spirit, and I am not sure I understand them either. 🤣 Going to clean the mess up!

@florimondmanca
Copy link
Owner

Going to clean the mess up!

By that I meant, we probably should go for dumb to no fixtures, and be okay with code duplication in test functions. create_request() is trying too hard to de-duplicate code, but it’s not useful because we’re testing the public API here anyway.

@dstarner
Copy link
Author

I do not have time to work on this, someone feel free to take over if its not already merged

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.

3 participants