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.

Sign up for GitHub

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
Merged

Add API Key functional testing #3401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions lib/pbench/client/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def __init__(self, host: str):
url = f"{self.scheme}://{self.host}"
self.url = url
self.username: Optional[str] = None
self.api_key: Optional[str] = None
self.auth_token: Optional[str] = None
self.session: Optional[requests.Session] = None
self.endpoints: Optional[JSONOBJECT] = None
Expand All @@ -101,8 +102,9 @@ def _headers(
Case-insensitive header dictionary
"""
headers = CaseInsensitiveDict()
if self.auth_token:
headers["authorization"] = f"Bearer {self.auth_token}"
token = self.api_key if self.api_key else self.auth_token
if token:
headers["authorization"] = f"Bearer {token}"
Comment on lines +105 to +107
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.

if user_headers:
headers.update(user_headers)
return headers
Expand Down Expand Up @@ -336,6 +338,25 @@ def login(self, user: str, password: str):
)
self.username = user
self.auth_token = response["access_token"]
self.api_key = None

def create_api_key(self):
"""Create an API key from the current authenticated user

Creating an API key will cause the new key to be used instead of a
normal login auth_token until the API key is removed.
"""
response = self.post(api=API.KEY)
self.api_key = response.json()["api_key"]
assert self.api_key, f"API key creation failed, {response.json()}"

def remove_api_key(self):
"""Remove the session's API key

NOTE: when we support `DELETE /api/v1/key/{key}` this should delete the
key from the server.
"""
self.api_key = None

def upload(self, tarball: Path, **kwargs) -> requests.Response:
"""Upload a tarball to the server.
Expand Down Expand Up @@ -427,7 +448,9 @@ def get_list(self, **kwargs) -> Iterator[Dataset]:
args = kwargs.copy()
if "limit" not in args:
args["limit"] = self.DEFAULT_PAGE_SIZE
json = self.get(api=API.DATASETS_LIST, params=args).json()
response = self.get(api=API.DATASETS_LIST, params=args, raise_error=False)
json = response.json()
assert response.ok, f"GET failed with {json['message']}"
while True:
for d in json["results"]:
yield Dataset(d)
Expand Down
12 changes: 12 additions & 0 deletions lib/pbench/client/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ def __getitem__(self, key: str) -> Any:
"""
return self.json[key]

def __repr__(self) -> str:
"""Represent by returning the JSON representation"""
return repr(self.json)

def __str__(self) -> str:
"""Stringify by returning the stringified JSON"""
return str(self.json)


class Dataset(JSONMap):
@staticmethod
Expand All @@ -67,3 +75,7 @@ def md5(tarball: Path) -> str:
"""
md5_file = Path(f"{str(tarball)}.md5")
return md5_file.read_text().split()[0]

def __str__(self) -> str:
"""Identify the dataset by ID and name"""
return f"Dataset({self.resource_id}, {self.name!r})"
3 changes: 3 additions & 0 deletions lib/pbench/server/database/alembic.migration
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ elif [[ "${1}" == "create" ]]; then
# We have been asked to auto-generate a migration based on the existing
# model compared against the most recent migration "head".
alembic revision --autogenerate
elif [[ "${1}" == "show" ]]; then
alembic heads
alembic history
else
printf "Unsupported operation requested, '%s'\n" "${1}" >&2
exit 1
Expand Down
22 changes: 22 additions & 0 deletions lib/pbench/test/functional/server/test_put.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,28 @@ def test_list_anonymous(self, server_client: PbenchServerClient):

assert count > 1

@pytest.mark.dependency(name="list_api_key", depends=["upload"], scope="session")
def test_list_api_key(self, server_client: PbenchServerClient, login_user):
"""List "my" datasets using an API key.

We should see all datasets owned by the tester account, both private
and public. That is, using the API key is the same as using the normal
auth token.
"""
server_client.create_api_key()
assert server_client.api_key, "No API key was set on the session"
datasets = server_client.get_list(mine="true")

expected = [
{"resource_id": Dataset.md5(f), "name": Dataset.stem(f), "metadata": {}}
for f in TARBALL_DIR.glob("*.tar.xz")
]
expected.sort(key=lambda e: e["resource_id"])
actual = [d.json for d in datasets]
assert expected == actual
server_client.remove_api_key()
assert not server_client.api_key, "API key was not removed as expected"

@pytest.mark.dependency(name="list_or", depends=["upload"], scope="session")
def test_list_filter_or(self, server_client: PbenchServerClient, login_user):
"""Check a simple OR filter list.
Expand Down