Skip to content

PyPI Organization Account Team permission level #11590

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

Open
s-mm opened this issue Jun 14, 2022 · 5 comments
Open

PyPI Organization Account Team permission level #11590

s-mm opened this issue Jun 14, 2022 · 5 comments

Comments

@s-mm
Copy link
Contributor

s-mm commented Jun 14, 2022

When a team is assigned permission for a project, the semantics is different for a team as opposed to an individual user. This issue will discuss the permissions that can be assigned to a team, the phrasing to be used and explicitly specifying the hierarchy that exists in team level permissions.

@ewdurbin
Copy link
Member

ewdurbin commented Jun 17, 2022

Initially I think we should have two permission levels available for Teams:

  1. "Manager" or "Administrator", which matches the same permissions level as the current "Owner" Role for Collaborators. "Can upload releases. Can invite other collaborators. Can delete files, releases, or the entire project."
  2. "Uploader", which matches the same permissions level as the current "Maintainer" Role for Collaborators. "Can upload releases for a package. Cannot invite collaborators. Cannot delete files, releases, or the project."

These will match the current behaviors well, but should probably be refined down the line. As we currently only have manage:project and upload permissions there really isn't anything else we can do at this point.

Alternatively, we do away with named "roles" or "permission levels" for teams completely and allow arbitrary permissions to be assigned to a given Team. At this point it would just be the two above. Thoughts @di @dstufft ?

@ghost

This comment was marked as off-topic.

@divbzero
Copy link
Contributor

divbzero commented Jun 29, 2022

Summarizing the discussion from an organization accounts project meeting on 28 Jun 2022 and outlining potential next steps…

Current State

We currently have two roles defined for projects:

  1. Owner — Can upload releases. Can invite other collaborators. Can delete files, releases, or the entire project.
  2. Maintainer — Can upload releases for a package. Cannot invite collaborators. Cannot delete files, releases, or the project.

The two roles map to two underlying permissions:

  1. Users with the Owner role for a project are granted "manage:project" and "upload" permissions.
  2. Users with the Maintainer role for a project are granted "upload" permission.

The role-to-permission mapping can be seen directly in the __acl__ method for Project:

if role_name == "Owner":
acls.append((Allow, f"user:{user_id}", ["manage:project", "upload"]))
else:
acls.append((Allow, f"user:{user_id}", ["upload"]))

Future State

The potential next steps we discussed boiled down to three questions.

Do we allow users to configure the underlying permissions directly, or do we keep the current approach and allow users to specify roles only?

From a UX standpoint, it’s probably simpler to allow most users to specify roles only, especially if there are only two underlying permissions. In the future, we could consider allowing advanced users to define custom roles and select which underlying permissions to grant for each role.

(This mapping of roles to permissions is similar to how GitHub approaches repository access control. In GitHub, teams or users can be assigned Read, Triage, Write, Maintain, or Admin roles, and each role corresponds to a set of permissions. GitHub also appears to provide a way for advanced users to define custom roles.)

What do we label the underlying permissions?

In the previous comment @ewdurbin proposed:

  • Manager or Administrator which seems to correspond to the current "manage:project" permission.
  • Uploader which seems to correspond to the current "upload" permission.

If possible, we should define the display labels in one place so we can update them easily.

Could we create a catalog that defines roles and permissions in a single place?

In the near term, it could be something like a dictionary in the Python code that catalogs what roles we have and which permissions they map to. This would make it easy for us to change the roles-to-permissions mapping, add comments to document intended use, or update the display labels if needed.

In the long term, we could move the roles-to-permissions mapping to the database to allow advanced users to define custom roles.

Near Term: Catalog Roles in Code

In the near term, we could define an Enum of permissions that could be used in the __acl__ method of database models and in the permission attribute of @view_config and @view_defaults decorators. Using an Enum requires a bit more boilerplate but would allow us to define all roles and permissions in a single file, change display labels by editing that one file, catch typos with type checking, and harness code completion in text editors.

We could define the permissions with an Enum like:

# warehouse/permissions.py

import enum

from gettext import gettext as _


class StrEnum(str, enum.Enum):
    """Base class for Enum with string value and display label."""
    # NAME = ("string-value", _("Display Label"))
    def __new__(cls, value, label):
        obj = str.__new__(cls, value)
        obj._value_ = value
        obj.label = label
        return obj


class ProjectPermission(StrEnum):
    # NAME = ("pyramid:permission", _("Display Label"))
    MANAGE = ("manage:project", _("Administrator"))
    UPLOAD = ("upload", _("Uploader"))


class ProjectRole(StrEnum):
    # NAME = ("DatabaseValue", _("Display Label"))
    OWNER = ("Owner", _("Owner"))
    MAINTAINER = ("Maintainer", _("Maintainer"))


project_permissions = {
    ProjectRole.OWNER: [ProjectPermission.MANAGE, ProjectPermission.UPLOAD],
    ProjectRole.MAINTAINER: [ProjectPermission.UPLOAD],
}

Use the permissions for __acl__:

# warehouse/packaging/models.py

from warehouse.permissions import project_permissions


class Project(db.Model):
    ...
    
    def __acl__(self):
        ...
        for user_id, role_name in sorted(...):
		        acls.append((Allow, f"user:{user_id}", project_permissions[role_name]))
        ...

Use the permissions for @view_config and @view_defaults:

# warehouse/manage/views.py

from warehouse.organization.models import Project
from warehouse.permissions import ProjectPermission


@view_config(
    route_name="manage.project.delete_project",
    context=Project,
    uses_session=True,
    require_methods=["POST"],
    permission=ProjectPermission.MANAGE,
    has_translations=True,
    require_reauth=True,
)
def delete_project(project, request):
    ...

Use the permissions for HTML templates:

# warehouse/templates/manage/roles.html

<div class="callout-block">
  <p>{% trans %}There are two possible roles for collaborators:{% endtrans %}</p>
  <dl>
    <dt>{{ gettext(ProjectRole.MAINTAINER.label) }}</dt>
    <dd>{% trans %}Can upload releases for a package. Cannot invite collaborators. Cannot delete files, releases, or the project.{% endtrans %}</dd>
    <dt>{{ gettext(ProjectRole.OWNER.label) }}</dt>
    <dd>{% trans %}Can upload releases. Can invite other collaborators. Can delete files, releases, or the entire project.{% endtrans %}</dd>
  </dl>
</div>

And change display labels everywhere by editing the permissions file:

--- a/warehouse/permissions.py
+++ b/warehouse/permissions.py

 class ProjectRole(StrEnum):
     # NAME = ("DatabaseValue", _("Display Label"))
-    OWNER = ("Owner", _("Owner"))
-    MAINTAINER = ("Maintainer", _("Maintainer"))
+    OWNER = ("Owner", _("Administrator"))
+    MAINTAINER = ("Maintainer", _("Release Manager"))

Long Term: Configure Roles in Database

In the long term, we could make roles configurable by storing role definitions in the database:

# warehouse/packaging/models.py

from sqlalchemy import Column, orm
from sqlalchemy.types import ARRAY, Enum, Text

from warehouse.permissions import ProjectPermission, ProjectRole


class RoleDefinition(db.Model)
    __tablename__ = "role_definitions"
    
    name = Column(
        Enum(ProjectRole, values_callable=lambda x: [e.value for e in x]),
        nullable=False,
    )
    label = Column(
        Text,
        nullable=False,
    )
    permissions = Column(
        ARRAY(Enum(ProjectPermission, values_callable=lambda x: [e.value for e in x])),
        nullable=False,
    )


class Project(db.Model):
    ...
    
    def __acl__(self):
        ...
        session = orm.object_session(self)
        project_permissions = {
            ProjectRole[rd.name]: [ProjectPermission[p] for p in rd.permissions]
            for rd in session.query(RoleDefinition).all()
        }
        for user_id, role_name in sorted(...):
		        acls.append((Allow, f"user:{user_id}", project_permissions[role_name]))
        ...

@ewdurbin
Copy link
Member

ewdurbin commented Jul 5, 2022

Thank you for this extensive research and commentary @divbzero.

I agree with your short term assessment. For the time being we have no immediate plans for expanded permission on Projects. So for the purposes of the organization work, that is sufficient for the time being.

I think the RoleDefinition described is an excellent path forward for arbitrary permission sets in the future as PyPI grows to have more features that need ACLs.

@pypi/warehouse-admins do you have thoughts on how we should approach this?

@divbzero
Copy link
Contributor

As a separate point of reference, GitHub implemented custom repository roles earlier this year.

divbzero added a commit to divbzero/warehouse that referenced this issue Sep 20, 2022
- {Administer => Owner}
- {Upload => Maintainer}

Eventually, we want to map the roles to underlying permissions as
discussed in pypi#11590.
ewdurbin pushed a commit to divbzero/warehouse that referenced this issue Sep 20, 2022
- {Administer => Owner}
- {Upload => Maintainer}

Eventually, we want to map the roles to underlying permissions as
discussed in pypi#11590.
ewdurbin pushed a commit that referenced this issue Sep 20, 2022
- {Administer => Owner}
- {Upload => Maintainer}

Eventually, we want to map the roles to underlying permissions as
discussed in #11590.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants