feat: Group-based permissions — Phase 1 schema (AccountType, Permission, PermissionGrant)#9547
Open
Subash-Mohan wants to merge 8 commits intomainfrom
Open
feat: Group-based permissions — Phase 1 schema (AccountType, Permission, PermissionGrant)#9547Subash-Mohan wants to merge 8 commits intomainfrom
Subash-Mohan wants to merge 8 commits intomainfrom
Conversation
Contributor
Greptile SummaryThis PR introduces Phase 1 of the group-based permissions schema: a new Compared to the initial review pass, most critical concerns have been resolved:
Two design-level concerns from prior threads remain open without a developer reply and are worth tracking into Phase 2:
Confidence Score: 3/5
Important Files Changed
Entity Relationship Diagram%%{init: {'theme': 'neutral'}}%%
erDiagram
USER {
UUID id PK
UserRole role
AccountType account_type "nullable"
}
USER_GROUP {
int id PK
string name
bool is_up_for_deletion
bool is_default "NEW"
}
USER__USER_GROUP {
int user_group_id PK,FK
UUID user_id PK,FK
bool is_curator
}
PERMISSION_GRANT {
int id PK
int group_id FK
Permission permission
GrantSource grant_source
UUID granted_by FK "nullable"
datetime granted_at
bool is_deleted
}
USER ||--o{ USER__USER_GROUP : "belongs to"
USER_GROUP ||--o{ USER__USER_GROUP : "contains"
USER_GROUP ||--o{ PERMISSION_GRANT : "has (cascade delete)"
USER ||--o{ PERMISSION_GRANT : "granted_by (SET NULL on delete)"
Reviews (8): Last reviewed commit: "feat: update unique constraint for permi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
backend/alembic/versions/25a5501dc766_group_permissions_phase1.py
Outdated
Show resolved
Hide resolved
backend/alembic/versions/25a5501dc766_group_permissions_phase1.py
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
- There is a concrete data-integrity risk in
backend/alembic/versions/25a5501dc766_group_permissions_phase1.py:granted_byappears to reference a user but lacks aForeignKeyConstraint, so orphaned or invalid references could be written without DB enforcement. - The redundant index in
backend/alembic/versions/25a5501dc766_group_permissions_phase1.pyis lower impact (performance/maintenance overhead), sinceUniqueConstraint("group_id", "permission")already supportsgroup_id-leading lookups in PostgreSQL. - Given the medium severity and high confidence on the FK issue, this is not a critical blocker but does introduce meaningful regression risk if merged as-is.
- Pay close attention to
backend/alembic/versions/25a5501dc766_group_permissions_phase1.py- add the missing foreign key ongranted_byand clean up the redundant index definition.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/alembic/versions/25a5501dc766_group_permissions_phase1.py">
<violation number="1" location="backend/alembic/versions/25a5501dc766_group_permissions_phase1.py:58">
P2: Missing `ForeignKeyConstraint` on `granted_by` — this column semantically references the `user` who granted the permission but has no FK, so the database won't enforce referential integrity. Add a foreign key to `user.id` (or document why it's intentionally omitted, e.g. to allow the granting user to be deleted without cascading).</violation>
<violation number="2" location="backend/alembic/versions/25a5501dc766_group_permissions_phase1.py:77">
P2: This index is redundant. The `UniqueConstraint("group_id", "permission")` on the same table already creates a composite index with `group_id` as the leading column, which PostgreSQL can use for `group_id`-only lookups. The extra index wastes disk space and adds write overhead without query benefit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
backend/alembic/versions/25a5501dc766_group_permissions_phase1.py
Outdated
Show resolved
Hide resolved
Contributor
🖼️ Visual Regression Report
|
Comment on lines
+4000
to
+4013
| Enum(Permission, native_enum=False), nullable=False | ||
| ) | ||
| grant_source: Mapped[GrantSource] = mapped_column( | ||
| Enum(GrantSource, native_enum=False), nullable=False | ||
| ) | ||
| granted_by: Mapped[UUID | None] = mapped_column( | ||
| ForeignKey("user.id", ondelete="SET NULL"), nullable=True | ||
| ) | ||
| granted_at: Mapped[datetime.datetime] = mapped_column( | ||
| DateTime(timezone=True), server_default=func.now(), nullable=False | ||
| ) | ||
|
|
||
| group: Mapped["UserGroup"] = relationship( | ||
| "UserGroup", back_populates="permission_grants" |
Contributor
There was a problem hiding this comment.
group_id FK and relationship missing cascade — will block group deletion
PermissionGrant is a first-class entity with its own integer PK (unlike the other association tables that use composite PKs to user_group.id). When the sync worker eventually hard-deletes a UserGroup after it's marked is_up_for_deletion=True, PostgreSQL will raise a FK constraint violation if any permission_grant rows reference that group.
Two fixes are needed:
- Add
ondelete="CASCADE"to the FK so the DB will automatically clean up grants when the group is deleted:
Suggested change
| Enum(Permission, native_enum=False), nullable=False | |
| ) | |
| grant_source: Mapped[GrantSource] = mapped_column( | |
| Enum(GrantSource, native_enum=False), nullable=False | |
| ) | |
| granted_by: Mapped[UUID | None] = mapped_column( | |
| ForeignKey("user.id", ondelete="SET NULL"), nullable=True | |
| ) | |
| granted_at: Mapped[datetime.datetime] = mapped_column( | |
| DateTime(timezone=True), server_default=func.now(), nullable=False | |
| ) | |
| group: Mapped["UserGroup"] = relationship( | |
| "UserGroup", back_populates="permission_grants" | |
| group_id: Mapped[int] = mapped_column(ForeignKey("user_group.id", ondelete="CASCADE"), nullable=False) |
- Add
cascadeto the ORM relationship onUserGroupso ORM-level deletions also propagate:
permission_grants: Mapped[list["PermissionGrant"]] = relationship(
"PermissionGrant", back_populates="group", cascade="all, delete-orphan"
)Without both, any code path that deletes a UserGroup row (directly or via ORM) will be blocked silently or raise a runtime error once this table has data.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/onyx/db/models.py
Line: 4000-4013
Comment:
**`group_id` FK and relationship missing cascade — will block group deletion**
`PermissionGrant` is a first-class entity with its own integer PK (unlike the other association tables that use composite PKs to `user_group.id`). When the sync worker eventually hard-deletes a `UserGroup` after it's marked `is_up_for_deletion=True`, PostgreSQL will raise a FK constraint violation if any `permission_grant` rows reference that group.
Two fixes are needed:
1. Add `ondelete="CASCADE"` to the FK so the DB will automatically clean up grants when the group is deleted:
```suggestion
group_id: Mapped[int] = mapped_column(ForeignKey("user_group.id", ondelete="CASCADE"), nullable=False)
```
2. Add `cascade` to the ORM relationship on `UserGroup` so ORM-level deletions also propagate:
```python
permission_grants: Mapped[list["PermissionGrant"]] = relationship(
"PermissionGrant", back_populates="group", cascade="all, delete-orphan"
)
```
Without both, any code path that deletes a `UserGroup` row (directly or via ORM) will be blocked silently or raise a runtime error once this table has data.
How can I resolve this? If you propose a fix, please make it concise.
nmgarza5
approved these changes
Mar 24, 2026
Comment on lines
+4000
to
+4013
| Enum(Permission, native_enum=False), nullable=False | ||
| ) | ||
| grant_source: Mapped[GrantSource] = mapped_column( | ||
| Enum(GrantSource, native_enum=False), nullable=False | ||
| ) | ||
| granted_by: Mapped[UUID | None] = mapped_column( | ||
| ForeignKey("user.id", ondelete="SET NULL"), nullable=True | ||
| ) | ||
| granted_at: Mapped[datetime.datetime] = mapped_column( | ||
| DateTime(timezone=True), server_default=func.now(), nullable=False | ||
| ) | ||
|
|
||
| group: Mapped["UserGroup"] = relationship( | ||
| "UserGroup", back_populates="permission_grants" |
backend/alembic/versions/25a5501dc766_group_permissions_phase1.py
Outdated
Show resolved
Hide resolved
Comment on lines
+4013
to
+4015
| is_deleted: Mapped[bool] = mapped_column( | ||
| Boolean, nullable=False, server_default=text("false") | ||
| ) |
Contributor
There was a problem hiding this comment.
is_deleted has no Python-side default
is_deleted uses server_default=text("false") for the DB but has no default=False for the Python side. When a PermissionGrant object is created in memory and its is_deleted attribute is accessed before the first flush, it will be None rather than False, which can cause subtle bugs (not grant.is_deleted evaluates to True for None).
Other boolean columns in the codebase (e.g., is_up_for_deletion, is_up_to_date on UserGroup) consistently provide both:
Suggested change
| is_deleted: Mapped[bool] = mapped_column( | |
| Boolean, nullable=False, server_default=text("false") | |
| ) | |
| is_deleted: Mapped[bool] = mapped_column( | |
| Boolean, nullable=False, default=False, server_default=text("false") | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/onyx/db/models.py
Line: 4013-4015
Comment:
**`is_deleted` has no Python-side `default`**
`is_deleted` uses `server_default=text("false")` for the DB but has no `default=False` for the Python side. When a `PermissionGrant` object is created in memory and its `is_deleted` attribute is accessed before the first flush, it will be `None` rather than `False`, which can cause subtle bugs (`not grant.is_deleted` evaluates to `True` for `None`).
Other boolean columns in the codebase (e.g., `is_up_for_deletion`, `is_up_to_date` on `UserGroup`) consistently provide both:
```suggestion
is_deleted: Mapped[bool] = mapped_column(
Boolean, nullable=False, default=False, server_default=text("false")
)
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/onyx/db/models.py">
<violation number="1" location="backend/onyx/db/models.py:4013">
P1: Adding `is_deleted` without updating uniqueness/read filtering makes soft-deleted permission grants behave like active rows. This can block re-grants and leak revoked permissions into authorization reads.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Subash-Mohan
force-pushed
the
group-permissions-phase1-migration
branch
from
0718191 to
f95e630
Compare
Contributor
|
Preview Deployment
|
Comment on lines
+346
to
+393
| class Permission(str, PyEnum): | ||
| """ | ||
| Permission tokens for group-based authorization. | ||
| 19 tokens total. full_admin_panel_access is an override — | ||
| if present, any permission check passes. | ||
| """ | ||
|
|
||
| # Basic (auto-granted to every new group) | ||
| BASIC_ACCESS = "basic" | ||
|
|
||
| # Read tokens — implied only, never granted directly | ||
| READ_CONNECTORS = "read:connectors" | ||
| READ_DOCUMENT_SETS = "read:document_sets" | ||
| READ_AGENTS = "read:agents" | ||
| READ_USERS = "read:users" | ||
|
|
||
| # Add / Manage pairs | ||
| ADD_AGENTS = "add:agents" | ||
| MANAGE_AGENTS = "manage:agents" | ||
| MANAGE_DOCUMENT_SETS = "manage:document_sets" | ||
| ADD_CONNECTORS = "add:connectors" | ||
| MANAGE_CONNECTORS = "manage:connectors" | ||
| MANAGE_LLMS = "manage:llms" | ||
|
|
||
| # Toggle tokens | ||
| READ_AGENT_ANALYTICS = "read:agent_analytics" | ||
| MANAGE_ACTIONS = "manage:actions" | ||
| READ_QUERY_HISTORY = "read:query_history" | ||
| MANAGE_USER_GROUPS = "manage:user_groups" | ||
| CREATE_USER_API_KEYS = "create:user_api_keys" | ||
| CREATE_SERVICE_ACCOUNT_API_KEYS = "create:service_account_api_keys" | ||
| CREATE_SLACK_DISCORD_BOTS = "create:slack_discord_bots" | ||
|
|
||
| # Override — any permission check passes | ||
| FULL_ADMIN_PANEL_ACCESS = "admin" | ||
|
|
||
| # Permissions that are implied by other grants and must never be stored | ||
| # directly in the permission_grant table. | ||
| IMPLIED: ClassVar[frozenset[Permission]] | ||
|
|
||
|
|
||
| Permission.IMPLIED = frozenset( | ||
| { | ||
| Permission.READ_CONNECTORS, | ||
| Permission.READ_DOCUMENT_SETS, | ||
| Permission.READ_AGENTS, | ||
| Permission.READ_USERS, | ||
| } |
Contributor
There was a problem hiding this comment.
UserRole enum during coexistence phase
Three new enum values share identical string representations with existing UserRole values:
| New enum | Value | Existing UserRole |
|---|---|---|
Permission.BASIC_ACCESS |
"basic" |
UserRole.BASIC = "basic" |
Permission.FULL_ADMIN_PANEL_ACCESS |
"admin" |
UserRole.ADMIN = "admin" |
AccountType.EXT_PERM_USER |
"ext_perm_user" |
UserRole.EXT_PERM_USER = "ext_perm_user" |
The PR description explicitly states these new enums "replace the identity aspect of UserRole", so both systems will coexist during the migration. Any code that compares or dispatches on the raw string value (e.g., serialized JSON, API responses, string-typed DB columns shared between systems) could silently match the wrong enum. The most dangerous case is "admin" — if the new permission checker short-circuits on permission_value == "admin" it would also match an UserRole.ADMIN string that leaked into the wrong context.
Consider disambiguating the string representations to make the two systems unambiguous at the serialization boundary:
# Example disambiguated names
BASIC_ACCESS = "perm:basic" # instead of "basic"
FULL_ADMIN_PANEL_ACCESS = "perm:admin" # instead of "admin"and in AccountType:
EXT_PERM_USER = "acct:ext_perm_user" # instead of "ext_perm_user"If changing the string values is too disruptive at this stage, at minimum add a comment documenting the intentional overlap and the phase-out plan to alert future reviewers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/onyx/db/enums.py
Line: 346-393
Comment:
**String value collisions with `UserRole` enum during coexistence phase**
Three new enum values share identical string representations with existing `UserRole` values:
| New enum | Value | Existing `UserRole` |
|---|---|---|
| `Permission.BASIC_ACCESS` | `"basic"` | `UserRole.BASIC = "basic"` |
| `Permission.FULL_ADMIN_PANEL_ACCESS` | `"admin"` | `UserRole.ADMIN = "admin"` |
| `AccountType.EXT_PERM_USER` | `"ext_perm_user"` | `UserRole.EXT_PERM_USER = "ext_perm_user"` |
The PR description explicitly states these new enums "replace the identity aspect of `UserRole`", so both systems will coexist during the migration. Any code that compares or dispatches on the raw string value (e.g., serialized JSON, API responses, string-typed DB columns shared between systems) could silently match the wrong enum. The most dangerous case is `"admin"` — if the new permission checker short-circuits on `permission_value == "admin"` it would also match an `UserRole.ADMIN` string that leaked into the wrong context.
Consider disambiguating the string representations to make the two systems unambiguous at the serialization boundary:
```python
# Example disambiguated names
BASIC_ACCESS = "perm:basic" # instead of "basic"
FULL_ADMIN_PANEL_ACCESS = "perm:admin" # instead of "admin"
```
and in `AccountType`:
```python
EXT_PERM_USER = "acct:ext_perm_user" # instead of "ext_perm_user"
```
If changing the string values is too disruptive at this stage, at minimum add a comment documenting the intentional overlap and the phase-out plan to alert future reviewers.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+371
to
+375
| READ_AGENT_ANALYTICS = "read:agent_analytics" | ||
| MANAGE_ACTIONS = "manage:actions" | ||
| READ_QUERY_HISTORY = "read:query_history" | ||
| MANAGE_USER_GROUPS = "manage:user_groups" | ||
| CREATE_USER_API_KEYS = "create:user_api_keys" |
Contributor
There was a problem hiding this comment.
read: prefix semantics broken by non-implied toggle tokens
Permission.IMPLIED establishes the convention that all read:* prefixed tokens are implied-only and never stored directly in permission_grant. However, READ_AGENT_ANALYTICS = "read:agent_analytics" and READ_QUERY_HISTORY = "read:query_history" use the same read: prefix but are NOT in Permission.IMPLIED — they are explicitly grantable "Toggle tokens."
This breaks the semantic invariant: any future authorization code that uses a prefix heuristic (e.g., permission.value.startswith("read:") to determine implied permissions) would incorrectly skip checking permission_grant for these two tokens, causing silent denial of access. Conversely, code that naively adds all read:* tokens to IMPLIED in a future phase would unintentionally exclude them from direct grants.
Consider renaming these two tokens to avoid the ambiguous read: prefix — for example view:agent_analytics / view:query_history — or explicitly documenting the break from the naming convention with a code comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/onyx/db/enums.py
Line: 371-375
Comment:
**`read:` prefix semantics broken by non-implied toggle tokens**
`Permission.IMPLIED` establishes the convention that all `read:*` prefixed tokens are implied-only and never stored directly in `permission_grant`. However, `READ_AGENT_ANALYTICS = "read:agent_analytics"` and `READ_QUERY_HISTORY = "read:query_history"` use the same `read:` prefix but are NOT in `Permission.IMPLIED` — they are explicitly grantable "Toggle tokens."
This breaks the semantic invariant: any future authorization code that uses a prefix heuristic (e.g., `permission.value.startswith("read:")` to determine implied permissions) would incorrectly skip checking `permission_grant` for these two tokens, causing silent denial of access. Conversely, code that naively adds all `read:*` tokens to `IMPLIED` in a future phase would unintentionally exclude them from direct grants.
Consider renaming these two tokens to avoid the ambiguous `read:` prefix — for example `view:agent_analytics` / `view:query_history` — or explicitly documenting the break from the naming convention with a code comment.
How can I resolve this? If you propose a fix, please make it concise.| ["user.id"], | ||
| ondelete="SET NULL", | ||
| ), | ||
| sa.UniqueConstraint("group_id", "permission"), |
Contributor
There was a problem hiding this comment.
UniqueConstraint missing explicit name parameter
sa.UniqueConstraint("group_id", "permission") has no name argument. PostgreSQL auto-generates a name (typically permission_grant_group_id_permission_key), but that name is not guaranteed to be stable across fresh installs vs. migrated databases, and any future migration that needs to drop_constraint must know the exact name.
Consistent constraint naming also makes alembic revision --autogenerate diffs cleaner and avoids false-positives. The same issue exists in the ORM model's __table_args__.
Suggested change
| sa.UniqueConstraint("group_id", "permission"), | |
| sa.UniqueConstraint("group_id", "permission", name="uq_permission_grant_group_permission"), |
And correspondingly in models.py:
UniqueConstraint("group_id", "permission", name="uq_permission_grant_group_permission"),Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/alembic/versions/25a5501dc766_group_permissions_phase1.py
Line: 91
Comment:
**`UniqueConstraint` missing explicit `name` parameter**
`sa.UniqueConstraint("group_id", "permission")` has no `name` argument. PostgreSQL auto-generates a name (typically `permission_grant_group_id_permission_key`), but that name is not guaranteed to be stable across fresh installs vs. migrated databases, and any future migration that needs to `drop_constraint` must know the exact name.
Consistent constraint naming also makes `alembic revision --autogenerate` diffs cleaner and avoids false-positives. The same issue exists in the ORM model's `__table_args__`.
```suggestion
sa.UniqueConstraint("group_id", "permission", name="uq_permission_grant_group_permission"),
```
And correspondingly in `models.py`:
```python
UniqueConstraint("group_id", "permission", name="uq_permission_grant_group_permission"),
```
How can I resolve this? If you propose a fix, please make it concise.…O for account_type migration
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
how accounts are created and what interface they use — replaces the identity aspect of UserRole
manage:agents, admin) for group-based authorization
(group_id, permission) and audit columns (granted_by, granted_at)
How Has This Been Tested?
Tested by running migrations
Additional Options
Summary by cubic
Phase 1 schema for group-based permissions: adds
AccountTypeon users,PermissionandGrantSourceenums, and apermission_granttable with audit fields, soft delete, and a unique(group_id, permission)constraint. Also adds a default-group flag, a user→groups index, model validation to block implied read tokens, fixes the Alembicdown_revision, and notes a TODO to backfillaccount_typebefore enforcing NOT NULL.New Features
account_typeonuser(backfill planned, then enforce NOT NULL).Permission(19 tokens incl.admin) andGrantSourceenums.permission_granttable with(group_id, permission)uniqueness,grant_source,granted_by,granted_at,is_deleted; group FK cascades on delete.is_defaulttouser_groupto protect default groups.user__user_group(user_id)for faster user→groups lookups.Refactors
AccountTypetoonyx.db.enumsfor reuse.Written for commit c0eed0f. Summary will update on new commits.