-
Notifications
You must be signed in to change notification settings - Fork 53
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
Test creation of API keys for other principals #609
Conversation
To my taste, a long-ish test that "tells a story" is more readable than lots of small tests that tell part of a story. This one is borderline too long, I agree, but I don't see a clean place to break it out or any obvious resusable abstraction that would be a natural fixture. I do wonder if we should take this moment to promote I had in the back of my mind the idea of making an |
This sounds like a good idea to me. |
Thanks for affirming this. I have two thoughts on how to simplify this, without too much effort. I'm out of time for now though. I'll move this PR to draft and attempt to resume it later this week. |
Now redundant after splitting into smaller tests
I've split the new long test into several smaller tests, that I think are easier to digest. This PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good.
I left some suggestions for future-proofing and streamlining a bit.
Thanks for the reviews! I'll make these changes. |
* Applied suggested changes from PR review * Use `context.api_key` to send API key via request headers, rather than via URL query parameters * Pass query parameters as dict using httpx request argument
@danielballan All recommendations have been incorporated. This is ready for review. |
This PR adds a test to verify that an admin can create API keys using the route
/principal/{uuid}/apikey
and that non-admins cannot. This test also verifies that a user cannot bypass the scopes checks by passing an empty scopes list when creating an API key and/or when requesting an admin resource.The test is fairly long (in lines of code, not in execution speed). I could probably break it into several individual tests that are backed by some common fixtures. Do you have advice on keeping this more concise or clear?