Skip to content

feat: sync resource pool viewer access on OAuth2 connect and disconnect#1317

Open
SalimKayal wants to merge 5 commits into
salimkayal-feat-auto-grant/revoke-RP-Viewers-on-Resource-Pool-CRUDfrom
salimkayal-feat-sync-rp-access-on-oauth2-connect-disconnect
Open

feat: sync resource pool viewer access on OAuth2 connect and disconnect#1317
SalimKayal wants to merge 5 commits into
salimkayal-feat-auto-grant/revoke-RP-Viewers-on-Resource-Pool-CRUDfrom
salimkayal-feat-sync-rp-access-on-oauth2-connect-disconnect

Conversation

@SalimKayal
Copy link
Copy Markdown
Collaborator

@SalimKayal SalimKayal commented May 20, 2026

Summary

This PR implements the second half of automatic resource pool authorization: when a user successfully completes an OAuth2 authorization flow (connects to a provider), they are automatically granted viewer access to all resource pools whose remote.provider_id matches that OAuth client. Conversely, when a user deletes their OAuth2 connection, that access is revoked.

This complements #1314 to make the OAuth2 connection the source of truth for resource pool access.

Motivation & Context

Resource pools can be linked to OAuth2 providers via remote.provider_id. In Chunk 1, we ensured that when an admin creates or updates such a resource pool, all currently-connected users are automatically granted access. However, if a user connects to the provider after the resource pool already exists, they would not receive access until the resource pool is modified.

This PR closes that gap by hooking into the OAuth2 lifecycle events:

  • Connect callback (GET /api/data/oauth2/callback) — triggered after the token exchange succeeds.
  • Disconnect (DELETE /api/data/oauth2/connections/{id}) — triggered when the user deletes their connection.

Design Decisions & Rationale

1. Event-Driven Sync at OAuth Lifecycle Boundaries

We hook into two existing flows rather than adding new endpoints:

Event Location Action
User connects to provider P connected_services/blueprints.py::authorize_callback Call _on_oauth2_connected(user_id, provider_id)
User disconnects from provider P connected_services/db.py::delete_oauth2_connection Call _on_oauth2_disconnected(user_id, provider_id)

This keeps the change localized and avoids polling or background jobs.

2. Best-Effort Sync — OAuth Flow Must Not Fail

Both the connect callback and disconnect operation are wrapped in try/except with warning logs:

try:
    await self.connected_services_repo._on_oauth2_connected(user_id, provider_id)
except Exception as e:
    logger.warning(f"Failed to sync resource pool memberships after OAuth2 connection ...")

Rationale: The OAuth2 connection itself is the primary operation. If SpiceDB is temporarily unavailable or a user record is inconsistent, we still want the user to successfully connect to the provider. The RP access can be retried or fixed later by reconnecting.

3. Disconnect Double-Call Pattern (Transactional + Best-Effort)

In delete_oauth2_connection, _on_oauth2_disconnected is called twice:

  1. Before session.delete(conn) — inside the transaction, so that if revocation fails due to a transient error, the transaction rolls back and the connection is preserved (allowing retry).
  2. After session.flush() — in a separate try/except, as a best-effort cleanup in case the first call succeeded but we want to ensure the connection is deleted even if revocation fails.

This ensures we do not leave orphaned connections when revocation fails, while still attempting to keep DB and Authz state consistent.

4. Cross-Component Dependency Injection

ConnectedServicesRepository now accepts an optional member_repo: MemberRepository | None. This is a cross-component import (connected_servicescrc), which is already practiced elsewhere (e.g., data_api/dependencies.py constructs both repositories). Using TYPE_CHECKING avoids circular import issues at runtime.

5. Exposing user_id and client_id on OAuth2Connection

The OAuth2Connection model and OAuth2ConnectionORM.dump() now include user_id and client_id fields. Previously, only provider_id (which is the same as client_id) was exposed. This allows the authorize_callback blueprint to access the connecting user's ID and the provider ID directly from the fetch_token result without re-querying the database.

Changes

  • components/renku_data_services/connected_services/db.py

    • ConnectedServicesRepository.__init__ now accepts an optional member_repo: MemberRepository.
    • Added _on_oauth2_connected(user_id, client_id): queries all RPs linked to the provider and grants the user as viewer on each.
    • Added _on_oauth2_disconnected(user_id, client_id): queries all RPs linked to the provider and revokes the user's viewer access from each.
    • Both helpers use InternalServiceAdmin() for authz and loop through matching RPs with per-RP try/except.
    • delete_oauth2_connection: calls _on_oauth2_disconnected before deletion and again after flush as best-effort.
  • components/renku_data_services/connected_services/blueprints.py

    • authorize_callback: after successful fetch_token, extracts user_id and provider_id from client.connection and calls _on_oauth2_connected with failure isolation.
  • components/renku_data_services/connected_services/models.py

    • OAuth2Connection dataclass added user_id: str | None = None and client_id: str | None = None.
  • components/renku_data_services/connected_services/orm.py

    • OAuth2ConnectionORM.dump() now includes user_id and client_id in the returned model.
  • bases/renku_data_services/data_api/dependencies.py & test/utils.py

    • Wired member_repo into ConnectedServicesRepository construction.
  • test/components/renku_data_services/connected_services/test_db.py

    • Added comprehensive integration tests:
      • test_oauth_connect_adds_user_to_rp
      • test_oauth_disconnect_removes_user_from_rp
      • test_oauth_connect_with_no_matching_rp_does_nothing
      • test_two_users_connect_same_integration_both_get_access
      • test_user_disconnect_only_affects_their_rp_access
      • test_delete_connection_revokes_rp_access

Behavioral Changes (Notable)

OAuth2 Callback Now Triggers Resource Pool Access Grants

Previously: After a user completed the OAuth2 callback (/api/data/oauth2/callback), they were connected to the provider but received no automatic resource pool access. Admins had to manually grant viewer on each relevant resource pool.

Now: The callback automatically grants viewer access to all resource pools linked to that provider. The user will see these resource pools immediately on their next GET /api/data/resource_pools request.

OAuth2 Disconnect Now Triggers Resource Pool Access Revocation

Previously: Deleting an OAuth2 connection only removed the connection row. Any resource pool viewer grants previously received via that connection remained in SpiceDB.

Now: Disconnecting unconditionally revokes viewer access from all resource pools linked to that provider. This is accepted because the OAuth connection is the source of truth (see Chunk 1 rationale).

OAuth2Connection Model Now Exposes user_id and client_id

Previously: OAuth2Connection only exposed id, provider_id, status, and next_url.

Now: It also exposes user_id and client_id. This is a additive change — existing consumers are unaffected. It enables the callback blueprint to pass the correct IDs to _on_oauth2_connected without additional DB queries.

Resilience: OAuth Flow Succeeds Even If RP Sync Fails

If SpiceDB or the membership repository is unavailable during the OAuth callback, the user will still successfully connect to the provider. A warning is logged and the RP access can be re-synced by reconnecting. This is a deliberate soft-failure strategy to avoid making the OAuth flow dependent on the authz subsystem.

Everything else is backward-compatible:

  • ConnectedServicesRepository.__init__ adds member_repo: MemberRepository | None = None — optional, defaults to None.
  • The auto-grant/revoke logic only runs when member_repo is provided.
  • No database migrations, no Authz schema changes, no API response format changes.

PR Stack

@SalimKayal SalimKayal changed the title Salimkayal feat sync rp access on oauth2 connect disconnect feat: sync rp access on oauth2 connect disconnect May 20, 2026
@SalimKayal SalimKayal force-pushed the salimkayal-feat-sync-rp-access-on-oauth2-connect-disconnect branch from b50f189 to 7438590 Compare May 21, 2026 11:57
@coveralls
Copy link
Copy Markdown

Coverage Report for CI Build 26224451094

Warning

No base build found for commit aac2112 on salimkayal-feat-auto-grant/revoke-RP-Viewers-on-Resource-Pool-CRUD.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 86.41%

Details

  • Patch coverage: 11 uncovered changes across 2 files (41 of 52 lines covered, 78.85%).

Uncovered Changes

File Changed Covered %
components/renku_data_services/connected_services/db.py 42 33 78.57%
components/renku_data_services/connected_services/blueprints.py 7 5 71.43%

Coverage Regressions

Requires a base build to compare against. How to fix this →


Coverage Stats

Coverage Status
Relevant Lines: 30965
Covered Lines: 26757
Line Coverage: 86.41%
Coverage Strength: 1.5 hits per line

💛 - Coveralls

@SalimKayal SalimKayal force-pushed the salimkayal-feat-auto-grant/revoke-RP-Viewers-on-Resource-Pool-CRUD branch from aac2112 to ad7c1f8 Compare May 21, 2026 17:11
@SalimKayal SalimKayal force-pushed the salimkayal-feat-sync-rp-access-on-oauth2-connect-disconnect branch from 7438590 to 3ffeb53 Compare May 21, 2026 17:12
Add _on_oauth2_connected and _on_oauth2_disconnected helpers
that auto-grant/revoke viewer access to resource pools linked
to the provider. Wire member_repo through constructors.

5 new tests cover: connect grant, disconnect revoke, no-match
no-op, multi-user grant, isolated disconnect.
Call _on_oauth2_connected after successful fetch_token in authorize
callback. Call _on_oauth2_disconnected before deleting connection row.
Add test verifying delete_oauth2_connection revokes RP access.
- Fix forward reference in dependencies.py by initializing
  connected_services_repo without member_repo and setting it later
- Add user_id/client_id to OAuth2Connection model for type safety
- Guard blueprint callback against None values
- Remove debug logging from _on_oauth2_connected
@SalimKayal SalimKayal force-pushed the salimkayal-feat-sync-rp-access-on-oauth2-connect-disconnect branch from 3ffeb53 to 94b8d9f Compare May 29, 2026 15:01
@SalimKayal SalimKayal force-pushed the salimkayal-feat-auto-grant/revoke-RP-Viewers-on-Resource-Pool-CRUD branch from ad7c1f8 to 0bb0524 Compare May 29, 2026 15:01
@SalimKayal SalimKayal changed the title feat: sync rp access on oauth2 connect disconnect feat: sync resource pool viewer access on OAuth2 connect and disconnect May 29, 2026
@SalimKayal SalimKayal requested review from leafty, olevski and sambuc May 29, 2026 15:26
@SalimKayal SalimKayal marked this pull request as ready for review May 29, 2026 15:29
@SalimKayal SalimKayal requested review from a team and sgaist as code owners May 29, 2026 15:29
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