From 8837b8ea79366bf3aa8ca805008dc3ff35cdc145 Mon Sep 17 00:00:00 2001 From: hagen-danswer Date: Tue, 24 Dec 2024 10:49:58 -0800 Subject: [PATCH] Curators can now update the curator relationship (#3536) * Curators can now update the curator relationship * mypy * mypy * whoops haha --- backend/ee/onyx/db/user_group.py | 100 +++++++++++++++--- backend/ee/onyx/server/user_group/api.py | 3 +- backend/onyx/server/documents/cc_pair.py | 2 +- backend/onyx/server/documents/connector.py | 6 +- backend/onyx/server/documents/credential.py | 2 +- .../onyx/server/features/document_set/api.py | 4 +- .../admin/groups/[groupId]/GroupDisplay.tsx | 85 ++++++++++----- 7 files changed, 158 insertions(+), 44 deletions(-) diff --git a/backend/ee/onyx/db/user_group.py b/backend/ee/onyx/db/user_group.py index 3463d717c38..1b1fcca74b4 100644 --- a/backend/ee/onyx/db/user_group.py +++ b/backend/ee/onyx/db/user_group.py @@ -122,7 +122,7 @@ def _cleanup_document_set__user_group_relationships__no_commit( ) -def validate_user_creation_permissions( +def validate_object_creation_for_user( db_session: Session, user: User | None, target_group_ids: list[int] | None = None, @@ -440,32 +440,108 @@ def remove_curator_status__no_commit(db_session: Session, user: User) -> None: _validate_curator_status__no_commit(db_session, [user]) -def update_user_curator_relationship( +def _validate_curator_relationship_update_requester( db_session: Session, user_group_id: int, - set_curator_request: SetCuratorRequest, + user_making_change: User | None = None, ) -> None: - user = fetch_user_by_id(db_session, set_curator_request.user_id) - if not user: - raise ValueError(f"User with id '{set_curator_request.user_id}' not found") + """ + This function validates that the user making the change has the necessary permissions + to update the curator relationship for the target user in the given user group. + """ - if user.role == UserRole.ADMIN: + if user_making_change is None or user_making_change.role == UserRole.ADMIN: + return + + # check if the user making the change is a curator in the group they are changing the curator relationship for + user_making_change_curator_groups = fetch_user_groups_for_user( + db_session=db_session, + user_id=user_making_change.id, + # only check if the user making the change is a curator if they are a curator + # otherwise, they are a global_curator and can update the curator relationship + # for any group they are a member of + only_curator_groups=user_making_change.role == UserRole.CURATOR, + ) + requestor_curator_group_ids = [ + group.id for group in user_making_change_curator_groups + ] + if user_group_id not in requestor_curator_group_ids: raise ValueError( - f"User '{user.email}' is an admin and therefore has all permissions " + f"user making change {user_making_change.email} is not a curator," + f" admin, or global_curator for group '{user_group_id}'" + ) + + +def _validate_curator_relationship_update_request( + db_session: Session, + user_group_id: int, + target_user: User, +) -> None: + """ + This function validates that the curator_relationship_update request itself is valid. + """ + if target_user.role == UserRole.ADMIN: + raise ValueError( + f"User '{target_user.email}' is an admin and therefore has all permissions " "of a curator. If you'd like this user to only have curator permissions, " "you must update their role to BASIC then assign them to be CURATOR in the " "appropriate groups." ) + elif target_user.role == UserRole.GLOBAL_CURATOR: + raise ValueError( + f"User '{target_user.email}' is a global_curator and therefore has all " + "permissions of a curator for all groups. If you'd like this user to only " + "have curator permissions for a specific group, you must update their role " + "to BASIC then assign them to be CURATOR in the appropriate groups." + ) + elif target_user.role not in [UserRole.CURATOR, UserRole.BASIC]: + raise ValueError( + f"This endpoint can only be used to update the curator relationship for " + "users with the CURATOR or BASIC role. \n" + f"Target user: {target_user.email} \n" + f"Target user role: {target_user.role} \n" + ) + # check if the target user is in the group they are changing the curator relationship for requested_user_groups = fetch_user_groups_for_user( db_session=db_session, - user_id=set_curator_request.user_id, + user_id=target_user.id, only_curator_groups=False, ) - group_ids = [group.id for group in requested_user_groups] if user_group_id not in group_ids: - raise ValueError(f"user is not in group '{user_group_id}'") + raise ValueError( + f"target user {target_user.email} is not in group '{user_group_id}'" + ) + + +def update_user_curator_relationship( + db_session: Session, + user_group_id: int, + set_curator_request: SetCuratorRequest, + user_making_change: User | None = None, +) -> None: + target_user = fetch_user_by_id(db_session, set_curator_request.user_id) + if not target_user: + raise ValueError(f"User with id '{set_curator_request.user_id}' not found") + + _validate_curator_relationship_update_request( + db_session=db_session, + user_group_id=user_group_id, + target_user=target_user, + ) + + _validate_curator_relationship_update_requester( + db_session=db_session, + user_group_id=user_group_id, + user_making_change=user_making_change, + ) + + logger.info( + f"user_making_change={user_making_change.email if user_making_change else 'None'} is " + f"updating the curator relationship for user={target_user.email} " + f"in group={user_group_id} to is_curator={set_curator_request.is_curator}" + ) relationship_to_update = ( db_session.query(User__UserGroup) @@ -486,7 +562,7 @@ def update_user_curator_relationship( ) db_session.add(relationship_to_update) - _validate_curator_status__no_commit(db_session, [user]) + _validate_curator_status__no_commit(db_session, [target_user]) db_session.commit() diff --git a/backend/ee/onyx/server/user_group/api.py b/backend/ee/onyx/server/user_group/api.py index 73431e1f65f..4f01a21adbf 100644 --- a/backend/ee/onyx/server/user_group/api.py +++ b/backend/ee/onyx/server/user_group/api.py @@ -83,7 +83,7 @@ def patch_user_group( def set_user_curator( user_group_id: int, set_curator_request: SetCuratorRequest, - _: User | None = Depends(current_admin_user), + user: User | None = Depends(current_curator_or_admin_user), db_session: Session = Depends(get_session), ) -> None: try: @@ -91,6 +91,7 @@ def set_user_curator( db_session=db_session, user_group_id=user_group_id, set_curator_request=set_curator_request, + user_making_change=user, ) except ValueError as e: logger.error(f"Error setting user curator: {e}") diff --git a/backend/onyx/server/documents/cc_pair.py b/backend/onyx/server/documents/cc_pair.py index dd282e2fe70..bf9e61864c7 100644 --- a/backend/onyx/server/documents/cc_pair.py +++ b/backend/onyx/server/documents/cc_pair.py @@ -510,7 +510,7 @@ def associate_credential_to_connector( db_session: Session = Depends(get_session), ) -> StatusResponse[int]: fetch_ee_implementation_or_noop( - "onyx.db.user_group", "validate_user_creation_permissions", None + "onyx.db.user_group", "validate_object_creation_for_user", None )( db_session=db_session, user=user, diff --git a/backend/onyx/server/documents/connector.py b/backend/onyx/server/documents/connector.py index 4fab885195a..065576f73d7 100644 --- a/backend/onyx/server/documents/connector.py +++ b/backend/onyx/server/documents/connector.py @@ -680,7 +680,7 @@ def create_connector_from_model( _validate_connector_allowed(connector_data.source) fetch_ee_implementation_or_noop( - "onyx.db.user_group", "validate_user_creation_permissions", None + "onyx.db.user_group", "validate_object_creation_for_user", None )( db_session=db_session, user=user, @@ -716,7 +716,7 @@ def create_connector_with_mock_credential( tenant_id: str = Depends(get_current_tenant_id), ) -> StatusResponse: fetch_ee_implementation_or_noop( - "onyx.db.user_group", "validate_user_creation_permissions", None + "onyx.db.user_group", "validate_object_creation_for_user", None )( db_session=db_session, user=user, @@ -776,7 +776,7 @@ def update_connector_from_model( try: _validate_connector_allowed(connector_data.source) fetch_ee_implementation_or_noop( - "onyx.db.user_group", "validate_user_creation_permissions", None + "onyx.db.user_group", "validate_object_creation_for_user", None )( db_session=db_session, user=user, diff --git a/backend/onyx/server/documents/credential.py b/backend/onyx/server/documents/credential.py index af39e9ea3c7..51d9643dc77 100644 --- a/backend/onyx/server/documents/credential.py +++ b/backend/onyx/server/documents/credential.py @@ -122,7 +122,7 @@ def create_credential_from_model( ) -> ObjectCreationIdResponse: if not _ignore_credential_permissions(credential_info.source): fetch_ee_implementation_or_noop( - "onyx.db.user_group", "validate_user_creation_permissions", None + "onyx.db.user_group", "validate_object_creation_for_user", None )( db_session=db_session, user=user, diff --git a/backend/onyx/server/features/document_set/api.py b/backend/onyx/server/features/document_set/api.py index 277db6b4891..0ba20ff0d84 100644 --- a/backend/onyx/server/features/document_set/api.py +++ b/backend/onyx/server/features/document_set/api.py @@ -31,7 +31,7 @@ def create_document_set( db_session: Session = Depends(get_session), ) -> int: fetch_ee_implementation_or_noop( - "onyx.db.user_group", "validate_user_creation_permissions", None + "onyx.db.user_group", "validate_object_creation_for_user", None )( db_session=db_session, user=user, @@ -56,7 +56,7 @@ def patch_document_set( db_session: Session = Depends(get_session), ) -> None: fetch_ee_implementation_or_noop( - "onyx.db.user_group", "validate_user_creation_permissions", None + "onyx.db.user_group", "validate_object_creation_for_user", None )( db_session=db_session, user=user, diff --git a/web/src/app/ee/admin/groups/[groupId]/GroupDisplay.tsx b/web/src/app/ee/admin/groups/[groupId]/GroupDisplay.tsx index 0085d9ec874..b6c97e0f1fa 100644 --- a/web/src/app/ee/admin/groups/[groupId]/GroupDisplay.tsx +++ b/web/src/app/ee/admin/groups/[groupId]/GroupDisplay.tsx @@ -38,6 +38,7 @@ import { BookmarkIcon, RobotIcon } from "@/components/icons/icons"; import { AddTokenRateLimitForm } from "./AddTokenRateLimitForm"; import { GenericTokenRateLimitTable } from "@/app/admin/token-rate-limits/TokenRateLimitTables"; import { useUser } from "@/components/user/UserProvider"; +import { GenericConfirmModal } from "@/components/modals/GenericConfirmModal"; interface GroupDisplayProps { users: User[]; @@ -68,8 +69,13 @@ const UserRoleDropdown = ({ return user.role; }); const [isSettingRole, setIsSettingRole] = useState(false); + const [showDemoteConfirm, setShowDemoteConfirm] = useState(false); + const [pendingRoleChange, setPendingRoleChange] = useState( + null + ); + const { user: currentUser } = useUser(); - const handleChange = async (value: string) => { + const applyRoleChange = async (value: string) => { if (value === localRole) return; if (value === UserRole.BASIC || value === UserRole.CURATOR) { setIsSettingRole(true); @@ -95,30 +101,61 @@ const UserRoleDropdown = ({ } }; - const isEditable = - (user.role === UserRole.BASIC || user.role === UserRole.CURATOR) && isAdmin; + const handleChange = (value: string) => { + if (value === UserRole.BASIC && user.id === currentUser?.id) { + setPendingRoleChange(value); + setShowDemoteConfirm(true); + } else { + applyRoleChange(value); + } + }; - if (isEditable) { - return ( -
- -
- ); - } else { - return
{USER_ROLE_LABELS[localRole]}
; - } + const isEditable = + user.role === UserRole.BASIC || user.role === UserRole.CURATOR; + + return ( + <> + {/* Confirmation modal - only shown when users try to demote themselves */} + {showDemoteConfirm && pendingRoleChange && ( + { + // Cancel the role change if user dismisses modal + setShowDemoteConfirm(false); + setPendingRoleChange(null); + }} + onConfirm={() => { + // Apply the role change if user confirms + setShowDemoteConfirm(false); + applyRoleChange(pendingRoleChange); + setPendingRoleChange(null); + }} + /> + )} + + {isEditable ? ( +
+ +
+ ) : ( +
{USER_ROLE_LABELS[localRole]}
+ )} + + ); }; export const GroupDisplay = ({