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

[ENH] Allow fetching of arbitrary user environments #864

Merged

Conversation

peytondmurray
Copy link
Contributor

Fixes #857.

Description

This pull request:

  • Adds the ability to specify a JSON web token (JWT) as a query parameter when making a GET request to the /environment/ endpoint. The response will include only environments visible to the specified JWT.
  • Adds the ability to specify a role_mapping parameter for the corresponding Python API, conda_store_server.api.list_environments. The role mapping should be a dictionary of the same type used to configure role mappings in the conda_store_config.py, e.g. passing:
role_mapping={
  "*/*": ["editor"],
  "default/env": ["viewer"]
}

to list_environments would return only environments that match either of the two dictionary keys; the keys mean the same thing as they do in the conda_store_config.py:

"*/*" = match all namespace names and match all environment names
"default/env" = match any namespace named "default" and match any environment named "env"

  • To enable this, I moved conda_store_server.server.auth.AuthenticationBackend.compile_arn_sql_like, which turns a role mapping key into a SQL-compatible regex that can be used to filter namespaces and backends, to conda_store_server._internal.utils.compile_arn_sql_like so that I could use it outside of the AuthenticationBackend. Doing this required updating a few different files, and I took the opportunity to define some useful type aliases, leave some type annotations, and add some docstrings.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

How to test

Tests of new functionality in both the REST API and the Python API were added, so CI should take care of this. You can also run them locally with pytest.

Copy link

netlify bot commented Aug 16, 2024

Deploy Preview for conda-store canceled.

Name Link
🔨 Latest commit e08a0e9
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/66d7902238a9460008e32943

Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Thanks @peytondmurray I left a number of suggestions and questions.

Other questions as this was not entirely clear to me from the PR description and after seeing the tests.

  1. As an end user if I want to use the REST API to get a list of envs I need to pass then
role_mapping={
  "*/*": ["editor"],
  "default/env": ["viewer"]
}

as a query parameter like environments?role_mapping? or how would you expect this to work?

  1. if so, what happens if the user does not provide a role for example
role_mapping={
  "*/*",
}

is that even possible?

@peytondmurray
Copy link
Contributor Author

peytondmurray commented Aug 28, 2024

Thanks @peytondmurray I left a number of suggestions and questions.

Other questions as this was not entirely clear to me from the PR description and after seeing the tests.

1. As an end user if I want to use the REST API to get a list of envs I need to pass then
role_mapping={
  "*/*": ["editor"],
  "default/env": ["viewer"]
}

as a query parameter like environments?role_mapping? or how would you expect this to work?

No, that's only for the Python API. For the REST API, you pass a JWT of the user you want to fetch environments for, e.g. /environment/?jwt=<jwt-of-whatever-user>.

2. if so, what happens if the user does not provide a role for example
role_mapping={
  "*/*",
}

is that even possible?

Role mappings are dictionaries, so this isn't currently possible - you need associated roles in order for this to work.

@peytondmurray peytondmurray force-pushed the 857-add-user-env-listing branch from 407361d to a71899b Compare August 28, 2024 16:37
@trallard
Copy link
Collaborator

So for the REST API the default behavior is to just return all the environments a user has some access to.
No filtering through role or something else.

@peytondmurray
Copy link
Contributor Author

peytondmurray commented Aug 28, 2024

No, sorry, I should have made that more clear - for the REST API, any user A who requests the environments that user B has access to must also have access to those environments, following the discussion we had previously.

This is the line that grabs the environments for B: https://github.com/peytondmurray/conda-store/blob/a71899b506ffe83d767ce7c0b50c58bda256599e/conda-store-server/conda_store_server/_internal/server/views/api.py#L687

and this is the line that filters those environments by those that are visible to the requester A: https://github.com/peytondmurray/conda-store/blob/a71899b506ffe83d767ce7c0b50c58bda256599e/conda-store-server/conda_store_server/_internal/server/views/api.py#L700

@trallard
Copy link
Collaborator

Right I did see the filtering when doing the code review this

No, that's only for the Python API. For the REST API, you pass a JWT of the user you want to fetch environments for, e.g. /environment/?jwt=.

was what confused me. So all good then.
Now, does the whole environment fetching need to be documented somewhere? I see the comments, which are good, but I'm thinking of end-users who might want to use this new functionality.

@peytondmurray
Copy link
Contributor Author

100% agreed. Once the openapi.json reference is updated we'll have that. But maybe it make sense to start a document where some common uses cases are shown in examples?

@peytondmurray peytondmurray force-pushed the 857-add-user-env-listing branch from a71899b to 494aaf3 Compare August 29, 2024 22:55
@peytondmurray peytondmurray merged commit 6a55d3e into conda-incubator:main Sep 3, 2024
27 of 28 checks passed
@peytondmurray peytondmurray deleted the 857-add-user-env-listing branch September 3, 2024 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[ENH] - Add endpoint for listing all envs a user has access to
3 participants