Merge pull request galaxyproject#18966 from jdavcs/dev_private_roles2

Decouple user email from role name

client/src/api/schema/schema.ts

@@ -4795,6 +4795,26 @@ export interface paths {
         patch?: never;
         trace?: never;
     };
+    "/api/users/{user_id}/roles": {
+        parameters: {
+            query?: never;
+            header?: never;
+            path?: never;
+            cookie?: never;
+        };
+        /**
+         * Get User Roles
+         * @description Return a collection of roles associated with this user. Only admins can see user roles.
+         */
+        get: operations["get_user_roles_api_users__user_id__roles_get"];
+        put?: never;
+        post?: never;
+        delete?: never;
+        options?: never;
+        head?: never;
+        patch?: never;
+        trace?: never;
+    };
     "/api/users/{user_id}/send_activation_email": {
         parameters: {
             query?: never;
@@ -13264,7 +13284,7 @@ export interface components {
             page_limit: number;
             /**
              * Roles
-             * @description A list available roles that can be assigned to a particular permission.
+             * @description A list containing available roles that can be assigned to a particular permission.
              */
             roles: components["schemas"]["BasicRoleModel"][];
             /**
@@ -34117,6 +34137,50 @@ export interface operations {
             };
         };
     };
+    get_user_roles_api_users__user_id__roles_get: {
+        parameters: {
+            query?: never;
+            header?: {
+                /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */
+                "run-as"?: string | null;
+            };
+            path: {
+                /** @description The ID of the user. */
+                user_id: string;
+            };
+            cookie?: never;
+        };
+        requestBody?: never;
+        responses: {
+            /** @description Successful Response */
+            200: {
+                headers: {
+                    [name: string]: unknown;
+                };
+                content: {
+                    "application/json": components["schemas"]["RoleListResponse"];
+                };
+            };
+            /** @description Request Error */
+            "4XX": {
+                headers: {
+                    [name: string]: unknown;
+                };
+                content: {
+                    "application/json": components["schemas"]["MessageExceptionModel"];
+                };
+            };
+            /** @description Server Error */
+            "5XX": {
+                headers: {
+                    [name: string]: unknown;
+                };
+                content: {
+                    "application/json": components["schemas"]["MessageExceptionModel"];
+                };
+            };
+        };
+    };
     send_activation_email_api_users__user_id__send_activation_email_post: {
         parameters: {
             query?: never;

lib/galaxy/managers/roles.py

@@ -84,7 +84,11 @@ def create_role(self, trans: ProvidesUserContext, role_definition_model: RoleDef
         user_ids = role_definition_model.user_ids or []
         group_ids = role_definition_model.group_ids or []
 
-        stmt = select(Role).where(Role.name == name).limit(1)
+        stmt = (
+            select(Role)
+            .where(Role.name == name)  # type:ignore[arg-type,comparison-overlap]  # Role.name is a SA hybrid property
+            .limit(1)
+        )
         if trans.sa_session.scalars(stmt).first():
             raise Conflict(f"A role with that name already exists [{name}]")
 

lib/galaxy/managers/users.py

@@ -658,11 +658,8 @@ def get_or_create_remote_user(self, remote_user_email):
             remote_user_email = remote_user_email.lower()
         user = get_user_by_email(self.session(), remote_user_email, self.app.model.User)
         if user:
-            # GVK: June 29, 2009 - This is to correct the behavior of a previous bug where a private
-            # role and default user / history permissions were not set for remote users.  When a
-            # remote user authenticates, we'll look for this information, and if missing, create it.
-            if not self.app.security_agent.get_private_user_role(user):
-                self.app.security_agent.create_private_user_role(user)
+            # Ensure a private role and default permissions are set for remote users (remote user creation bug existed prior to 2009)
+            self.app.security_agent.get_private_user_role(user, auto_create=True)
             if self.app_type == "galaxy":
                 if not user.default_permissions:
                     self.app.security_agent.user_set_default_permissions(user)

lib/galaxy/model/__init__.py

@@ -109,6 +109,7 @@
     association_proxy,
     AssociationProxy,
 )
+from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy.ext.orderinglist import ordering_list
 from sqlalchemy.orm import (
     aliased,
@@ -1233,10 +1234,7 @@ def is_authenticated(self):
 
     def attempt_create_private_role(self):
         session = object_session(self)
-        role_name = self.email
-        role_desc = f"Private Role for {self.email}"
-        role_type = Role.types.PRIVATE
-        role = Role(name=role_name, description=role_desc, type=role_type)
+        role = Role(type=Role.types.PRIVATE)
         assoc = UserRoleAssociation(self, role)
         session.add(assoc)
         with transaction(session):
@@ -3798,7 +3796,7 @@ class Role(Base, Dictifiable, RepresentById):
     id: Mapped[int] = mapped_column(primary_key=True)
     create_time: Mapped[datetime] = mapped_column(default=now, nullable=True)
     update_time: Mapped[datetime] = mapped_column(default=now, onupdate=now, nullable=True)
-    name: Mapped[Optional[str]] = mapped_column(String(255), index=True, unique=True)
+    _name: Mapped[str] = mapped_column("name", String(255), index=True)
     description: Mapped[Optional[str]] = mapped_column(TEXT)
     type: Mapped[Optional[str]] = mapped_column(String(40), index=True)
     deleted: Mapped[Optional[bool]] = mapped_column(index=True, default=False)
@@ -3817,8 +3815,25 @@ class types(str, Enum):
         ADMIN = "admin"
         SHARING = "sharing"
 
+    @staticmethod
+    def default_name(role_type):
+        return f"{role_type.value} role"
+
+    @hybrid_property
+    def name(self):
+        if self.type == Role.types.PRIVATE:
+            user_assocs = self.users
+            assert len(user_assocs) == 1, f"Did not find exactly one user for private role {self}"
+            return user_assocs[0].user.email
+        else:
+            return self._name
+
+    @name.setter  # type:ignore[no-redef]  # property setter
+    def name(self, name):
+        self._name = name
+
     def __init__(self, name=None, description=None, type=types.SYSTEM, deleted=False):
-        self.name = name
+        self.name = name or Role.default_name(type)
         self.description = description
         self.type = type
         self.deleted = deleted

lib/galaxy/model/migrations/alembic/versions_gxy/9a5207190a4d_remove_unique_constraint_from_role_name.py

@@ -0,0 +1,40 @@
+"""Remove unique constraint from role name, add not null constraint
+
+Revision ID: 9a5207190a4d
+Revises: a99a5b52ccb8
+Create Date: 2024-10-08 14:08:28.418055
+
+"""
+
+from galaxy.model.database_object_names import build_index_name
+from galaxy.model.migrations.util import (
+    alter_column,
+    create_index,
+    drop_index,
+    transaction,
+)
+
+# revision identifiers, used by Alembic.
+revision = "9a5207190a4d"
+down_revision = "a99a5b52ccb8"
+branch_labels = None
+depends_on = None
+
+
+table_name = "role"
+column_name = "name"
+index_name = build_index_name(table_name, [column_name])
+
+
+def upgrade():
+    with transaction():
+        drop_index(index_name, table_name)
+        alter_column(table_name, column_name, nullable=False)
+        create_index(index_name, table_name, [column_name])
+
+
+def downgrade():
+    with transaction():
+        drop_index(index_name, table_name)
+        alter_column(table_name, column_name, nullable=True)
+        create_index(index_name, table_name, [column_name], unique=True)

lib/galaxy/model/security.py

@@ -159,31 +159,9 @@ def get_valid_roles(self, trans, item, query=None, page=None, page_limit=None, i
             is_public_item = False
         # Admins can always choose from all non-deleted roles
         if trans.user_is_admin or trans.app.config.expose_user_email:
-            if trans.user_is_admin:
-                stmt = select(Role).where(Role.deleted == false())
-            else:
-                # User is not an admin but the configuration exposes all private roles to all users.
-                stmt = select(Role).where(and_(Role.deleted == false(), Role.type == Role.types.PRIVATE))
-            if search_query:
-                stmt = stmt.where(Role.name.like(search_query, escape="/"))
-
-            count_stmt = select(func.count()).select_from(stmt)
-            total_count = trans.sa_session.scalar(count_stmt)
-
-            if limit is not None:
-                # Takes the least number of results from beginning that includes the requested page
-                stmt = stmt.order_by(Role.name).limit(limit)
-                page_start = (page * page_limit) - page_limit
-                page_end = page_start + page_limit
-                if total_count < page_start + 1:
-                    # Return empty list if there are less results than the requested position
-                    roles = []
-                else:
-                    roles = trans.sa_session.scalars(stmt).all()
-                    roles = roles[page_start:page_end]
-            else:
-                stmt = stmt.order_by(Role.name)
-                roles = trans.sa_session.scalars(stmt).all()
+            roles = _get_valid_roles_exposed(
+                trans.sa_session, search_query, trans.user_is_admin, limit, page, page_limit
+            )
         # Non-admin and public item
         elif is_public_item:
             # Add the current user's private role
@@ -1526,7 +1504,6 @@ def _set_user_roles(self, user, role_ids):
         else:
             delete_stmt = delete_stmt.where(UserRoleAssociation.role_id != private_role.id)
         role_ids = self._filter_private_roles(role_ids)
-        # breakpoint()
 
         insert_values = [{"user_id": user.id, "role_id": role_id} for role_id in role_ids]
         self._set_associations(user, UserRoleAssociation, delete_stmt, insert_values)
@@ -1808,3 +1785,46 @@ def is_foreign_key_violation(error):
         # If this is a PostgreSQL foreign key error, then error.orig is an instance of psycopg2.errors.ForeignKeyViolation
         # and should have an attribute `pgcode` = 23503.
         return int(getattr(error.orig, "pgcode", -1)) == 23503
+
+
+def _get_valid_roles_exposed(session, search_query, is_admin, limit, page, page_limit):
+    """Case: trans.user_is_admin or trans.app.config.expose_user_email"""
+    stmt = select(Role).where(Role.deleted == false())
+
+    if not is_admin:
+        # User is not an admin but the configuration exposes all private roles to all users,
+        # so only private roles are returned.
+        stmt = stmt.where(Role.type == Role.types.PRIVATE)
+
+    if search_query:
+        stmt = stmt.where(Role.name.like(search_query, escape="/"))
+
+        # Also check against user emails for associated users of private roles ONLY
+        stmt2 = (
+            select(Role)
+            .join(Role.users)
+            .join(User)
+            .where(and_(Role.type == Role.types.PRIVATE, User.email.like(search_query, escape="/")))
+        )
+        stmt = stmt.union(stmt2)
+
+    count_stmt = select(func.count()).select_from(stmt)
+    total_count = session.scalar(count_stmt)
+
+    stmt = stmt.order_by(Role.name)
+
+    if limit is not None:
+        # Takes the least number of results from beginning that includes the requested page
+        stmt = stmt.limit(limit)
+        page_start = (page * page_limit) - page_limit
+        page_end = page_start + page_limit
+        if total_count < page_start + 1:
+            # Return empty list if there are less results than the requested position
+            return []
+
+    stmt = select(Role).from_statement(stmt)
+    roles = session.scalars(stmt).all()
+    if limit is not None:
+        roles = roles[page_start:page_end]
+
+    return roles

lib/galaxy/schema/schema.py

@@ -3124,7 +3124,7 @@ class LibraryAvailablePermissions(Model):
     roles: List[BasicRoleModel] = Field(
         ...,
         title="Roles",
-        description="A list available roles that can be assigned to a particular permission.",
+        description="A list containing available roles that can be assigned to a particular permission.",
     )
     page: int = Field(
         ...,

lib/galaxy/webapps/galaxy/api/roles.py

@@ -7,22 +7,18 @@
 from fastapi import Body
 
 from galaxy.managers.context import ProvidesUserContext
-from galaxy.managers.roles import RoleManager
-from galaxy.schema.fields import (
-    DecodedDatabaseIdField,
-    Security,
-)
+from galaxy.schema.fields import DecodedDatabaseIdField
 from galaxy.schema.schema import (
     RoleDefinitionModel,
     RoleListResponse,
     RoleModelResponse,
 )
-from galaxy.webapps.base.controller import url_for
 from galaxy.webapps.galaxy.api import (
     depends,
     DependsOnTrans,
     Router,
 )
+from galaxy.webapps.galaxy.services.roles import RolesService
 
 log = logging.getLogger(__name__)
 
@@ -32,48 +28,32 @@
 router = Router(tags=["roles"])
 
 
-def role_to_model(role):
-    item = role.to_dict(view="element")
-    role_id = Security.security.encode_id(role.id)
-    item["url"] = url_for("role", id=role_id)
-    return RoleModelResponse(**item)
-
-
 @router.cbv
 class FastAPIRoles:
-    role_manager: RoleManager = depends(RoleManager)
+    service: RolesService = depends(RolesService)
 
     @router.get("/api/roles")
     def index(self, trans: ProvidesUserContext = DependsOnTrans) -> RoleListResponse:
-        roles = self.role_manager.list_displayable_roles(trans)
-        return RoleListResponse(root=[role_to_model(r) for r in roles])
+        return self.service.get_index(trans=trans)
 
     @router.get("/api/roles/{id}")
     def show(self, id: DecodedDatabaseIdField, trans: ProvidesUserContext = DependsOnTrans) -> RoleModelResponse:
-        role = self.role_manager.get(trans, id)
-        return role_to_model(role)
+        return self.service.show(trans, id)
 
     @router.post("/api/roles", require_admin=True)
     def create(
         self, trans: ProvidesUserContext = DependsOnTrans, role_definition_model: RoleDefinitionModel = Body(...)
     ) -> RoleModelResponse:
-        role = self.role_manager.create_role(trans, role_definition_model)
-        return role_to_model(role)
+        return self.service.create(trans, role_definition_model)
 
     @router.delete("/api/roles/{id}", require_admin=True)
     def delete(self, id: DecodedDatabaseIdField, trans: ProvidesUserContext = DependsOnTrans) -> RoleModelResponse:
-        role = self.role_manager.get(trans, id)
-        role = self.role_manager.delete(trans, role)
-        return role_to_model(role)
+        return self.service.delete(trans, id)
 
     @router.post("/api/roles/{id}/purge", require_admin=True)
     def purge(self, id: DecodedDatabaseIdField, trans: ProvidesUserContext = DependsOnTrans) -> RoleModelResponse:
-        role = self.role_manager.get(trans, id)
-        role = self.role_manager.purge(trans, role)
-        return role_to_model(role)
+        return self.service.purge(trans, id)
 
     @router.post("/api/roles/{id}/undelete", require_admin=True)
     def undelete(self, id: DecodedDatabaseIdField, trans: ProvidesUserContext = DependsOnTrans) -> RoleModelResponse:
-        role = self.role_manager.get(trans, id)
-        role = self.role_manager.undelete(trans, role)
-        return role_to_model(role)
+        return self.service.undelete(trans, id)