Skip to content

Role-restricted API keys#7807

Open
labkey-adam wants to merge 12 commits into
release26.7-SNAPSHOTfrom
26.7_fb_role_restricted_api_keys
Open

Role-restricted API keys#7807
labkey-adam wants to merge 12 commits into
release26.7-SNAPSHOTfrom
26.7_fb_role_restricted_api_keys

Conversation

@labkey-adam

@labkey-adam labkey-adam commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@cnathe cnathe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is what Claude code review told me:

Finding #1 — [Severity: Critical] — Auth / Broken access control — core/src/org/labkey/core/login/DbLoginAuthenticationProvider.java:126 (and api/.../SecurityManager.java getSessionUser/attemptAuthentication)

▎ Issue: The role restriction is only ever applied to a session user, never to the user returned for the API-key request itself. Trace of a normal stateless API-key request (apikey header, no session cookie — the standard
▎ usage for curl, Rlabkey, the Python labkey package):
▎ 1. SecurityManager.attemptAuthentication calls getSessionUser(request) → no session → null.
▎ 2. It then calls authenticateBasic(...) → AuthenticationManager.authenticate → handleAuthentication, which calls setAuthenticatedUser and stores RESTRICTION_ROLE_KEY on a newly created session, but returns the unrestricted
▎ primaryAuthUser (response.getUser()).
▎ 3. attemptAuthentication returns new Pair<>(u, request) at SecurityManager.java:642 with that unrestricted user, and AuthFilter.java:178 runs the request as that user.

▎ The new RoleRestrictedUser(...) wrapping happens only in getSessionUser (SecurityManager.java:527), i.e. on a subsequent request that carries the JSESSIONID cookie minted in step 2. getRestrictionRole() is consumed in
▎ exactly one place — storing it on the session (SecurityManager.java:826) — and nowhere applies it to the returned user.
▎ Why it matters: A "read-only" (or Author/Editor-restricted) API key grants the user's full, unrestricted permissions on every stateless request — which is essentially all real API-key traffic, since those clients don't
▎ retain the response cookie. The one flagship use case named in the class Javadoc ("read-only API keys, editor-only API keys") does not actually restrict anything. This is a silent privilege-escalation/authorization bypass on
▎ a security feature whose entire purpose is to limit access.
▎ Suggestion: Apply the restriction to the user that is actually returned from the API-key auth path, not just to the session. The cleanest spot is where the restriction role is already known — DbLoginAuthenticationProvider
▎ line 123/126: wrap the user in the response, e.g. AuthenticationResponse.success(configuration, new RoleRestrictedUser(auth.getUser(), auth.restrictionRole())) when auth.restrictionRole() != null. Since
▎ finalizePrimaryAuthentication uses response.getUser() as primaryAuthUser, the restricted user then flows back through authenticate() → attemptAuthentication → AuthFilter.

@cnathe

cnathe commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

@labkey-adam what about the app user profile page usage of this APIKeysPanel component to generate API keys. Do we want to bump the components package version there as well (i.e. in limsModules) to pick up this change?

@labkey-adam

Copy link
Copy Markdown
Contributor Author

@labkey-adam what about the app user profile page usage of this APIKeysPanel component to generate API keys. Do we want to bump the components package version there as well (i.e. in limsModules) to pick up this change?

Thanks, good point... we will want to update the component there as well. I'll create a PR.

…ion into ApiKeyManager to guarantee consistency. Make PermissionsContexts responsible for stashing session attributes. Stash permissions instead of a role for simplicity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants