chore: add access enforcement to projects handler#2005
chore: add access enforcement to projects handler#2005
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 5b73d02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| return ctx, err | ||
| } | ||
|
|
||
| ctx, err = access.LoadIntoContext(ctx, s.logger, s.db) |
There was a problem hiding this comment.
I'm happy to hear suggestion on a better pattern here. This step loads grants into context so it can be used to verify downstream. I wanted this to be a Goa or HTTP middleware, but we don't have authcontext at that point.
One other option is to load these as part of the Authorize step above, but unsure if that's good practice.
Loading grants is a DB query, so a future step will be to cache these.
tgmendes
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add isEnterpriseOrg guard to Require, RequireAny, and Filter so RBAC enforcement is skipped for non-enterprise accounts at the access layer. Update all tests to use explicit enterprise context to avoid vacuous passes.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ts (behind feature flag)
API key auth does not load grants (no session), so requireAccess and filterByAccess now skip enforcement when grants are absent from context. Also removes leftover fmt.Println debug statements from LoadIntoContext.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if err := s.requireAccess(ctx, access.Check{Scope: access.ScopeBuildRead, ResourceID: proj.ID.String()}); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🚩 GetProject reveals project existence to unauthorized RBAC users
In GetProject, the RBAC check (requireAccess) runs after the project is fetched from the DB (server/internal/projects/impl.go:103-117). This means an authenticated enterprise user without build:read access gets a 403 (Forbidden) for existing projects but a 404 (Not Found) for non-existent ones, leaking project existence information. This ordering is necessary because the project ID (needed for the access check) isn't known until after the DB lookup. Whether this is acceptable depends on the security posture for enterprise RBAC — if project existence is sensitive, consider returning 404 uniformly for both cases.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if payload.OrganizationID != authCtx.ActiveOrganizationID { | ||
| return nil, oops.E(oops.CodeForbidden, nil, "organization does not match active organization context") | ||
| } |
There was a problem hiding this comment.
🚩 CreateProject and ListProjects now restrict to active organization only
The new checks at lines 138 and 232 enforce that payload.OrganizationID must match authCtx.ActiveOrganizationID. Previously, users could create/list projects in any organization they belonged to (verified by the slices.IndexFunc org membership check). This is a behavioral narrowing — users switching between organizations in the same session can no longer operate on non-active orgs. This appears intentional for RBAC (grants are loaded for the active org), but callers relying on the old behavior will now get 403 errors.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func LoadIntoContext(ctx context.Context, logger *slog.Logger, db accessrepo.DBTX) (context.Context, error) { | ||
| logger = logger.With(attr.SlogComponent("access")) | ||
| authCtx, ok := contextvalues.GetAuthContext(ctx) | ||
| if !ok || authCtx == nil || authCtx.SessionID == nil || authCtx.ActiveOrganizationID == "" || authCtx.UserID == "" { | ||
| return ctx, nil | ||
| } | ||
|
|
||
| if authCtx.AccountType != "enterprise" { | ||
| return ctx, nil | ||
| } | ||
|
|
||
| principals := []urn.Principal{urn.NewPrincipal(urn.PrincipalTypeUser, authCtx.UserID)} | ||
| // TODO: once we have role mapping we need to also add grants for roles here | ||
| // principals = append(principals, roleMapping[authCtx.UserID]...) | ||
|
|
||
| grants, err := LoadGrants(ctx, db, authCtx.ActiveOrganizationID, principals) | ||
| if err != nil { | ||
| logger.ErrorContext( | ||
| ctx, | ||
| "failed to load access grants", | ||
| attr.SlogOrganizationID(authCtx.ActiveOrganizationID), | ||
| attr.SlogUserID(authCtx.UserID), | ||
| attr.SlogError(err), | ||
| ) | ||
| return ctx, fmt.Errorf("load access grants: %w", err) | ||
| } | ||
|
|
||
| return GrantsToContext(ctx, grants), nil |
There was a problem hiding this comment.
🚩 LoadIntoContext runs a DB query on every authenticated request for enterprise orgs
The LoadIntoContext call in APIKeyAuth (server/internal/projects/impl.go:89) executes a database query (GetPrincipalGrants) for every request from an enterprise org with a valid session. Since the generated Goa endpoints route both API key and session auth through APIKeyAuth (see server/internal/access/context.go:47), this adds a DB round-trip per request. For high-traffic endpoints like ListProjects or GetProject, this could be a performance concern. Consider caching grants (e.g., in Redis with a short TTL) similar to how productfeatures.Client caches feature flags.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
We will Cache these later, DW boo
2 participants
This adds RBAC enforcement policies to the projects endpoints, as the first PR using this.
Noteworthy callouts: