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

API : update a namespace's metadata and role mappings #508

Merged
merged 6 commits into from
Jul 25, 2023

Conversation

pierrotsmnrd
Copy link
Contributor

This PR adds a new API endpoint to update a namespace's role mappings and metadata.

the PUT /conda-store/api/v1/namespace/{namespace}/ endpoint takes a JSON payload to update the metadata, the role mappings, or both.

  • update the metadata :
{
  "metadata": {"my_key":"my_value"}
}
  • update the role mappings :
{
  "role_mappings": {
    "admin/admin": ["admin", "viewer", "developer"],
    "admin/*": ["viewer"],
    "demo/*": ["developer"]
  }
}
  • update both :
{
  "metadata": {"my_key":"my_value"},
  "role_mappings": {
    "admin/admin": ["admin"],
    "admin/*": ["viewer"],
    "demo/*": ["developer"]
  }
}

@costrouc tell me if that works for you or if you'd see another approach or pattern.

According to the original issue #491 :

  • namespace update rest api method that uses the namespace update
  • create, list, delete for adding namespace role mappings. I think you should only be able to edit namespace role mappings if you have namespace::update permissions. If you have namespace::read it should be sufficient to view members.

We now need to add an endpoint to list the role mappings.

@@ -34,6 +34,7 @@ class Permissions(enum.Enum):
BUILD_DELETE = "build::delete"
NAMESPACE_CREATE = "namespace::create"
NAMESPACE_READ = "namespace::read"
NAMESPACE_UPDATE = "namespace::update"
Copy link
Member

@costrouc costrouc Jul 24, 2023

Choose a reason for hiding this comment

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

@pierrotsmnrd this PR looks great to me. No real issues. My questions is on the granularity of permissions:

Should we have one permission namespace::update or should we have more? I think it is always easier to join permissions than to split them later on. Roles already provide an easy way to group them. To me having the ability to change a namespaces metadata and giving users permissions to a namespace are at much different permission levels.

So for example:

  • namespace::update update the metadata for a namespace
  • namespace-role-mapping::read
  • namespace-role-mapping::create
  • namespace-role-mapping::delete

I'm not entirely sure on the naming. This would allow us to give people viewer access to namespaces but not see who else is a member etc. Not sure if there is an exact use case for now but it could be useful to give people view access to a namespace but not see who is there.

def update_namespace(
db,
name: str,
metadata_: Optional[Dict] | None = None,
Copy link
Member

@costrouc costrouc Jul 24, 2023

Choose a reason for hiding this comment

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

Let's not use | since this isn't backwards compatible with 3.9. Use typing.Union

@pierrotsmnrd pierrotsmnrd changed the title [WIP] API : update a namespace's metadata and role mappings API : update a namespace's metadata and role mappings Jul 25, 2023
Copy link
Member

@costrouc costrouc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @pierrotsmnrd

@costrouc costrouc merged commit a832882 into main Jul 25, 2023
7 checks passed
@costrouc costrouc deleted the role_mapping_api branch July 25, 2023 20:22
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.

2 participants