Skip to content

fix: add authorization checks to Studio OAuth service endpoints#16127

Open
adilburaksen wants to merge 3 commits into
cdapio:developfrom
adilburaksen:fix/oauth-handler-authorization
Open

fix: add authorization checks to Studio OAuth service endpoints#16127
adilburaksen wants to merge 3 commits into
cdapio:developfrom
adilburaksen:fix/oauth-handler-authorization

Conversation

@adilburaksen
Copy link
Copy Markdown

Summary

OAuthHandler exposes three endpoints for managing OAuth providers and credentials
under the Studio data-pipeline service:

  • GET /v1/oauth/provider/{provider}/credential/{credential} — returns an OAuth access token
  • PUT /v1/oauth/provider/{provider}/credential/{credential} — stores credentials
  • PUT /v1/oauth/provider/{provider} — stores a provider config

Unlike the adjacent ConnectionHandler, which calls ContextAccessEnforcer on every
operation, OAuthHandler performed no authorization checks. Any authenticated user
(or an unauthenticated request if the namespace is world-readable) could read back
OAuth access tokens stored by other users.

Fix

Initialize ContextAccessEnforcer from context.getContextAccessEnforcer() in
initialize(), then call enforceOnParent(EntityType.SYSTEM_APP_ENTITY, NamespaceId.SYSTEM, StandardPermission.GET/CREATE) at the entry point of each
handler method, following the pattern already established in ConnectionHandler.java.

Testing

Existing OAuthHandlerTest exercises the happy path. Authorization rejection is
covered by the enforcer's own unit tests; the pattern mirrors the already-tested
ConnectionHandler usage.

OAuthHandler endpoints that create/read OAuth providers and credentials
lacked any permission enforcement, while the adjacent ConnectionHandler
enforces ContextAccessEnforcer checks on every endpoint.

Add enforceOnParent(SYSTEM_APP_ENTITY, NamespaceId.SYSTEM, ...) to:
- putOAuthProvider (CREATE) -- prevents unauthorized provider registration
  and arbitrary tokenRefreshURL injection
- putOAuthCredential (CREATE) -- prevents unauthorized credential storage
- getOAuthCredential (GET) -- prevents unauthorized OAuth access token retrieval

Without these checks, any authenticated caller who can reach the Studio
system-service route can read or exchange stored OAuth credentials for
access tokens belonging to other users.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces authorization checks in OAuthHandler by integrating ContextAccessEnforcer into several endpoints. However, the current implementation will fail to compile because the checked AccessException is neither imported nor handled with try-catch blocks. Additionally, the use of StandardPermission.GET in getOAuthCredential violates the API contract for parent-level enforcement, and several other endpoints in the handler are still missing necessary authorization checks.

- Change StandardPermission.GET to LIST in getOAuthCredential; GET has
  isCheckedOnParent()=false so auth backends may skip the check entirely
- Add LIST check to getAuthURL (was unprotected)
- Add CREATE check to deleteOAuthProvider (was unprotected)
- Add LIST check to getOAuthCredentialValidity (was unprotected)

All six handler methods now enforce on the parent entity before
executing any store or network operation.
@adilburaksen
Copy link
Copy Markdown
Author

Addressed all review feedback in the follow-up commit e250ec981cc:

StandardPermission.GETLIST in getOAuthCredential: Correct — GET has isCheckedOnParent()=false in StandardPermission, so auth backends that check this flag (e.g. third-party AccessController plugins) would silently skip the enforcement. Changed to LIST which has isCheckedOnParent()=true and is the established pattern for read-on-parent checks (matches ConnectionHandler.listConnections).

AccessException try-catch: Not needed — AccessException extends RuntimeException (unchecked). The DefaultContextAccessEnforcer signature declares throws AccessException but since it's a RuntimeException the compiler does not require a catch or declaration at the call site. ConnectionHandler does the same.

Three uncovered endpoints: Added LIST to getAuthURL and getOAuthCredentialValidity, and CREATE to deleteOAuthProvider. All six handler methods now call enforceOnParent before any store operation.

DELETE operation should enforce DELETE permission, not CREATE.
The previous commit incorrectly used StandardPermission.CREATE
for a delete endpoint.
@adilburaksen adilburaksen force-pushed the fix/oauth-handler-authorization branch from 55c88af to e7d5e51 Compare May 16, 2026 18:45
@GnsP GnsP added the build Triggers github actions build label May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Triggers github actions build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants