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

Add API Key functional testing #3401

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

dbutenhof
Copy link
Member

PBENCH-1135

Add mechanism and a test case to verify that we can authenticate and identify the client user from an API key, simplistically using GET /datasets?mine.

This also adds a new alembic-migration command, show to display what alembic sees as the "heads" and "history" of the revision chain. I added this when I accidentally constructed multiple heads and got weird errors while I was building the AuthType.API_KEY upgrade, but didn't include it in that final PR. It seems useful, so I'm sweeping it in here.

PBENCH-1135

Add mechanism and a test case to verify that we can authenticate and identify
the client user from an API key, simplistically using `GET /datasets?mine`.

This also adds a new `alembic-migration` command, `show` to display what
alembic sees as the "heads" and "history" of the revision chain. I added this
when I accidentally constructed multiple heads and got weird errors while I
was building the `AuthType.API_KEY` upgrade, but didn't include it in that
final PR. It seems useful, so I'm sweeping it in here.
@dbutenhof dbutenhof added Server API Of and relating to application programming interfaces to services and functions Functional Testing labels Apr 26, 2023
@dbutenhof dbutenhof self-assigned this Apr 26, 2023
Copy link
Member

@webbnh webbnh 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, I just have one item for consideration.

Comment on lines +105 to +107
token = self.api_key if self.api_key else self.auth_token
if token:
headers["authorization"] = f"Bearer {token}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if the choice for the bearer token should be explicit, instead of a quiet fallback.

I'm concerned that (knowing the way we test things...), this implicit approach could lead to a test using an API key when it thinks it's using an access token (e.g., because some previously-run test scenario happened to add an API Key to a shared client)...which would eventually be an unpleasant surprise.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered how to work that in; it would be a bit more messy as I already have **kwargs to pass additional requests parameters so I'd need to filter out higher level kwargs before passing them down. (The alternative, of adding an explicit parameter to each higher level API, was also unpleasant.) Neither is impossible, but I decided not to complicate things. We can always consider that later.

Copy link
Member

Choose a reason for hiding this comment

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

We can always consider that later.

Of course (which is why I approved...).

it would be a bit more messy as I already have **kwargs to pass additional requests parameters so I'd need to filter out higher level kwargs before passing them down. (The alternative, of adding an explicit parameter to each higher level API, was also unpleasant.)

I don't disagree. However, the another alternative would be to have an attribute on the PbenchServerClient which indicates whether and how to set the "authorization" header. Prior to this PR, it was "smoke 'em if you got 'em"; now it's "set it if we have an API key or an access token". If the control were explicit, then a test could try all three options with the same PbenchServerClient instance without having to "log out" and/or delete the API key. (There are some details in terms of how to implement/express this in the code, but the principle of letting the caller control
the header presence/value seems sound.)

Copy link
Member Author

Choose a reason for hiding this comment

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

another alternative would be to have an attribute on the PbenchServerClient

I'm trying to decide whether I shouldn't like that idea. It's an easy implementation path, and you're right that it provides flexibility to switch back and forth. It also puts a bit more cleanup burden on someone, though we may be able to bury that in the fixtures.

However it doesn't address the fact that the authentication style is an implicit "mode" outside the normal call chain, which seems to me what you were really complaining about. Making it an explicit attribute of each call with a well known default is a lot more painful to implement, but would avoid the implicit mode aspect entirely.

At least it's another option to consider.

Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to decide whether I shouldn't like that idea.

😆 Well, you don't have to like it -- you just have to decide whether it is the least of weevils.

It also puts a bit more cleanup burden on someone

I'm not sure that it does. I think it separates policy from mechanism, so that the test can select the authentication that it wants for the next request that it makes. It's the equivalent of a global switch, which saves us from having to specify it (in whichever way) on each API call. Yes, it is "sticky", although no more so than the result of this PR, but it's part of an explicit methodology (unlike the result of this PR).

The cleanup burden is going through the existing use cases and making them specify their respective desired modes. (Perhaps this is what you meant.) But, my instinct is that this will make the tests more robust, and it might offer possibilities for better "parametrization". (Or, as you say, it'll just be hidden in the fixtures.)

Making it an explicit attribute of each call with a well known default is a lot more painful to implement, but would avoid the implicit mode aspect entirely.

True, but, to the extent that we want to use a given API key or access token value repeatedly, it means that the caller would have to manage that value and pass it in through the PbenchServerClient API, instead of being able to create/cache it in the instance (otherwise, we end up with an awkward API which can request a mode for which the client has no token value cached, which strikes me as a design failure; and, it would mean that the "well-known default" would have to be "no token", because we won't, in general, have one to use until the caller creates it).

Also, I expect that the flow will be that the caller uses the PbenchServerClient instance to obtain an access token, and then it uses that access token to obtain an API key...and, in that sort of scenario, the test/client instance is going to have both values, and the test needs to choose which one it wants the client to use when.

Copy link
Member

@siddardh-ra siddardh-ra 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

@dbutenhof dbutenhof merged commit 7dc871e into distributed-system-analysis:main Apr 26, 2023
@dbutenhof dbutenhof deleted the apikey branch April 26, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Of and relating to application programming interfaces to services and functions Functional Testing Server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants