diff --git a/.agents/feature-map/components/access-control.md b/.agents/feature-map/components/access-control.md index 04df478f22..cd983d833d 100644 --- a/.agents/feature-map/components/access-control.md +++ b/.agents/feature-map/components/access-control.md @@ -155,7 +155,10 @@ OpenSearch chunk document - `UserGroup` (`onyx/db/models.py`, EE-managed via `ee/onyx/db/user_group.py`): a named group. Two are seeded and protected as "default": Admin and Basic (`onyx/db/user_group.py:assert_group_config_is_editable`, - `assert_not_shared_with_default_group`). Sharing a resource with the Basic + `assert_not_shared_with_default_group`). The one write a default group takes + besides membership is the permission toggle on Basic, limited to + `auth/permissions.py:DEFAULT_GROUP_TOGGLEABLE_PERMISSIONS` + (`onyx/db/user_group.py:assert_group_permissions_editable`). Sharing a resource with the Basic default group is refused outright: use the resource's own `is_public` flag instead, since Basic holds every user. The Community downgrade (`ee/onyx/db/community_downgrade.py:remove_custom_user_groups__no_commit`, diff --git a/.agents/feature-map/components/auth-and-identity.md b/.agents/feature-map/components/auth-and-identity.md index f6e3f6cadd..fdc9006423 100644 --- a/.agents/feature-map/components/auth-and-identity.md +++ b/.agents/feature-map/components/auth-and-identity.md @@ -110,7 +110,7 @@ the coding-agent sandbox). |---|---|---|---| | CRUD | `/admin/api-key*` | `server/api_key/api.py` | `Permission.MANAGE_SERVICE_ACCOUNT_API_KEYS`. Creating or updating a key is **admin-equivalent by design**: `group_ids` is uncapped, so a holder can add a key to the Admin group (`api.py:create_api_key` comment). | | GET | `/user/pats/scopes` | `server/pat/api.py:list_selectable_scopes` | `Permission.BASIC_ACCESS`. Drops `use:llm_gateway` from the list while the workspace `llm_gateway_enabled` setting is off. | -| GET/POST/DELETE | `/user/pats` | `server/pat/api.py` | List/create needs `CREATE_USER_API_KEYS` (create) or `BASIC_ACCESS` (list/delete); a user only ever sees/revokes their own. Minting a PAT with `use:llm_gateway` while the setting is off is rejected with `INVALID_INPUT` (`server/pat/api.py:_validate_assignable_scopes`). | +| GET/POST/DELETE | `/user/pats` | `server/pat/api.py` | Create needs `CREATE_USER_API_KEYS`. The seeded Basic group grants it by default, and a full admin can switch it off or on from the Basic group's page (`auth/permissions.py:DEFAULT_GROUP_TOGGLEABLE_PERMISSIONS`). The same permission gates MCP OAuth consent (`server/oauth_provider/api.py`), so the toggle covers both. List and delete need `BASIC_ACCESS`, and a user only ever sees or revokes their own. Minting a PAT with `use:llm_gateway` while the setting is off is rejected with `INVALID_INPUT` (`server/pat/api.py:_validate_assignable_scopes`). | | GET/PUT | `/admin/security`, GET `/admin/security/pinned-fields` | `server/security/api.py` | `FULL_ADMIN_PANEL_ACCESS`. Workspace security overrides: password policy, `password_auth_enabled`, `valid_email_domains`, JWT validation fields, SSRF level, incognito availability, and `allow_connector_group_restrictions`. Env-pinned fields win over stored values. Turning on `allow_connector_group_restrictions` needs the Business tier. | | CRUD | `/admin/user-group*` | `ee/onyx/server/user_group/api.py` | See [[access-control]] §2; this is where group membership, and thus `PermissionGrant`-derived capability, is actually assigned. | | GET/POST/PATCH | `/admin/sso-providers*`, `/admin/sso-providers/{id}/domains` | `server/manage/sso/api.py` | `FULL_ADMIN_PANEL_ACCESS` (exact gating per route). Domain-routing SSO providers beyond the first require business tier (`_require_business_tier_for_additional_enabled_provider`). | diff --git a/backend/alembic/versions/b6d920407aa6_grant_create_pat_to_basic_group.py b/backend/alembic/versions/b6d920407aa6_grant_create_pat_to_basic_group.py new file mode 100644 index 0000000000..3ed209d94d --- /dev/null +++ b/backend/alembic/versions/b6d920407aa6_grant_create_pat_to_basic_group.py @@ -0,0 +1,146 @@ +"""grant create:user_api_keys to the Basic group + +Every member of the seeded Basic group may mint personal access tokens by default. The +grant is written as a SYSTEM row on the group and appended to its members' +stored permissions, since those are only recomputed on membership and grant +edits. + +Revision ID: b6d920407aa6 +Revises: c4d8e2f1a9b3 + +""" + +from uuid import UUID + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.dialects import postgresql +from sqlalchemy.dialects.postgresql import insert as pg_insert + +# revision identifiers, used by Alembic. +revision = "b6d920407aa6" +down_revision = "c4d8e2f1a9b3" +branch_labels = None +depends_on = None + +BASIC_GROUP_NAME = "Basic" +PERMISSION = "create:user_api_keys" +GRANT_SOURCE = "SYSTEM" + +user_group_table: sa.TableClause = sa.table( + "user_group", + sa.column("id", sa.Integer), + sa.column("name", sa.String), + sa.column("is_default", sa.Boolean), +) + +permission_grant_table: sa.TableClause = sa.table( + "permission_grant", + sa.column("group_id", sa.Integer), + sa.column("permission", sa.String), + sa.column("grant_source", sa.String), + sa.column("is_deleted", sa.Boolean), +) + +user__user_group_table: sa.TableClause = sa.table( + "user__user_group", + sa.column("user_group_id", sa.Integer), + sa.column("user_id", sa.Uuid), +) + +user_table: sa.TableClause = sa.table( + "user", + sa.column("id", sa.Uuid), + sa.column("effective_permissions", postgresql.JSONB), +) + +permission_json: sa.BindParameter[list[str]] = sa.literal( + [PERMISSION], type_=postgresql.JSONB +) + +basic_group_id: sa.ScalarSelect[int] = ( + sa.select(user_group_table.c.id) + .where( + user_group_table.c.name == BASIC_GROUP_NAME, + user_group_table.c.is_default.is_(True), + ) + .scalar_subquery() +) + +basic_member_ids: sa.Select[tuple[UUID]] = sa.select( + user__user_group_table.c.user_id +).where(user__user_group_table.c.user_group_id == basic_group_id) + + +def upgrade() -> None: + conn: sa.engine.Connection = op.get_bind() + + # A soft-deleted row would block the insert on the unique key, so revive it. + conn.execute( + pg_insert(permission_grant_table) + .values( + group_id=basic_group_id, + permission=PERMISSION, + grant_source=GRANT_SOURCE, + is_deleted=False, + ) + .on_conflict_do_update( + index_elements=["group_id", "permission"], + set_={"is_deleted": False}, + ) + ) + + conn.execute( + user_table.update() + .where( + user_table.c.id.in_(basic_member_ids), + sa.not_(user_table.c.effective_permissions.contains(permission_json)), + ) + .values( + effective_permissions=user_table.c.effective_permissions.op("||")( + permission_json + ) + ) + ) + + +def downgrade() -> None: + conn: sa.engine.Connection = op.get_bind() + + conn.execute( + sa.delete(permission_grant_table).where( + permission_grant_table.c.group_id == basic_group_id, + permission_grant_table.c.permission == PERMISSION, + ) + ) + + # Members who still hold the permission through another group keep it. + other_grant_holders = ( + sa.select(user__user_group_table.c.user_id) + .select_from( + user__user_group_table.join( + permission_grant_table, + permission_grant_table.c.group_id + == user__user_group_table.c.user_group_id, + ) + ) + .where( + user__user_group_table.c.user_id.is_not(None), + permission_grant_table.c.permission == PERMISSION, + permission_grant_table.c.is_deleted.is_(False), + ) + ) + conn.execute( + user_table.update() + .where( + user_table.c.id.in_(basic_member_ids), + user_table.c.id.not_in(other_grant_holders), + ) + .values( + # jsonb - text drops the element. A JSONB bind would compile to the + # undefined jsonb - jsonb. + effective_permissions=user_table.c.effective_permissions.op("-")( + sa.literal(PERMISSION, type_=sa.Text) + ) + ) + ) diff --git a/backend/ee/onyx/server/user_group/api.py b/backend/ee/onyx/server/user_group/api.py index 65ce2a21a2..0b1997c3dd 100644 --- a/backend/ee/onyx/server/user_group/api.py +++ b/backend/ee/onyx/server/user_group/api.py @@ -75,7 +75,11 @@ from onyx.db.engine.sql_engine import get_session from onyx.db.enums import ConnectorManageRole, Permission, PermissionAuthority from onyx.db.models import User from onyx.db.persona import fetch_persona_by_id_for_user, get_personas_by_ids -from onyx.db.user_group import assert_group_config_is_editable +from onyx.db.user_group import ( + assert_group_config_is_editable, + assert_group_permissions_editable, + group_permission_toggles_allowed, +) from onyx.error_handling.error_codes import OnyxErrorCode from onyx.error_handling.exceptions import OnyxError from onyx.server.security.store import get_security_settings @@ -141,6 +145,7 @@ def list_user_groups( is_user_groups_admin=is_user_groups_admin, is_full_admin=is_full_admin, is_default=user_group.is_default, + permission_toggles_allowed=group_permission_toggles_allowed(user_group), ), ) for user_group in user_groups @@ -178,6 +183,7 @@ def get_user_group( user, Permission.FULL_ADMIN_PANEL_ACCESS ), is_default=user_group.is_default, + permission_toggles_allowed=group_permission_toggles_allowed(user_group), ), ) @@ -242,9 +248,7 @@ def set_user_group_permissions( group = fetch_user_group(db_session, user_group_id) if group is None: raise OnyxError(OnyxErrorCode.NOT_FOUND, "User group not found") - assert_group_config_is_editable( - db_session, user_group_id, "change the permissions of" - ) + assert_group_permissions_editable(group, request.permissions) non_toggleable = [p for p in request.permissions if p in NON_TOGGLEABLE_PERMISSIONS] if non_toggleable: diff --git a/backend/onyx/auth/permission_projection.py b/backend/onyx/auth/permission_projection.py index 3e904bb321..052d677629 100644 --- a/backend/onyx/auth/permission_projection.py +++ b/backend/onyx/auth/permission_projection.py @@ -209,23 +209,25 @@ def user_group_permissions( is_user_groups_admin: bool, is_full_admin: bool, is_default: bool, + permission_toggles_allowed: bool, ) -> dict[str, bool]: """User group affordance map. ``manage`` (rename, assign agents/document sets, set manager) and ``edit_token_limits`` are the per-group ``manages_group`` decision — group in the manager's managed set, or global MANAGE_USER_GROUPS. A scoped manager gets full token-limit CRUD for groups they manage: every token route (read/create/update/delete) now admits scope. ``delete`` needs global MANAGE_USER_GROUPS (its route has no - ``allow_scope``). ``edit_permissions`` is FULL_ADMIN (the permission-toggle route). + ``allow_scope``). ``edit_permissions`` is FULL_ADMIN plus ``permission_toggles_allowed`` + (the permission-toggle route). - A seeded default group (Admin/Basic) has members and nothing else, so it keeps only - ``manage_members``, and only for a full admin — mirroring the routes, where every other - write raises CONFLICT.""" + A seeded default group (Admin/Basic) holds members and nothing else, so a full admin + gets ``manage_members`` and, on Basic only, ``edit_permissions`` for its allowlisted + toggles. Every other write on a default group raises CONFLICT at the route.""" is_editable = can_manage and not is_default result: UserGroupPermissions = { "manage": is_editable, "manage_members": is_full_admin if is_default else can_manage, "delete": is_user_groups_admin and not is_default, - "edit_permissions": is_full_admin and not is_default, + "edit_permissions": is_full_admin and permission_toggles_allowed, "edit_token_limits": is_editable, } return cast(dict[str, bool], result) diff --git a/backend/onyx/auth/permissions.py b/backend/onyx/auth/permissions.py index a0a76c4e7a..e40864eeef 100644 --- a/backend/onyx/auth/permissions.py +++ b/backend/onyx/auth/permissions.py @@ -10,7 +10,7 @@ from collections.abc import Callable, Coroutine from typing import Any from fastapi import Depends, Request -from pydantic import BaseModel, field_validator +from pydantic import BaseModel, computed_field, field_validator from onyx.db.enums import AccountType, Permission, PermissionAuthority from onyx.db.models import User @@ -98,6 +98,12 @@ NON_TOGGLEABLE_PERMISSIONS: frozenset[Permission] = frozenset( | Permission.IMPLIED ) +# The only grants an admin may switch on or off for the seeded Basic group, which +# holds the whole org. Everything else on a default group stays fixed by migrations. +DEFAULT_GROUP_TOGGLEABLE_PERMISSIONS: frozenset[Permission] = frozenset( + {Permission.CREATE_USER_API_KEYS} +) + # Permissions auto-granted to all users in Community Edition. # In CE there is no group-permission UI, so these capabilities must be # available without explicit grants. In EE they are controlled normally @@ -138,6 +144,12 @@ class PermissionRegistryEntry(BaseModel): permissions: list[Permission] group: int + @computed_field + @property + def default_group_toggleable(self) -> bool: + """Whether the row is offered on the Basic group's page.""" + return all(p in DEFAULT_GROUP_TOGGLEABLE_PERMISSIONS for p in self.permissions) + @field_validator("permissions") @classmethod def must_be_toggleable(cls, v: list[Permission]) -> list[Permission]: diff --git a/backend/onyx/db/user_group.py b/backend/onyx/db/user_group.py index 8d4c1a0af7..858d4c255c 100644 --- a/backend/onyx/db/user_group.py +++ b/backend/onyx/db/user_group.py @@ -1,7 +1,9 @@ """Invariants for the seeded default groups (Admin/Basic). A default group holds members and nothing else: no connectors, document sets, agents, -skills, providers or limits, and its name and permission grant are fixed at seed time. +skills, providers or limits, and its name is fixed. Its permission grants come from +migrations, except the few an admin may switch on the Basic group +(``DEFAULT_GROUP_TOGGLEABLE_PERMISSIONS``). Lives in Community Edition even though group management is Enterprise: the groups are seeded by a core migration and the resource-sharing paths that must refuse them are core @@ -13,15 +15,23 @@ from collections.abc import Collection from sqlalchemy import select from sqlalchemy.orm import Session +from onyx.auth.permissions import DEFAULT_GROUP_TOGGLEABLE_PERMISSIONS +from onyx.db.enums import Permission from onyx.db.models import UserGroup +from onyx.db.users import DEFAULT_BASIC_GROUP_NAME from onyx.error_handling.error_codes import OnyxErrorCode from onyx.error_handling.exceptions import OnyxError +def group_permission_toggles_allowed(user_group: UserGroup) -> bool: + """Custom groups take any toggleable grant, Basic its allowlist, Admin nothing.""" + return not user_group.is_default or user_group.name == DEFAULT_BASIC_GROUP_NAME + + def assert_group_config_is_editable( db_session: Session, user_group_id: int, action: str ) -> None: - """Membership is the only thing a default group has, so every other write is refused. + """A default group holds only members, so the config writes this guards are refused. ``action`` completes "Cannot {action} a default system group.". A missing group falls through to the caller's own NOT_FOUND.""" assert_groups_config_are_editable(db_session, [user_group_id], action) @@ -70,3 +80,25 @@ def assert_not_shared_with_default_group( + ", ".join(sorted(default_names)) + ". Make the resource public instead.", ) + + +def assert_group_permissions_editable( + user_group: UserGroup, permissions: Collection[Permission] +) -> None: + """A custom group takes any toggleable grant. Basic takes only the allowlisted + ones, since it holds the whole org, and Admin takes none.""" + if not user_group.is_default: + return + if not group_permission_toggles_allowed(user_group): + raise OnyxError( + OnyxErrorCode.CONFLICT, + "Cannot change the permissions of a default system group.", + ) + refused: list[str] = sorted( + p.value for p in permissions if p not in DEFAULT_GROUP_TOGGLEABLE_PERMISSIONS + ) + if refused: + raise OnyxError( + OnyxErrorCode.CONFLICT, + f"Permissions {refused} cannot be toggled on the Basic group.", + ) diff --git a/backend/tests/external_dependency_unit/auth/test_permission_projection_contract.py b/backend/tests/external_dependency_unit/auth/test_permission_projection_contract.py index 6a7d1be424..0395a45193 100644 --- a/backend/tests/external_dependency_unit/auth/test_permission_projection_contract.py +++ b/backend/tests/external_dependency_unit/auth/test_permission_projection_contract.py @@ -1270,6 +1270,7 @@ def test_user_group_projection_matches_gates(db_session: Session) -> None: actor, Permission.FULL_ADMIN_PANEL_ACCESS ), is_default=False, + permission_toggles_allowed=True, ) assert tags["manage"] == manage_enforced, actor.email assert tags["manage_members"] == manage_enforced, actor.email @@ -1295,6 +1296,7 @@ def test_user_group_projection_matches_gates(db_session: Session) -> None: in_scope, Permission.FULL_ADMIN_PANEL_ACCESS ), is_default=False, + permission_toggles_allowed=True, ) == { "manage": True, "manage_members": True, @@ -1315,6 +1317,7 @@ def test_user_group_projection_matches_gates(db_session: Session) -> None: groups_admin, Permission.FULL_ADMIN_PANEL_ACCESS ), is_default=False, + permission_toggles_allowed=True, ) == { "manage": True, "manage_members": True, @@ -1346,6 +1349,7 @@ def test_default_user_group_projection_is_membership_only(db_session: Session) - actor, Permission.FULL_ADMIN_PANEL_ACCESS ), is_default=True, + permission_toggles_allowed=False, ) assert _tags(admin) == { @@ -1364,6 +1368,22 @@ def test_default_user_group_projection_is_membership_only(db_session: Session) - "edit_token_limits": False, } + # Basic is the one default group that offers permission toggles, full admin only. + assert user_group_permissions( + can_manage=False, + is_user_groups_admin=True, + is_full_admin=True, + is_default=True, + permission_toggles_allowed=True, + )["edit_permissions"] + assert not user_group_permissions( + can_manage=False, + is_user_groups_admin=True, + is_full_admin=False, + is_default=True, + permission_toggles_allowed=True, + )["edit_permissions"] + def test_user_group_key_coverage() -> None: assert set( @@ -1372,6 +1392,7 @@ def test_user_group_key_coverage() -> None: is_user_groups_admin=True, is_full_admin=True, is_default=False, + permission_toggles_allowed=True, ) ) == set(USER_GROUP_ACTIONS) diff --git a/backend/tests/external_dependency_unit/db/test_grant_create_pat_to_basic_group_migration.py b/backend/tests/external_dependency_unit/db/test_grant_create_pat_to_basic_group_migration.py new file mode 100644 index 0000000000..ae80650999 --- /dev/null +++ b/backend/tests/external_dependency_unit/db/test_grant_create_pat_to_basic_group_migration.py @@ -0,0 +1,138 @@ +"""Migration b6d920407aa6 grants create:user_api_keys to the Basic group and +backfills its members' stored permissions. The test database already sits at +head, so the test runs downgrade then upgrade inside its transaction and +rolls back.""" + +import importlib.util +from importlib.machinery import ModuleSpec +from pathlib import Path +from types import ModuleType +from uuid import uuid4 + +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import select +from sqlalchemy.orm import Session + +from onyx.db.enums import GrantSource, Permission +from onyx.db.models import PermissionGrant, User, User__UserGroup, UserGroup +from onyx.db.users import DEFAULT_BASIC_GROUP_NAME + +_MIGRATION_PATH = ( + Path(__file__).parents[3] + / "alembic/versions/b6d920407aa6_grant_create_pat_to_basic_group.py" +) +PAT_PERMISSION = Permission.CREATE_USER_API_KEYS.value +BASIC_PERMISSION = Permission.BASIC_ACCESS.value + + +def _load_migration() -> ModuleType: + spec: ModuleSpec | None = importlib.util.spec_from_file_location( + "pat_grant_migration", _MIGRATION_PATH + ) + assert spec is not None and spec.loader is not None + migration: ModuleType = importlib.util.module_from_spec(spec) + spec.loader.exec_module(migration) + return migration + + +def _basic_grant(db_session: Session, basic: UserGroup) -> PermissionGrant | None: + return db_session.scalar( + select(PermissionGrant).where( + PermissionGrant.group_id == basic.id, + PermissionGrant.permission == Permission.CREATE_USER_API_KEYS, + ) + ) + + +def _stored_permissions(db_session: Session, user: User) -> set[str]: + db_session.expire(user) + return set(user.effective_permissions) + + +def _make_user(db_session: Session, permissions: list[str]) -> User: + user: User = User( + id=uuid4(), + email=f"pat-migration-{uuid4().hex[:8]}@example.com", + hashed_password="unused", + effective_permissions=permissions, + ) + db_session.add(user) + return user + + +def test_basic_members_lose_and_regain_the_grant( + db_session: Session, + tenant_context: None, # noqa: ARG001 +) -> None: + try: + basic: UserGroup | None = db_session.scalar( + select(UserGroup).where( + UserGroup.name == DEFAULT_BASIC_GROUP_NAME, + UserGroup.is_default.is_(True), + ) + ) + assert basic is not None + + other_group: UserGroup = UserGroup(name=f"pat-migration-{uuid4().hex[:8]}") + db_session.add(other_group) + db_session.flush() + db_session.add( + PermissionGrant( + group_id=other_group.id, + permission=Permission.CREATE_USER_API_KEYS, + grant_source=GrantSource.USER, + ) + ) + + member: User = _make_user(db_session, [BASIC_PERMISSION, PAT_PERMISSION]) + dual: User = _make_user(db_session, [BASIC_PERMISSION, PAT_PERMISSION]) + outsider: User = _make_user(db_session, []) + db_session.flush() + db_session.add_all( + [ + User__UserGroup(user_group_id=basic.id, user_id=member.id), + User__UserGroup(user_group_id=basic.id, user_id=dual.id), + User__UserGroup(user_group_id=other_group.id, user_id=dual.id), + ] + ) + db_session.flush() + + migration: ModuleType = _load_migration() + with Operations.context(MigrationContext.configure(db_session.connection())): + migration.downgrade() + assert _basic_grant(db_session, basic) is None + assert _stored_permissions(db_session, member) == {BASIC_PERMISSION} + # Held through the other group, so the downgrade leaves it alone. + assert _stored_permissions(db_session, dual) == { + BASIC_PERMISSION, + PAT_PERMISSION, + } + assert _stored_permissions(db_session, outsider) == set() + + migration.upgrade() + grant: PermissionGrant | None = _basic_grant(db_session, basic) + assert grant is not None + assert grant.grant_source == GrantSource.SYSTEM + assert not grant.is_deleted + assert _stored_permissions(db_session, member) == { + BASIC_PERMISSION, + PAT_PERMISSION, + } + db_session.expire(dual) + assert sorted(dual.effective_permissions) == [ + BASIC_PERMISSION, + PAT_PERMISSION, + ] + assert _stored_permissions(db_session, outsider) == set() + + # A soft-deleted Basic grant is revived rather than duplicated. + grant.is_deleted = True + member.effective_permissions = [BASIC_PERMISSION] + db_session.flush() + migration.upgrade() + db_session.expire(grant) + assert not grant.is_deleted + assert PAT_PERMISSION in _stored_permissions(db_session, member) + finally: + db_session.rollback() diff --git a/backend/tests/integration/common_utils/managers/user_group.py b/backend/tests/integration/common_utils/managers/user_group.py index a0a703e1aa..6617476764 100644 --- a/backend/tests/integration/common_utils/managers/user_group.py +++ b/backend/tests/integration/common_utils/managers/user_group.py @@ -110,9 +110,22 @@ class UserGroupManager: user_group: DATestUserGroup, user_performing_action: DATestUser, include_non_toggleable: bool = False, + ) -> list[str]: + return UserGroupManager.get_permissions_by_id( + user_group.id, user_performing_action, include_non_toggleable + ) + + # *_by_id variants: SCIM tests hold only the group id the SCIM API returns, and + # default-group tests hold the API model from get_default. + + @staticmethod + def get_permissions_by_id( + user_group_id: int | str, + user_performing_action: DATestUser, + include_non_toggleable: bool = False, ) -> list[str]: response = client.get( - f"{API_SERVER_URL}/manage/admin/user-group/{user_group.id}/permissions", + f"{API_SERVER_URL}/manage/admin/user-group/{user_group_id}/permissions", params=( {"include_non_toggleable": "true"} if include_non_toggleable else None ), @@ -121,8 +134,6 @@ class UserGroupManager: response.raise_for_status() return response.json() - # *_by_id variants: SCIM tests hold only the group id the SCIM API returns. - @staticmethod def set_permissions_by_id( user_group_id: int | str, diff --git a/backend/tests/integration/tests/pat/test_pat_api.py b/backend/tests/integration/tests/pat/test_pat_api.py index f34478c283..a4a9f33ae5 100644 --- a/backend/tests/integration/tests/pat/test_pat_api.py +++ b/backend/tests/integration/tests/pat/test_pat_api.py @@ -1,14 +1,5 @@ -""" -Integration tests for Personal Access Token (PAT) API. - -Test Suite: -1. test_pat_lifecycle_happy_path - Complete PAT lifecycle (create, auth, revoke) -2. test_pat_user_isolation_and_authentication - User authentication and multi-user isolation -3. test_pat_expiration_flow - Expiration logic (end-of-day UTC, never-expiring) -4. test_pat_validation_errors - Input validation and error handling -5. test_pat_sorting_and_last_used - Sorting and last_used_at tracking -6. test_pat_role_based_access_control - Admin vs Basic permissions -""" +"""Integration tests for the personal access token API: lifecycle, isolation, +expiration, validation, sorting, and permission inheritance.""" import os import time @@ -25,30 +16,6 @@ from tests.integration.common_utils.managers.user_group import UserGroupManager from tests.integration.common_utils.test_models import DATestUser -def _grant_create_pat_permission( - user: DATestUser, - admin_user: DATestUser, -) -> None: - """Grant CREATE_USER_API_KEYS to a non-admin user via a temp group. - - PAT creation is gated by Permission.CREATE_USER_API_KEYS; non-admin test - users must be placed in a group that grants it before they can call - POST /user/pats. - """ - group = UserGroupManager.create( - name=f"pat_creator_{user.id[:8]}", - user_ids=[user.id], - cc_pair_ids=[], - user_performing_action=admin_user, - ) - response = UserGroupManager.set_permissions( - user_group=group, - permissions=[Permission.CREATE_USER_API_KEYS.value], - user_performing_action=admin_user, - ) - response.raise_for_status() - - def test_pat_lifecycle_happy_path(reset: None) -> None: # noqa: ARG001 """Complete PAT lifecycle: create, authenticate, revoke.""" user: DATestUser = UserManager.create(name="pat_user") @@ -109,11 +76,6 @@ def test_pat_user_isolation_and_authentication( user_a: DATestUser = UserManager.create(name="user_a") user_b: DATestUser = UserManager.create(name="user_b") - # user_a is the first registered user and lands in Admin → has - # FULL_ADMIN_PANEL_ACCESS which short-circuits the PAT-creation gate. - # user_b is basic and needs CREATE_USER_API_KEYS via a group grant. - _grant_create_pat_permission(user_b, user_a) - # Create tokens for both users user_a_pats = [] for i in range(2): @@ -350,8 +312,6 @@ def test_pat_role_based_access_control(reset: None) -> None: # noqa: ARG001 basic_user: DATestUser = UserManager.create(name="basic_user") assert not basic_user.is_admin - _grant_create_pat_permission(basic_user, admin_user) - admin_pat = PATManager.create( name="Admin Token", expiration_days=7, @@ -448,7 +408,6 @@ def test_pat_group_permission_access_control(reset: None) -> None: # noqa: ARG0 permissions=[ Permission.MANAGE_CONNECTORS.value, Permission.MANAGE_USER_GROUPS.value, - Permission.CREATE_USER_API_KEYS.value, ], user_performing_action=admin_user, ) @@ -483,7 +442,6 @@ def test_pat_group_permission_access_control(reset: None) -> None: # noqa: ARG0 # Sanity: a plain basic user's PAT still cannot reach that endpoint plain_basic: DATestUser = UserManager.create(name="plain_basic") - _grant_create_pat_permission(plain_basic, admin_user) plain_pat = PATManager.create( name="Plain Basic Token", expiration_days=7, diff --git a/backend/tests/integration/tests/permissions/conftest.py b/backend/tests/integration/tests/permissions/conftest.py index cbeba1e777..b42d4f3d8a 100644 --- a/backend/tests/integration/tests/permissions/conftest.py +++ b/backend/tests/integration/tests/permissions/conftest.py @@ -83,7 +83,7 @@ def permission_admin_user(module_reset: None) -> DATestUser: # noqa: ARG001 def permission_basic_user( permission_admin_user: DATestUser, # noqa: ARG001 ) -> DATestUser: - """Second registered user — joins the default Basic user group with only basic permissions.""" + """Second registered user. Joins the default Basic group and holds only its grants.""" return UserManager.create(name="perm_basic") diff --git a/backend/tests/integration/tests/permissions/test_pat_scope_assignment.py b/backend/tests/integration/tests/permissions/test_pat_scope_assignment.py index 55307ca035..4410380589 100644 --- a/backend/tests/integration/tests/permissions/test_pat_scope_assignment.py +++ b/backend/tests/integration/tests/permissions/test_pat_scope_assignment.py @@ -1,24 +1,13 @@ """Integration tests for assigning scopes when minting a PAT via the user API. -Minting needs CREATE_USER_API_KEYS, so those tests use ``pat_creator``. Listing -scopes is BASIC_ACCESS and stays on the basic user, proving it needs no creation -rights. +Minting needs CREATE_USER_API_KEYS, which the seeded Basic group grants, so every +test runs as the basic user. """ -from typing import Any - -import pytest - from onyx.db.enums import Permission from tests.integration.common_utils.managers.pat import PATManager from tests.integration.common_utils.test_models import DATestUser - -@pytest.fixture(scope="module") -def pat_creator(permission_holder_user_factory: Any) -> DATestUser: - return permission_holder_user_factory(Permission.CREATE_USER_API_KEYS.value) - - # The scopes a user may assign today (admin scopes are intentionally excluded). EXPECTED_SELECTABLE_SCOPES = { Permission.READ_SEARCH.value, @@ -48,30 +37,36 @@ def test_scope_implications(permission_basic_user: DATestUser) -> None: assert by_scope[Permission.USE_LLM_GATEWAY.value]["min_tier"] == "business" -def test_scopes_round_trip(pat_creator: DATestUser) -> None: +def test_scopes_round_trip(permission_basic_user: DATestUser) -> None: scopes = [Permission.READ_SEARCH, Permission.READ_CHAT] expected = {s.value for s in scopes} - created = PATManager.create("scoped-token", None, pat_creator, scopes=scopes) + created = PATManager.create( + "scoped-token", None, permission_basic_user, scopes=scopes + ) assert created.scopes is not None and set(created.scopes) == expected - listed = next(pat for pat in PATManager.list(pat_creator) if pat.id == created.id) + listed = next( + pat for pat in PATManager.list(permission_basic_user) if pat.id == created.id + ) assert listed.scopes is not None and set(listed.scopes) == expected -def test_unscoped_token_is_unrestricted(pat_creator: DATestUser) -> None: - created = PATManager.create("full-token", None, pat_creator) +def test_unscoped_token_is_unrestricted(permission_basic_user: DATestUser) -> None: + created = PATManager.create("full-token", None, permission_basic_user) assert created.scopes is None -def test_empty_scopes_rejected(pat_creator: DATestUser) -> None: - response = PATManager.create_response("empty-token", None, pat_creator, scopes=[]) - assert response.status_code == 400 - assert response.json()["error_code"] == "INVALID_INPUT" - - -def test_non_selectable_scope_rejected(pat_creator: DATestUser) -> None: +def test_empty_scopes_rejected(permission_basic_user: DATestUser) -> None: response = PATManager.create_response( - "admin-token", None, pat_creator, scopes=[Permission.READ_ADMIN] + "empty-token", None, permission_basic_user, scopes=[] + ) + assert response.status_code == 400 + assert response.json()["error_code"] == "INVALID_INPUT" + + +def test_non_selectable_scope_rejected(permission_basic_user: DATestUser) -> None: + response = PATManager.create_response( + "admin-token", None, permission_basic_user, scopes=[Permission.READ_ADMIN] ) assert response.status_code == 400 assert response.json()["error_code"] == "INVALID_INPUT" diff --git a/backend/tests/integration/tests/permissions_access/test_create_user_api_keys.py b/backend/tests/integration/tests/permissions_access/test_create_user_api_keys.py index a9ecbdffa2..8c53d60f8c 100644 --- a/backend/tests/integration/tests/permissions_access/test_create_user_api_keys.py +++ b/backend/tests/integration/tests/permissions_access/test_create_user_api_keys.py @@ -4,6 +4,9 @@ Covers the personal-access-token creation endpoint in ``backend/onyx/server/pat/api.py`` (router prefix ``/user/pats``). The basic-access GET endpoints on the same router are covered by ``test_basic_access.py`` — this file only exercises the CREATE token. + +The seeded Basic group grants this permission, so every standard user is a +holder. The matrix below replaces the shared USER_KINDS to expect that. """ import os @@ -14,7 +17,6 @@ import pytest from onyx.db.enums import Permission from tests.integration.common_utils.test_models import DATestAPIKey, DATestUser from tests.integration.tests.permissions._access_matrix import ( - USER_KINDS, Endpoint, assert_response, call_endpoint, @@ -28,6 +30,17 @@ pytestmark = pytest.mark.skipif( PERMISSION = Permission.CREATE_USER_API_KEYS.value +USER_KINDS: list[tuple[str, str]] = [ + ("admin", "allowed"), + ("holder", "allowed"), + ("service_account_holder", "allowed"), + ("basic", "allowed"), + ("service_account", "denied"), + ("bot", "denied"), + ("ext_perm", "denied"), + ("anonymous", "anon_denied"), +] + _CREATE_PAT_BODY: dict[str, Any] = { "name": "perm-test-pat", "expiration_days": 7, diff --git a/backend/tests/integration/tests/permissions_membership/test_manage_user_groups.py b/backend/tests/integration/tests/permissions_membership/test_manage_user_groups.py index 5c92e81566..63249a3b63 100644 --- a/backend/tests/integration/tests/permissions_membership/test_manage_user_groups.py +++ b/backend/tests/integration/tests/permissions_membership/test_manage_user_groups.py @@ -328,9 +328,10 @@ def test_holder_cannot_touch_the_basic_group( permission_admin_user: DATestUser, holder_user: DATestUser, ) -> None: - """Basic grants only BASIC_ACCESS, so the amplification check above can't be what - stops the holder — the default-group gate is. The affordance map agrees, which is - what keeps the groups page from listing Admin/Basic for them.""" + """The holder is a Basic member and already holds every Basic grant, so the + amplification check above can't be what stops them. The default-group gate is. The + affordance map agrees, which is what keeps the groups page from listing Admin/Basic + for them.""" basic = UserGroupManager.get_default( user_performing_action=permission_admin_user, name="Basic" ) diff --git a/backend/tests/integration/tests/usergroup/test_basic_group_pat_toggle.py b/backend/tests/integration/tests/usergroup/test_basic_group_pat_toggle.py new file mode 100644 index 0000000000..910dbcf840 --- /dev/null +++ b/backend/tests/integration/tests/usergroup/test_basic_group_pat_toggle.py @@ -0,0 +1,68 @@ +"""The seeded Basic group grants create:user_api_keys by default. An admin can switch +it off and back on, and nothing else can be toggled there.""" + +import os + +import httpx +import pytest + +from ee.onyx.server.user_group.models import UserGroup +from onyx.db.enums import Permission +from tests.integration.common_utils.managers.pat import PATManager +from tests.integration.common_utils.managers.user import UserManager +from tests.integration.common_utils.managers.user_group import UserGroupManager +from tests.integration.common_utils.test_models import DATestPAT, DATestUser + +PAT_PERMISSION = Permission.CREATE_USER_API_KEYS.value + + +@pytest.mark.skipif( + os.environ.get("RUN_EE_TESTS", "").lower() != "true", + reason="User group tests are enterprise only", +) +def test_basic_group_pat_toggle(new_admin_user: DATestUser) -> None: + basic: UserGroup = UserGroupManager.get_default( + user_performing_action=new_admin_user, name="Basic" + ) + member: DATestUser = UserManager.create(name="basic_pat_toggle_member") + + # On by default, so a fresh basic user mints a token. + assert PAT_PERMISSION in UserGroupManager.get_permissions_by_id( + basic.id, new_admin_user + ) + minted: DATestPAT = PATManager.create( + name="before-toggle", expiration_days=7, user_performing_action=member + ) + assert minted.token is not None + + off: httpx.Response = UserGroupManager.set_permissions_by_id( + basic.id, [], new_admin_user + ) + off.raise_for_status() + assert PAT_PERMISSION not in UserGroupManager.get_permissions_by_id( + basic.id, new_admin_user + ) + denied: httpx.Response = PATManager.create_response("after-toggle", 7, member) + assert denied.status_code == 403 + + on: httpx.Response = UserGroupManager.set_permissions_by_id( + basic.id, [PAT_PERMISSION], new_admin_user + ) + on.raise_for_status() + restored: DATestPAT = PATManager.create( + name="after-restore", expiration_days=7, user_performing_action=member + ) + assert restored.token is not None + + # Only the allowlisted toggles exist on Basic, and Admin takes none. + refused: httpx.Response = UserGroupManager.set_permissions_by_id( + basic.id, [PAT_PERMISSION, Permission.MANAGE_LLMS.value], new_admin_user + ) + assert refused.status_code == 409 + admin_group: UserGroup = UserGroupManager.get_default( + user_performing_action=new_admin_user, name="Admin" + ) + refused_on_admin: httpx.Response = UserGroupManager.set_permissions_by_id( + admin_group.id, [PAT_PERMISSION], new_admin_user + ) + assert refused_on_admin.status_code == 409 diff --git a/backend/tests/integration/tests/usergroup/test_default_group_read_only.py b/backend/tests/integration/tests/usergroup/test_default_group_read_only.py index 31d10ea698..a51ef4a45e 100644 --- a/backend/tests/integration/tests/usergroup/test_default_group_read_only.py +++ b/backend/tests/integration/tests/usergroup/test_default_group_read_only.py @@ -1,5 +1,6 @@ -"""The seeded default groups (Admin/Basic) are visible, hold members and nothing else, -and only a full admin may change that membership. +"""The seeded default groups (Admin/Basic) are visible and hold members and nothing +else. Only a full admin may change that membership or switch Basic's allowlisted +permission toggles. The list endpoint still returns them to any READ_USER_GROUPS holder that asks — the service-account picker relies on that — so "full admins only" rides on the ``permissions`` @@ -44,8 +45,9 @@ def test_default_groups_are_listed_only_when_requested(admin_user: DATestUser) - @ENTERPRISE_SKIP def test_default_group_affordances_are_membership_only(admin_user: DATestUser) -> None: """Whole-map equality: a new action defaulting to True must fail here rather than - quietly render a control the routes reject.""" - for name in ("Admin", "Basic"): + quietly render a control the routes reject. Basic alone offers its allowlisted + permission toggles.""" + for name, edit_permissions in (("Admin", False), ("Basic", True)): group = UserGroupManager.get_default( user_performing_action=admin_user, name=name ) @@ -53,7 +55,7 @@ def test_default_group_affordances_are_membership_only(admin_user: DATestUser) - "manage": False, "manage_members": True, "delete": False, - "edit_permissions": False, + "edit_permissions": edit_permissions, "edit_token_limits": False, }, name @@ -73,6 +75,7 @@ def test_default_group_rejects_every_non_membership_write( json={"id": group.id, "name": "Renamed"}, headers=headers, ), + # outside the Basic allowlist, so still refused "permissions": client.put( f"{GROUP_URL}/{group.id}/permissions", json={"permissions": [Permission.MANAGE_LLMS.value]}, diff --git a/web/src/i18n/messages/ar.json b/web/src/i18n/messages/ar.json index 8f961aa08f..9f84619f3c 100644 --- a/web/src/i18n/messages/ar.json +++ b/web/src/i18n/messages/ar.json @@ -4426,6 +4426,7 @@ "tooltip": "يجري تحديث تضمينات المستندات بسبب تغييرات حديثة على هذه المجموعة." }, "systemGroup": { + "basicDescription": "يدير {appName} هذه المجموعة. الأعضاء هم الشيء الوحيد الذي تحتويه، والأذونات أدناه تنطبق على الجميع فيها.", "description": "يدير {appName} هذه المجموعة. الأعضاء هم الشيء الوحيد الذي تحتويه، والشيء الوحيد الذي يمكنك تغييره.", "title": "مجموعة نظام" }, diff --git a/web/src/i18n/messages/de.json b/web/src/i18n/messages/de.json index 5dd6f25a82..0fa31d6ca7 100644 --- a/web/src/i18n/messages/de.json +++ b/web/src/i18n/messages/de.json @@ -4426,6 +4426,7 @@ "tooltip": "Die Dokument-Embeddings werden aufgrund kürzlicher Änderungen an dieser Gruppe aktualisiert." }, "systemGroup": { + "basicDescription": "{appName} verwaltet diese Gruppe. Sie enthält nur Mitglieder, und die Berechtigungen unten gelten für alle darin.", "description": "{appName} verwaltet diese Gruppe. Sie enthält nur Mitglieder, und nur diese können Sie ändern.", "title": "Systemgruppe" }, diff --git a/web/src/i18n/messages/en.json b/web/src/i18n/messages/en.json index ab735b519b..344e0791a3 100644 --- a/web/src/i18n/messages/en.json +++ b/web/src/i18n/messages/en.json @@ -4426,6 +4426,7 @@ "tooltip": "Document embeddings are being updated due to recent changes to this group." }, "systemGroup": { + "basicDescription": "{appName} manages this group. Members are the only thing it holds, and the permissions below apply to everyone in it.", "description": "{appName} manages this group. Members are the only thing it holds, and the only thing you can change.", "title": "System group" }, diff --git a/web/src/i18n/messages/es.json b/web/src/i18n/messages/es.json index 06ec6a3b16..3fabefa473 100644 --- a/web/src/i18n/messages/es.json +++ b/web/src/i18n/messages/es.json @@ -4426,6 +4426,7 @@ "tooltip": "Los embeddings de los documentos se están actualizando por los cambios recientes en este grupo." }, "systemGroup": { + "basicDescription": "{appName} gestiona este grupo. Solo contiene miembros, y los permisos de abajo se aplican a todos ellos.", "description": "{appName} gestiona este grupo. Solo contiene miembros y es lo único que puedes cambiar.", "title": "Grupo del sistema" }, diff --git a/web/src/i18n/messages/fr.json b/web/src/i18n/messages/fr.json index aebf5164d4..94a5e86c47 100644 --- a/web/src/i18n/messages/fr.json +++ b/web/src/i18n/messages/fr.json @@ -4426,6 +4426,7 @@ "tooltip": "Les embeddings des documents sont en cours de mise à jour à la suite de modifications récentes de ce groupe." }, "systemGroup": { + "basicDescription": "{appName} gère ce groupe. Il ne contient que des membres, et les permissions ci-dessous s'appliquent à tous.", "description": "{appName} gère ce groupe. Il ne contient que des membres, et c'est la seule chose que vous pouvez modifier.", "title": "Groupe système" }, diff --git a/web/src/i18n/messages/ja.json b/web/src/i18n/messages/ja.json index 29f9f9c0cb..a0f1f0e4bd 100644 --- a/web/src/i18n/messages/ja.json +++ b/web/src/i18n/messages/ja.json @@ -4426,6 +4426,7 @@ "tooltip": "このグループの最近の変更により、ドキュメントの embedding を更新しています。" }, "systemGroup": { + "basicDescription": "このグループは {appName} が管理します。保持するのはメンバーのみで、以下の権限は全員に適用されます。", "description": "このグループは {appName} が管理します。保持するのはメンバーのみで、変更できるのもメンバーだけです。", "title": "システムグループ" }, diff --git a/web/src/i18n/messages/ko.json b/web/src/i18n/messages/ko.json index 1425138826..bb670bd336 100644 --- a/web/src/i18n/messages/ko.json +++ b/web/src/i18n/messages/ko.json @@ -4426,6 +4426,7 @@ "tooltip": "이 그룹의 최근 변경 사항으로 인해 문서 embedding을 업데이트하는 중입니다." }, "systemGroup": { + "basicDescription": "{appName}가 이 그룹을 관리합니다. 이 그룹에는 멤버만 포함되며, 아래 권한은 모든 멤버에게 적용됩니다.", "description": "{appName}가 이 그룹을 관리합니다. 이 그룹에는 멤버만 포함되며, 변경할 수 있는 항목도 멤버뿐입니다.", "title": "시스템 그룹" }, diff --git a/web/src/i18n/messages/pt.json b/web/src/i18n/messages/pt.json index 644de8777d..cd14cb3399 100644 --- a/web/src/i18n/messages/pt.json +++ b/web/src/i18n/messages/pt.json @@ -4426,6 +4426,7 @@ "tooltip": "Os embeddings dos documentos estão sendo atualizados por causa de alterações recentes neste grupo." }, "systemGroup": { + "basicDescription": "O {appName} gerencia este grupo. Ele contém apenas membros, e as permissões abaixo se aplicam a todos eles.", "description": "O {appName} gerencia este grupo. Ele contém apenas membros, e isso é a única coisa que você pode alterar.", "title": "Grupo do sistema" }, diff --git a/web/src/i18n/messages/zh.json b/web/src/i18n/messages/zh.json index 21f84750ba..2d755f1ad9 100644 --- a/web/src/i18n/messages/zh.json +++ b/web/src/i18n/messages/zh.json @@ -4426,6 +4426,7 @@ "tooltip": "由于此用户组最近发生更改,正在更新文档 embedding。" }, "systemGroup": { + "basicDescription": "此用户组由 {appName} 管理。它只包含成员,下方的权限适用于其中的所有人。", "description": "此用户组由 {appName} 管理。它只包含成员,您也只能更改成员。", "title": "系统用户组" }, diff --git a/web/src/views/admin/GroupsPage/EditGroupPage.tsx b/web/src/views/admin/GroupsPage/EditGroupPage.tsx index 672a060103..9d87fd46b0 100644 --- a/web/src/views/admin/GroupsPage/EditGroupPage.tsx +++ b/web/src/views/admin/GroupsPage/EditGroupPage.tsx @@ -38,7 +38,7 @@ import { useUser } from "@/providers/UserProvider"; import { useSettings } from "@/lib/settings/hooks"; import { Tier } from "@/lib/settings/types"; import { tierAtLeast } from "@/lib/tiers"; -import type { MemberRow, TokenRateLimitDisplay } from "./interfaces"; +import type { MemberRow, TokenRateLimitDisplay } from "./types"; import { makeBaseColumns, makeMemberTableColumns, @@ -629,9 +629,12 @@ function EditGroupPage({ groupId }: EditGroupPageProps) { )} @@ -744,6 +747,7 @@ function EditGroupPage({ groupId }: EditGroupPageProps) { )} diff --git a/web/src/views/admin/GroupsPage/GroupPermissionsSection.tsx b/web/src/views/admin/GroupsPage/GroupPermissionsSection.tsx index 3d7c5a94eb..71d63bfba2 100644 --- a/web/src/views/admin/GroupsPage/GroupPermissionsSection.tsx +++ b/web/src/views/admin/GroupsPage/GroupPermissionsSection.tsx @@ -29,7 +29,7 @@ import Card from "@/refresh-components/cards/Card"; import { Collapsible } from "@opal/components"; import { errorHandlingFetcher } from "@/lib/fetcher"; import { SWR_KEYS } from "@/lib/swr-keys"; -import type { PermissionRegistryEntry } from "@/views/admin/GroupsPage/interfaces"; +import type { PermissionRegistryEntry } from "@/views/admin/GroupsPage/types"; // Icon mapping — the only permission metadata kept in the frontend. // `id` keys must match the backend PERMISSION_REGISTRY entries. @@ -50,17 +50,23 @@ const ICON_MAP: Record = { interface GroupPermissionsSectionProps { enabledPermissions: Set; onPermissionsChange: (permissions: Set) => void; + // A seeded default group only offers the rows the backend marks for it. + isDefaultGroup?: boolean; } function GroupPermissionsSection({ enabledPermissions, onPermissionsChange, + isDefaultGroup = false, }: GroupPermissionsSectionProps) { const t = useTranslations("admin.groups"); - const { data: registry, isLoading } = useSWR( + const { data: fullRegistry, isLoading } = useSWR( SWR_KEYS.permissionRegistry, errorHandlingFetcher ); + const registry = isDefaultGroup + ? fullRegistry?.filter((entry) => entry.default_group_toggleable) + : fullRegistry; function isRowEnabled(entry: PermissionRegistryEntry): boolean { return entry.permissions.every((p) => enabledPermissions.has(p)); diff --git a/web/src/views/admin/GroupsPage/shared.tsx b/web/src/views/admin/GroupsPage/shared.tsx index 75924e073e..f86bf3dcfe 100644 --- a/web/src/views/admin/GroupsPage/shared.tsx +++ b/web/src/views/admin/GroupsPage/shared.tsx @@ -6,7 +6,7 @@ import { SvgSlack } from "@opal/logos"; import type { IconFunctionComponent } from "@opal/types"; import Text from "@/refresh-components/texts/Text"; import { AccountType, UserStatus } from "@/lib/types"; -import type { ApiKeyDescriptor, MemberRow } from "./interfaces"; +import type { ApiKeyDescriptor, MemberRow } from "./types"; // --------------------------------------------------------------------------- // Constants diff --git a/web/src/views/admin/GroupsPage/interfaces.ts b/web/src/views/admin/GroupsPage/types.ts similarity index 95% rename from web/src/views/admin/GroupsPage/interfaces.ts rename to web/src/views/admin/GroupsPage/types.ts index c4bd6d07f4..144a81cb97 100644 --- a/web/src/views/admin/GroupsPage/interfaces.ts +++ b/web/src/views/admin/GroupsPage/types.ts @@ -29,4 +29,5 @@ export interface PermissionRegistryEntry { description: string; permissions: string[]; group: number; + default_group_toggleable: boolean; } diff --git a/web/src/views/admin/GroupsPage/useGroupMemberCandidates.ts b/web/src/views/admin/GroupsPage/useGroupMemberCandidates.ts index f83789d205..6e699eb9b9 100644 --- a/web/src/views/admin/GroupsPage/useGroupMemberCandidates.ts +++ b/web/src/views/admin/GroupsPage/useGroupMemberCandidates.ts @@ -7,7 +7,7 @@ import { SWR_KEYS } from "@/lib/swr-keys"; import { useUser } from "@/providers/UserProvider"; import { AccountType, UserStatus } from "@/lib/types"; import type { FullUserSnapshot } from "@/views/admin/UsersPage/types"; -import type { ApiKeyDescriptor, MemberRow } from "./interfaces"; +import type { ApiKeyDescriptor, MemberRow } from "./types"; interface ManageUsersResponse { accepted: FullUserSnapshot[]; diff --git a/web/tests/e2e/admin/groups/GroupsAdminPage.ts b/web/tests/e2e/admin/groups/GroupsAdminPage.ts index e5e19eafae..358d4ed806 100644 --- a/web/tests/e2e/admin/groups/GroupsAdminPage.ts +++ b/web/tests/e2e/admin/groups/GroupsAdminPage.ts @@ -165,11 +165,21 @@ export class GroupsAdminPage { return this.page.getByText("System group", { exact: true }); } - /** Header of the group-permissions section (full admin, custom groups only). */ + /** Header of the group-permissions section (full admin, custom groups and Basic). */ get permissionsSectionHeader(): Locator { return this.page.getByText("Group Permissions", { exact: true }); } + /** One permission row in that section, matched by its display name. */ + permissionRow(displayName: string): Locator { + return this.page.getByText(displayName, { exact: true }); + } + + /** Every permission switch on the edit page. */ + get permissionSwitches(): Locator { + return this.page.getByRole("switch"); + } + /** Header of the shared-resources section (custom groups only). */ get sharedResourcesSectionHeader(): Locator { return this.page.getByText("Shared with This Group", { exact: true }); diff --git a/web/tests/e2e/admin/groups/groups.spec.ts b/web/tests/e2e/admin/groups/groups.spec.ts index f29be37366..f466958957 100644 --- a/web/tests/e2e/admin/groups/groups.spec.ts +++ b/web/tests/e2e/admin/groups/groups.spec.ts @@ -103,7 +103,13 @@ test.describe("Groups page — layout", () => { await expect(groupsPage.saveButton).toBeEnabled(); await expect(groupsPage.deleteGroupButton).toHaveCount(0); - await expect(groupsPage.permissionsSectionHeader).toHaveCount(0); + // Basic offers its allowlisted toggle and nothing else. The switch state is + // left alone because the gating spec flips it from another worker. + await expect(groupsPage.permissionsSectionHeader).toBeVisible(); + await expect( + groupsPage.permissionRow("Create User Access Token") + ).toBeVisible(); + await expect(groupsPage.permissionSwitches).toHaveCount(1); await expect(groupsPage.sharedResourcesSectionHeader).toHaveCount(0); }); diff --git a/web/tests/e2e/admin/permissions/permission_gating.spec.ts b/web/tests/e2e/admin/permissions/permission_gating.spec.ts index 8e9641fafb..ed90405be3 100644 --- a/web/tests/e2e/admin/permissions/permission_gating.spec.ts +++ b/web/tests/e2e/admin/permissions/permission_gating.spec.ts @@ -879,8 +879,9 @@ test.describe("Permission gating — READ_QUERY_HISTORY", () => { }); test.describe("Permission gating — CREATE_USER_API_KEYS", () => { - test("Access Tokens section and New Access Token button are gated behind CREATE_USER_API_KEYS", async ({ + test("The Basic group grants Access Tokens by default and an admin can switch it off", async ({ page, + browser, adminClient, testUserContext, }) => { @@ -892,30 +893,16 @@ test.describe("Permission gating — CREATE_USER_API_KEYS", () => { "Permission registry unavailable (CE environment)" ); - const { groupId, email, password } = testUserContext; + const { email, password } = testUserContext; + const basicGroup = (await adminClient.getUserGroups()).find( + (g) => g.is_default && g.name === "Basic" + ); + expect(basicGroup).toBeDefined(); + const basicGroupId = basicGroup!.id; let createdPatId: number | undefined; try { - // Phase 1: Without permission and no tokens — section should be hidden entirely - await page.context().clearCookies(); - await apiLogin(page, email, password); - await page.goto("/app/settings/accounts-access"); - await page.waitForLoadState("networkidle"); - - await expect( - page.getByText("Access Tokens", { exact: true }) - ).not.toBeVisible(); - await expect( - page.getByRole("button", { name: "New Access Token" }) - ).not.toBeVisible(); - - // Phase 2: Grant CREATE_USER_API_KEYS — section visible, button enabled - await page.context().clearCookies(); - await loginAs(page, "admin"); - await adminClient.setUserGroupPermissions(groupId, [ - Permission.CREATE_USER_API_KEYS, - ]); - + // Phase 1: Basic grants it by default, so the section shows and the button is enabled await page.context().clearCookies(); await apiLogin(page, email, password); await page.goto("/app/settings/accounts-access"); @@ -931,7 +918,7 @@ test.describe("Permission gating — CREATE_USER_API_KEYS", () => { await expect(newTokenButton).toBeVisible({ timeout: 10000 }); await expect(newTokenButton).toBeEnabled(); - // Create a PAT via API so Phase 3 can test "token exists but no permission" + // Create a PAT via API so Phase 2 can test "token exists but no permission" const createResponse = await page.request.post("/api/user/pats", { data: { name: `E2E PAT ${Date.now()}`, @@ -942,10 +929,10 @@ test.describe("Permission gating — CREATE_USER_API_KEYS", () => { const patData = await createResponse.json(); createdPatId = patData.id; - // Phase 3: Revoke permission — section stays (token exists) but button disabled + // Phase 2: Switch it off on Basic. The section stays (a token exists) but the button is disabled await page.context().clearCookies(); await loginAs(page, "admin"); - await adminClient.setUserGroupPermissions(groupId, []); + await adminClient.setUserGroupPermissions(basicGroupId, []); await page.context().clearCookies(); await apiLogin(page, email, password); @@ -958,9 +945,39 @@ test.describe("Permission gating — CREATE_USER_API_KEYS", () => { await expect(newTokenButton).toBeVisible({ timeout: 10000 }); await expect(newTokenButton).toBeDisabled(); + + // Phase 3: Switch it back on and the button is enabled again + await page.context().clearCookies(); + await loginAs(page, "admin"); + await adminClient.setUserGroupPermissions(basicGroupId, [ + Permission.CREATE_USER_API_KEYS, + ]); + + await page.context().clearCookies(); + await apiLogin(page, email, password); + await page.goto("/app/settings/accounts-access"); + await page.waitForLoadState("networkidle"); + + await expect(newTokenButton).toBeVisible({ timeout: 10000 }); + await expect(newTokenButton).toBeEnabled(); } finally { + // Basic is shared with every other spec, so restore it from a context + // that outlives this test's page if the test timed out. + await cleanup(async () => { + const restoreContext = await browser.newContext({ + storageState: "admin_auth.json", + }); + try { + await new OnyxApiClient( + restoreContext.request + ).setUserGroupPermissions(basicGroupId, [ + Permission.CREATE_USER_API_KEYS, + ]); + } finally { + await restoreContext.close(); + } + }); await cleanup(async () => { - // Delete the PAT (only requires BASIC_ACCESS) if (createdPatId !== undefined) { await page.context().clearCookies(); await apiLogin(page, email, password);