feat: Collection Permission System#1549
Open
tankerkiller125 wants to merge 3 commits into
Open
Conversation
Contributor
Summary by CodeRabbitRelease Notes
WalkthroughThis PR implements a complete tenant-scoped authorization system replacing owner-based checks with fine-grained permissions, viewers, and row-level access grants. It includes a permission catalog, Ent privacy integration with mutation rules and query filters, service-layer viewer threading, new permission management endpoints, middleware for permission enforcement, and widespread test updates to use a system-context helper. ChangesAuthorization Core & Data Models
Ent Privacy & Authorization Rules
Service Layer Authorization Integration
API Layer Authorization & Endpoints
Security Recommendations🔒 Permission Escalation Prevention
🔒 Last-Admin Protection
🔒 Cross-Tenant Data Leakage
🔒 System Context Bypass
🔒 Permission Validation
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly Related PRs
Suggested Labels
Suggested Reviewers
✨ Finishing Touches⚔️ Resolve merge conflicts
✨ Simplify code
|
coderabbitai
Bot
added
⬆️ enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/internal/core/services/service_user_password_reset.go (1)
127-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce
PasswordMinLengthin the reset flow too.This path still only rejects empty strings. Anyone with a valid reset token can set a 1-character password here, which bypasses the new server-side password policy added in registration. Add the same minimum-length guard before hashing or DB work.
Suggested fix
if rawToken == "" || newPassword == "" { span.SetAttributes(attribute.String("reset.outcome", "missing_input")) return ErrorPasswordResetInvalid } + if len(newPassword) < PasswordMinLength { + span.SetAttributes(attribute.String("reset.outcome", "password_too_short")) + return ErrorPasswordTooShort + } hash := hasher.HashToken(rawToken)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/core/services/service_user_password_reset.go` around lines 127 - 139, In ResetPassword, add the same minimum-length guard used in registration by checking if len(newPassword) < PasswordMinLength immediately after the existing empty-string check (before any hashing or DB work); when triggered set the span attribute (e.g., span.SetAttributes(attribute.String("reset.outcome","password_too_short"))) and return the same error value the registration path uses for short passwords (use the existing project error constant for "password too short" so callers get consistent behavior).backend/app/api/handlers/v1/v1_ctrl_group.go (1)
95-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwagger success code is out of sync with the handler response.
Line 95 documents
@Success 200, but this endpoint returnshttp.StatusCreated(201) at Line 120.As per coding guidelines, use appropriate HTTP status codes and keep Swagger annotations aligned with handler responses.
Suggested fix
-// `@Success` 200 {object} GroupInvitation +// `@Success` 201 {object} GroupInvitation🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/handlers/v1/v1_ctrl_group.go` at line 95, Update the Swagger success annotation to match the handler's actual response: replace the existing "`@Success` 200 {object} GroupInvitation" annotation with "`@Success` 201 {object} GroupInvitation" so the docs reflect the handler returning http.StatusCreated; ensure the change is made in v1_ctrl_group.go next to the existing Swagger comment block for the group invitation endpoint.Source: Coding guidelines
backend/app/api/recurring.go (1)
153-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winShort-circuit on invalid UUID metadata before calling thumbnail creation.
Lines 154-160 log parse errors, but execution still reaches Line 161 and calls
CreateThumbnailwith zero-value UUIDs.Suggested fix
groupId, err := uuid.Parse(msg.Metadata["group_id"]) if err != nil { log.Error().Err(err).Str("group_id", msg.Metadata["group_id"]).Msg("failed to parse group ID from message metadata") + msg.Ack() + continue } attachmentId, err := uuid.Parse(msg.Metadata["attachment_id"]) if err != nil { log.Error().Err(err).Str("attachment_id", msg.Metadata["attachment_id"]).Msg("failed to parse attachment ID from message metadata") + msg.Ack() + continue } err = app.repos.Attachments.CreateThumbnail(authz.NewSystemContext(ctx), groupId, attachmentId, msg.Metadata["title"], msg.Metadata["path"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/recurring.go` around lines 153 - 161, The code parses groupId and attachmentId into UUIDs but only logs parse errors and still calls app.repos.Attachments.CreateThumbnail with possibly zero-value UUIDs; modify the handler so that after attempting to parse msg.Metadata["group_id"] into groupId and msg.Metadata["attachment_id"] into attachmentId (the uuid.Parse calls) you short-circuit on error — e.g., log the error (as already done) and then continue/return to skip calling CreateThumbnail — and only call app.repos.Attachments.CreateThumbnail(authz.NewSystemContext(ctx), groupId, attachmentId, msg.Metadata["title"], msg.Metadata["path"]) when both parses succeeded to ensure valid UUIDs are passed.
🧹 Nitpick comments (2)
backend/internal/core/services/testctx_test.go (1)
13-17: 🏗️ Heavy liftPreserve non-system authz coverage alongside
testCtx().Because
testCtx()uses a system context that bypasses privacy checks, add companion negative tests using real viewer contexts for permission-sensitive service paths so authz regressions remain detectable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/core/services/testctx_test.go` around lines 13 - 17, testCtx() creates a system context via authz.NewSystemContext which bypasses authorization, so add companion negative tests (using real viewer contexts, not testCtx()) for any permission-sensitive service paths exercised by the services test suite to preserve authz coverage; locate service tests that call functions under test with testCtx() and add parallel subtests that build a non-privileged viewer context (e.g., a regular user or anonymous viewer) and assert permission-denied behavior (error type or status) for the same calls, ensuring these new tests fail on regressions while leaving existing system-context tests unchanged.backend/app/api/recurring.go (1)
71-105: Harden pub/sub producer ACLs for privileged job topics.These handlers now execute under system context; ensure topic publish permissions are restricted to trusted internal producers and rotate broker credentials regularly to reduce abuse risk.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/recurring.go` around lines 71 - 105, Handlers registered via runner.AddFunc (the runJobSubscription callbacks that call authz.NewSystemContext and invoke app.services.Exports.RunExport / RunImport for the "collection_export" and "collection_import" topics) run with system privileges; harden the pub/sub side by restricting publish ACLs on those topics to trusted internal producers only, rotate broker credentials regularly, and ensure broker-level authentication is enforced. Additionally, if feasible add a lightweight producer authenticity check in runJobSubscription (e.g., verify a signed metadata token or trusted producer ID in msg.Metadata) before creating a system context and invoking RunExport/RunImport so only authorized producers can trigger privileged jobs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/routes.go`:
- Around line 165-169: The DELETE route currently uses userMW which allows any
tenant member to delete exports; update the route registration that calls
chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(), userMW...) to instead use the
export permission middleware, e.g.
chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(),
withPerm(authz.PermDataExport)...), so deletion is protected by the same
data:export permission as creation/download (no other code changes required).
In `@backend/internal/core/services/service_user.go`:
- Around line 670-699: The last-admin guard in service_user.go is currently a
read-then-delete race (uses svc.repos.Permissions.MemberCount and
svc.repos.Permissions.AdminHolders followed by svc.repos.Users.Delete); make it
atomic by moving the check into the same repository transaction or by adding a
new repo method that does both check and delete inside one DB transaction (for
example add Users.DeleteIfNotLastAdmin(ctx, id) or
Permissions.DeleteUserIfNotLastAdmin(tx, id) that verifies admin holders for
each group and aborts with repo.ErrLastAdmin if it would orphan a tenant, then
performs the delete); update the service call to use that single transactional
repo method and remove the separate read-then-delete sequence.
- Around line 191-192: The code currently sets the new user's Permissions field
directly from token.Permissions which preserves empty/nil invite permissions and
breaks legacy invites; change the construction that contains "Permissions:
token.Permissions" to assign token.Permissions if non-empty, otherwise set it to
authz.FullAccess(), e.g. check for nil/len == 0 and default to
authz.FullAccess() so legacy invitations grant full access; ensure you reference
the same authz.FullAccess() helper used when minting invitations and preserve
any required import or type conversions.
In `@backend/internal/data/authz/viewer.go`:
- Around line 32-39: NewViewer stores the caller's permGroupIDs slice directly
which allows external mutation to change the Viewer’s authorization scope;
modify NewViewer so it makes an owned copy of the permGroupIDs slice before
assigning to the Viewer (e.g., allocate a new slice of len(permGroupIDs), copy
into it, and set Viewer.PermGroupIDs to that copy) to prevent post-construction
mutation affecting authorization decisions.
In `@backend/internal/data/authzrules/admin.go`:
- Around line 105-120: On create (inside the m.Op().Is(ent.OpCreate) branch) add
checks to reject grantees outside the viewer tenant: if m.UserID() is present,
query m.Client().User.Query().Where(user.ID(uid),
user.HasGroupWith(group.ID(v.TenantID))).Exist(authz.NewSystemContext(ctx)) and
deny on error or non-existence; similarly if m.PermissionGroupID() is present,
query m.Client().PermissionGroup.Query().Where(permissiongroup.ID(pgID),
permissiongroup.HasGroupWith(group.ID(v.TenantID))).Exist(authz.NewSystemContext(ctx))
and deny on error or non-existence; keep existing EntityID check and only return
privacy.Allow after all grantee checks pass. Ensure error returns use
privacy.Denyf with the error detail like the existing Entity check.
In `@backend/internal/data/authzrules/entity.go`:
- Around line 18-25: GrantTarget currently returns predicates that match only
the grantee IDs; update it to also constrain grants to the viewer's active
tenant so grants don't cross tenant boundaries. Modify GrantTarget
(predicate.AccessGrant) to include a tenant/group predicate such as
accessgrant.GroupID(v.TenantID) combined with the existing grantee checks (i.e.,
wrap the UserID and PermissionGroupIDIn clauses with
accessgrant.And(accessgrant.GroupID(v.TenantID), ... ) or add GroupID inside
each Or branch) so every returned predicate requires the grant's tenant to equal
v.TenantID.
In `@backend/internal/data/authzrules/resources.go`:
- Around line 167-178: The code currently derives required permission from the
mutation payload via m.Kind(), which lets update/delete use the patch value
instead of the stored row; change the logic so that only create operations use
m.Kind(), and for non-create ops load the existing row's kind from storage and
derive required = authz.PermDataImport vs authz.PermDataExport from that stored
value before calling v.Has(required). Concretely, in the block containing
m.Op().Is(ent.OpCreate) and the preceding permission check, keep the
m.Op().Is(ent.OpCreate) branch to use m.Kind() for creates (and call
allowIfTenantCreate), but for the else (update/delete) resolve the existing
export row (by ID or the mutation’s selector), read its Kind field, set required
accordingly, then check v.Has(required) and continue to apply
m.Where(export.GroupID(v.TenantID)) / return privacy.Allow; ensure any DB lookup
handles not-found/error cases and returns privacy.Deny or the appropriate error.
---
Outside diff comments:
In `@backend/app/api/handlers/v1/v1_ctrl_group.go`:
- Line 95: Update the Swagger success annotation to match the handler's actual
response: replace the existing "`@Success` 200 {object} GroupInvitation"
annotation with "`@Success` 201 {object} GroupInvitation" so the docs reflect the
handler returning http.StatusCreated; ensure the change is made in
v1_ctrl_group.go next to the existing Swagger comment block for the group
invitation endpoint.
In `@backend/app/api/recurring.go`:
- Around line 153-161: The code parses groupId and attachmentId into UUIDs but
only logs parse errors and still calls app.repos.Attachments.CreateThumbnail
with possibly zero-value UUIDs; modify the handler so that after attempting to
parse msg.Metadata["group_id"] into groupId and msg.Metadata["attachment_id"]
into attachmentId (the uuid.Parse calls) you short-circuit on error — e.g., log
the error (as already done) and then continue/return to skip calling
CreateThumbnail — and only call
app.repos.Attachments.CreateThumbnail(authz.NewSystemContext(ctx), groupId,
attachmentId, msg.Metadata["title"], msg.Metadata["path"]) when both parses
succeeded to ensure valid UUIDs are passed.
In `@backend/internal/core/services/service_user_password_reset.go`:
- Around line 127-139: In ResetPassword, add the same minimum-length guard used
in registration by checking if len(newPassword) < PasswordMinLength immediately
after the existing empty-string check (before any hashing or DB work); when
triggered set the span attribute (e.g.,
span.SetAttributes(attribute.String("reset.outcome","password_too_short"))) and
return the same error value the registration path uses for short passwords (use
the existing project error constant for "password too short" so callers get
consistent behavior).
---
Nitpick comments:
In `@backend/app/api/recurring.go`:
- Around line 71-105: Handlers registered via runner.AddFunc (the
runJobSubscription callbacks that call authz.NewSystemContext and invoke
app.services.Exports.RunExport / RunImport for the "collection_export" and
"collection_import" topics) run with system privileges; harden the pub/sub side
by restricting publish ACLs on those topics to trusted internal producers only,
rotate broker credentials regularly, and ensure broker-level authentication is
enforced. Additionally, if feasible add a lightweight producer authenticity
check in runJobSubscription (e.g., verify a signed metadata token or trusted
producer ID in msg.Metadata) before creating a system context and invoking
RunExport/RunImport so only authorized producers can trigger privileged jobs.
In `@backend/internal/core/services/testctx_test.go`:
- Around line 13-17: testCtx() creates a system context via
authz.NewSystemContext which bypasses authorization, so add companion negative
tests (using real viewer contexts, not testCtx()) for any permission-sensitive
service paths exercised by the services test suite to preserve authz coverage;
locate service tests that call functions under test with testCtx() and add
parallel subtests that build a non-privileged viewer context (e.g., a regular
user or anonymous viewer) and assert permission-denied behavior (error type or
status) for the same calls, ensuring these new tests fail on regressions while
leaving existing system-context tests unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6e69db44-3350-40b9-8f78-3513806ded1b
⛔ Files ignored due to path filters (136)
backend/app/api/static/docs/docs.gois excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.yamlis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.yamlis excluded by!backend/app/api/static/docs/**backend/internal/data/ent/accessgrant.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/accessgrant/accessgrant.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/accessgrant/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/accessgrant_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/accessgrant_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/accessgrant_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/accessgrant_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/apikey/apikey.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/apikey_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/apikey_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/apikey_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment/attachment.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles/authroles.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens/authtokens.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/client.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/ent.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entity.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entity/entity.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entity/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entity_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entity_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entity_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entityfield/entityfield.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entityfield_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entityfield_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entityfield_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entitytemplate/entitytemplate.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entitytemplate_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entitytemplate_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entitytemplate_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entitytype/entitytype.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entitytype_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entitytype_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/entitytype_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/export/export.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/export_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/export_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/export_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/generate.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken/groupinvitationtoken.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/has_id.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/hook/hook.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/intercept/intercept.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry/maintenanceentry.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/migrate/schema.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/mutation.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier/notifier.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/passwordresettokens/passwordresettokens.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/passwordresettokens_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/passwordresettokens_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/passwordresettokens_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/permissiongroup.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/permissiongroup/permissiongroup.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/permissiongroup/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/permissiongroup_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/permissiongroup_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/permissiongroup_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/permissiongroup_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/predicate/predicate.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/privacy/privacy.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/access_grant.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/api_key.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/attachment.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/auth_roles.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/auth_tokens.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/entity.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/entity_field.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/entity_template.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/entity_type.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/export.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/group_invitation_token.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/maintenance_entry.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/notifier.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/password_reset_tokens.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/permission_group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/policy_test.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/tag.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/template_field.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/user_group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/tag/tag.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/tag_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/tag_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/tag_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/templatefield/templatefield.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/templatefield_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/templatefield_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/templatefield_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/tx.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/usergroup.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/usergroup/usergroup.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/usergroup_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/usergroup_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/usergroup_update.gois excluded by!backend/internal/data/ent/**docs/public/api/openapi-3.0.jsonis excluded by!docs/public/api/**docs/public/api/openapi-3.0.yamlis excluded by!docs/public/api/**docs/public/api/swagger-2.0.jsonis excluded by!docs/public/api/**docs/public/api/swagger-2.0.yamlis excluded by!docs/public/api/**
📒 Files selected for processing (89)
backend/app/api/cli_reset_password.gobackend/app/api/demo.gobackend/app/api/handlers/v1/v1_ctrl_actions.gobackend/app/api/handlers/v1/v1_ctrl_entities.gobackend/app/api/handlers/v1/v1_ctrl_exports.gobackend/app/api/handlers/v1/v1_ctrl_group.gobackend/app/api/handlers/v1/v1_ctrl_permissions.gobackend/app/api/main.gobackend/app/api/middleware.gobackend/app/api/recurring.gobackend/app/api/routes.gobackend/app/api/setup.gobackend/internal/core/services/all.gobackend/internal/core/services/contexts.gobackend/internal/core/services/contexts_test.gobackend/internal/core/services/main_test.gobackend/internal/core/services/service_exports.gobackend/internal/core/services/service_exports_test.gobackend/internal/core/services/service_group.gobackend/internal/core/services/service_items_attachments_external_test.gobackend/internal/core/services/service_items_attachments_test.gobackend/internal/core/services/service_permissions.gobackend/internal/core/services/service_user.gobackend/internal/core/services/service_user_password_reset.gobackend/internal/core/services/service_user_password_reset_test.gobackend/internal/core/services/service_user_session_test.gobackend/internal/core/services/testctx_test.gobackend/internal/data/authz/authz_test.gobackend/internal/data/authz/grants.gobackend/internal/data/authz/permissions.gobackend/internal/data/authz/viewer.gobackend/internal/data/authzrules/admin.gobackend/internal/data/authzrules/authzrules.gobackend/internal/data/authzrules/entity.gobackend/internal/data/authzrules/filters.gobackend/internal/data/authzrules/resources.gobackend/internal/data/migrations/migcheck/migcheck_test.gobackend/internal/data/migrations/migcheck/pgmig_test.gobackend/internal/data/migrations/postgres/20260609000000_permissions.sqlbackend/internal/data/migrations/sqlite3/20260609000000_permissions.sqlbackend/internal/data/repo/enforcement_test.gobackend/internal/data/repo/main_test.gobackend/internal/data/repo/privacy_canary_test.gobackend/internal/data/repo/repo_authz.gobackend/internal/data/repo/repo_authz_test.gobackend/internal/data/repo/repo_entities.gobackend/internal/data/repo/repo_entities_test.gobackend/internal/data/repo/repo_entity_templates_test.gobackend/internal/data/repo/repo_group.gobackend/internal/data/repo/repo_group_test.gobackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.gobackend/internal/data/repo/repo_maintenance_entry_test.gobackend/internal/data/repo/repo_password_reset_test.gobackend/internal/data/repo/repo_permissions.gobackend/internal/data/repo/repo_tags_test.gobackend/internal/data/repo/repo_tokens_test.gobackend/internal/data/repo/repo_tx.gobackend/internal/data/repo/repo_users.gobackend/internal/data/repo/repo_users_test.gobackend/internal/data/repo/repo_wipe_integration_test.gobackend/internal/data/repo/repos_all.gobackend/internal/data/repo/testctx_test.gobackend/internal/web/mid/errors.gobackend/internal/web/mid/realip.godocs/src/content/docs/en/contribute/development/backend/permissions.mdxfrontend/components/Collection/InviteCreateModal.vuefrontend/components/Collection/MemberPermissionsDialog.vuefrontend/components/Collection/PermissionGroupDialog.vuefrontend/components/Item/SharePanel.vuefrontend/components/Template/Card.vuefrontend/components/ui/dialog-provider/utils.tsfrontend/composables/use-permissions.tsfrontend/layouts/default.vuefrontend/lib/api/__test__/user/group.test.tsfrontend/lib/api/classes/permissions.tsfrontend/lib/api/permissions-utils.tsfrontend/lib/api/types/data-contracts.tsfrontend/lib/api/user.tsfrontend/locales/en.jsonfrontend/pages/collection/index.vuefrontend/pages/collection/index/entity-types.vuefrontend/pages/collection/index/members.vuefrontend/pages/collection/index/permission-groups.vuefrontend/pages/item/[id]/index.vuefrontend/pages/item/[id]/index/edit.vuefrontend/pages/reset-password.vuefrontend/pages/tag/[id].vuefrontend/pages/templates.vue
Comment on lines
+165
to
169
| r.Post("/group/exports", chain.ToHandlerFunc(v1Ctrl.HandleExportsCreate(), withPerm(authz.PermDataExport)...)) | ||
| r.Get("/group/exports", chain.ToHandlerFunc(v1Ctrl.HandleExportsList(), userMW...)) | ||
| r.Get("/group/exports/{id}", chain.ToHandlerFunc(v1Ctrl.HandleExportGet(), userMW...)) | ||
| r.Get("/group/exports/{id}/download", chain.ToHandlerFunc(v1Ctrl.HandleExportDownload(), userMW...)) | ||
| r.Get("/group/exports/{id}/download", chain.ToHandlerFunc(v1Ctrl.HandleExportDownload(), withPerm(authz.PermDataExport)...)) | ||
| r.Delete("/group/exports/{id}", chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(), userMW...)) |
Contributor
There was a problem hiding this comment.
Protect export deletion with data:export.
/group/exports/{id} is still mounted with userMW, but backend/app/api/handlers/v1/v1_ctrl_exports.go deletes both the blob artifact and the export row. That lets any tenant member remove another user's export even when they cannot create or download exports.
🔐 Suggested fix
- r.Delete("/group/exports/{id}", chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(), userMW...))
+ r.Delete("/group/exports/{id}", chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(), withPerm(authz.PermDataExport)...))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| r.Post("/group/exports", chain.ToHandlerFunc(v1Ctrl.HandleExportsCreate(), withPerm(authz.PermDataExport)...)) | |
| r.Get("/group/exports", chain.ToHandlerFunc(v1Ctrl.HandleExportsList(), userMW...)) | |
| r.Get("/group/exports/{id}", chain.ToHandlerFunc(v1Ctrl.HandleExportGet(), userMW...)) | |
| r.Get("/group/exports/{id}/download", chain.ToHandlerFunc(v1Ctrl.HandleExportDownload(), userMW...)) | |
| r.Get("/group/exports/{id}/download", chain.ToHandlerFunc(v1Ctrl.HandleExportDownload(), withPerm(authz.PermDataExport)...)) | |
| r.Delete("/group/exports/{id}", chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(), userMW...)) | |
| r.Post("/group/exports", chain.ToHandlerFunc(v1Ctrl.HandleExportsCreate(), withPerm(authz.PermDataExport)...)) | |
| r.Get("/group/exports", chain.ToHandlerFunc(v1Ctrl.HandleExportsList(), userMW...)) | |
| r.Get("/group/exports/{id}", chain.ToHandlerFunc(v1Ctrl.HandleExportGet(), userMW...)) | |
| r.Get("/group/exports/{id}/download", chain.ToHandlerFunc(v1Ctrl.HandleExportDownload(), withPerm(authz.PermDataExport)...)) | |
| r.Delete("/group/exports/{id}", chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(), withPerm(authz.PermDataExport)...)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/routes.go` around lines 165 - 169, The DELETE route currently
uses userMW which allows any tenant member to delete exports; update the route
registration that calls chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(),
userMW...) to instead use the export permission middleware, e.g.
chain.ToHandlerFunc(v1Ctrl.HandleExportDelete(),
withPerm(authz.PermDataExport)...), so deletion is protected by the same
data:export permission as creation/download (no other code changes required).
Comment on lines
+191
to
+192
| // Invited members get the permissions chosen at invite time. | ||
| Permissions: token.Permissions, |
Contributor
There was a problem hiding this comment.
Preserve legacy invitation behavior when Permissions is empty.
token.Permissions is copied verbatim into the new user. Invitations created before this feature will read back with nil/empty permissions, so joining through one now creates a member with no tenant permissions instead of the previous full-access behavior. Default empty invitation permissions to authz.FullAccess() here as well, not just when minting new invitations.
Suggested fix
+ invitedPerms := token.Permissions
+ if len(invitedPerms) == 0 {
+ invitedPerms = authz.FullAccess()
+ }
+
usrCreate := repo.UserCreate{
Name: data.Name,
Email: data.Email,
Password: &hashed,
IsSuperuser: false,
DefaultGroupID: group.ID,
IsOwner: creatingGroup,
// Invited members get the permissions chosen at invite time.
- Permissions: token.Permissions,
+ Permissions: invitedPerms,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Invited members get the permissions chosen at invite time. | |
| Permissions: token.Permissions, | |
| invitedPerms := token.Permissions | |
| if len(invitedPerms) == 0 { | |
| invitedPerms = authz.FullAccess() | |
| } | |
| usrCreate := repo.UserCreate{ | |
| Name: data.Name, | |
| Email: data.Email, | |
| Password: &hashed, | |
| IsSuperuser: false, | |
| DefaultGroupID: group.ID, | |
| IsOwner: creatingGroup, | |
| // Invited members get the permissions chosen at invite time. | |
| Permissions: invitedPerms, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/core/services/service_user.go` around lines 191 - 192, The
code currently sets the new user's Permissions field directly from
token.Permissions which preserves empty/nil invite permissions and breaks legacy
invites; change the construction that contains "Permissions: token.Permissions"
to assign token.Permissions if non-empty, otherwise set it to
authz.FullAccess(), e.g. check for nil/len == 0 and default to
authz.FullAccess() so legacy invitations grant full access; ensure you reference
the same authz.FullAccess() helper used when minting invitations and preserve
any required import or type conversions.
Comment on lines
+670
to
+699
| // Last-admin guard: deleting this account must not orphan a tenant that | ||
| // still has other members but would lose its only permissions admin. | ||
| sysCtx := authz.NewSystemContext(ctx) | ||
| usr, err := svc.repos.Users.GetOneID(sysCtx, id) | ||
| if err != nil { | ||
| recordServiceSpanError(span, err) | ||
| return err | ||
| } | ||
| for _, gid := range usr.GroupIDs { | ||
| members, err := svc.repos.Permissions.MemberCount(sysCtx, gid) | ||
| if err != nil { | ||
| recordServiceSpanError(span, err) | ||
| return err | ||
| } | ||
| if members <= 1 { | ||
| continue // sole member: nothing to orphan | ||
| } | ||
| holders, err := svc.repos.Permissions.AdminHolders(sysCtx, gid, id) | ||
| if err != nil { | ||
| recordServiceSpanError(span, err) | ||
| return err | ||
| } | ||
| if holders == 0 { | ||
| err := validate.NewRequestError(repo.ErrLastAdmin, http.StatusConflict) | ||
| recordServiceSpanError(span, err) | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| err = svc.repos.Users.Delete(ctx, id) |
Contributor
There was a problem hiding this comment.
Make the last-admin guard atomic with the delete.
This check is still a read-then-delete sequence. If two admins delete themselves concurrently, both requests can observe another remaining admin and both proceed, leaving the tenant with no admin holder. Move this invariant into the same repository transaction as the delete, or add a dedicated repo method that checks and deletes under one transaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/core/services/service_user.go` around lines 670 - 699, The
last-admin guard in service_user.go is currently a read-then-delete race (uses
svc.repos.Permissions.MemberCount and svc.repos.Permissions.AdminHolders
followed by svc.repos.Users.Delete); make it atomic by moving the check into the
same repository transaction or by adding a new repo method that does both check
and delete inside one DB transaction (for example add
Users.DeleteIfNotLastAdmin(ctx, id) or Permissions.DeleteUserIfNotLastAdmin(tx,
id) that verifies admin holders for each group and aborts with repo.ErrLastAdmin
if it would orphan a tenant, then performs the delete); update the service call
to use that single transactional repo method and remove the separate
read-then-delete sequence.
Comment on lines
+32
to
+39
| func NewViewer(userID, tenantID uuid.UUID, superuser bool, perms []string, permGroupIDs []uuid.UUID) *Viewer { | ||
| v := &Viewer{ | ||
| UserID: userID, | ||
| TenantID: tenantID, | ||
| Superuser: superuser, | ||
| Perms: make(map[Permission]struct{}, len(perms)), | ||
| PermGroupIDs: permGroupIDs, | ||
| } |
Contributor
There was a problem hiding this comment.
Copy permGroupIDs in NewViewer to prevent authorization-scope mutation after construction.
NewViewer stores the caller’s slice directly. If that backing array is mutated later, the viewer’s group scope changes implicitly, which can alter row-level authorization decisions.
Suggested fix
func NewViewer(userID, tenantID uuid.UUID, superuser bool, perms []string, permGroupIDs []uuid.UUID) *Viewer {
+ groupIDs := append([]uuid.UUID(nil), permGroupIDs...)
v := &Viewer{
UserID: userID,
TenantID: tenantID,
Superuser: superuser,
Perms: make(map[Permission]struct{}, len(perms)),
- PermGroupIDs: permGroupIDs,
+ PermGroupIDs: groupIDs,
}
v.AddPerms(perms)
return v
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/data/authz/viewer.go` around lines 32 - 39, NewViewer stores
the caller's permGroupIDs slice directly which allows external mutation to
change the Viewer’s authorization scope; modify NewViewer so it makes an owned
copy of the permGroupIDs slice before assigning to the Viewer (e.g., allocate a
new slice of len(permGroupIDs), copy into it, and set Viewer.PermGroupIDs to
that copy) to prevent post-construction mutation affecting authorization
decisions.
Comment on lines
+105
to
+120
| if m.Op().Is(ent.OpCreate) { | ||
| if gid, ok := m.GroupID(); ok && gid != v.TenantID { | ||
| return privacy.Denyf("authz: cross-tenant grant") | ||
| } | ||
| if eid, ok := m.EntityID(); ok { | ||
| exists, err := m.Client().Entity.Query(). | ||
| Where(entity.ID(eid), entity.HasGroupWith(group.ID(v.TenantID)), EntityReadable(v)). | ||
| Exist(authz.NewSystemContext(ctx)) | ||
| if err != nil { | ||
| return privacy.Denyf("authz: checking grant entity: %v", err) | ||
| } | ||
| if !exists { | ||
| return privacy.Denyf("authz: grant entity not accessible") | ||
| } | ||
| } | ||
| return privacy.Allow |
Contributor
There was a problem hiding this comment.
Reject grantees that are outside the viewer's tenant.
This create path verifies the entity's tenant, but it never verifies the target UserID or PermissionGroupID. A caller with permissions:manage can therefore create a grant for an arbitrary user UUID or a permission group from another tenant if they know the ID.
Security recommendation: on create, require user grantees to have membership in v.TenantID, and require permission-group grantees to belong to v.TenantID, before returning privacy.Allow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/data/authzrules/admin.go` around lines 105 - 120, On create
(inside the m.Op().Is(ent.OpCreate) branch) add checks to reject grantees
outside the viewer tenant: if m.UserID() is present, query
m.Client().User.Query().Where(user.ID(uid),
user.HasGroupWith(group.ID(v.TenantID))).Exist(authz.NewSystemContext(ctx)) and
deny on error or non-existence; similarly if m.PermissionGroupID() is present,
query m.Client().PermissionGroup.Query().Where(permissiongroup.ID(pgID),
permissiongroup.HasGroupWith(group.ID(v.TenantID))).Exist(authz.NewSystemContext(ctx))
and deny on error or non-existence; keep existing EntityID check and only return
privacy.Allow after all grantee checks pass. Ensure error returns use
privacy.Denyf with the error detail like the existing Entity check.
Comment on lines
+18
to
+25
| func GrantTarget(v *authz.Viewer) predicate.AccessGrant { | ||
| if len(v.PermGroupIDs) == 0 { | ||
| return accessgrant.UserID(v.UserID) | ||
| } | ||
| return accessgrant.Or( | ||
| accessgrant.UserID(v.UserID), | ||
| accessgrant.PermissionGroupIDIn(v.PermGroupIDs...), | ||
| ) |
Contributor
There was a problem hiding this comment.
Bind grant matching to the active tenant as well as the grantee.
GrantTarget currently matches only UserID / PermissionGroupID. Because this helper feeds EntityReadable, EntityUpdatable, delete predicates, and child-resource checks, any direct grant on tenant A will also match the same user while they are active in tenant B. That breaks the tenant boundary for granted rows.
Security recommendation: include the grant's tenant in this predicate as well (for example accessgrant.GroupID(v.TenantID) alongside the grantee match), so grants only apply inside the viewer's active tenant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/data/authzrules/entity.go` around lines 18 - 25, GrantTarget
currently returns predicates that match only the grantee IDs; update it to also
constrain grants to the viewer's active tenant so grants don't cross tenant
boundaries. Modify GrantTarget (predicate.AccessGrant) to include a tenant/group
predicate such as accessgrant.GroupID(v.TenantID) combined with the existing
grantee checks (i.e., wrap the UserID and PermissionGroupIDIn clauses with
accessgrant.And(accessgrant.GroupID(v.TenantID), ... ) or add GroupID inside
each Or branch) so every returned predicate requires the grant's tenant to equal
v.TenantID.
Comment on lines
+167
to
+178
| required := authz.PermDataExport | ||
| if kind, ok := m.Kind(); ok && kind == export.KindImport { | ||
| required = authz.PermDataImport | ||
| } | ||
| if !v.Has(required) { | ||
| return privacy.Denyf("authz: missing %s", required) | ||
| } | ||
| if m.Op().Is(ent.OpCreate) { | ||
| return allowIfTenantCreate(m, v) | ||
| } | ||
| m.Where(export.GroupID(v.TenantID)) | ||
| return privacy.Allow |
Contributor
There was a problem hiding this comment.
Authorize export mutations from the persisted job kind, not the patch payload.
Line 168 derives the required permission from m.Kind(), but on updates and deletes that field is usually unset unless the mutation is changing it. In that case import jobs fall back to data:export, which lets an export-only user mutate or delete existing import rows.
Security recommendation: for non-create operations, resolve the current row kind first and choose PermDataImport vs PermDataExport from the stored value before allowing the mutation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/data/authzrules/resources.go` around lines 167 - 178, The
code currently derives required permission from the mutation payload via
m.Kind(), which lets update/delete use the patch value instead of the stored
row; change the logic so that only create operations use m.Kind(), and for
non-create ops load the existing row's kind from storage and derive required =
authz.PermDataImport vs authz.PermDataExport from that stored value before
calling v.Has(required). Concretely, in the block containing
m.Op().Is(ent.OpCreate) and the preceding permission check, keep the
m.Op().Is(ent.OpCreate) branch to use m.Kind() for creates (and call
allowIfTenantCreate), but for the else (update/delete) resolve the existing
export row (by ID or the mutation’s selector), read its Kind field, set required
accordingly, then check v.Has(required) and continue to apply
m.Where(export.GroupID(v.TenantID)) / return privacy.Allow; ensure any DB lookup
handles not-found/error cases and returns privacy.Deny or the appropriate error.
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
1 participant
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.
What type of PR is this?
What this PR does / why we need it:
Adds permissions to collections, allowing for fine grained control over what people can do inside collections.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Need to triple check that things work as they should and we aren't introducing a security issue anywhere.