Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One Token, Multiple Credentials #2970

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

One Token, Multiple Credentials #2970

wants to merge 27 commits into from

Conversation

elias-ba
Copy link
Contributor

@elias-ba elias-ba commented Feb 25, 2025

Description

Overview

This PR refactors how OAuth tokens are stored and referenced, so that multiple “credentials” with the same scope set (for the same OAuth provider) can share a single refresh token. This prevents errors caused when OAuth providers (e.g. Google) invalidate or omit a refresh token if it has already been issued for the same user+client+scopes combination.

Why?
Previously, each credential row stored its own OAuth tokens in credentials.body. This was problematic when:

  • A provider didn’t return a refresh token for a new credential if it was equivalent to an existing token, causing incomplete credentials (missing refresh token).
  • Reauthorizing one credential could invalidate refresh tokens used by other credentials.

What’s New?
We introduce a new oauth_tokens table (previously referred to as credential_bodies) where the actual OAuth token data (including refresh_token, access_token, expires_at, etc.) now lives. Each row in oauth_tokens is keyed by [user_id, oauth_client_id, scope set]. The credentials table references this token row via oauth_token_id.

Problems Addressed

  1. Multiple Credentials, Same Scopes

    • When a user creates two credentials with identical scopes, Google often returns no new refresh_token for the second.
    • The old system tried to save an incomplete credential lacking a refresh_token—which blocked the user.
  2. Reauthorization Overwrites Old Tokens

    • When reauthorizing one credential, OAuth providers will issue a brand-new refresh token and invalidate the old one.
    • Under the old model, that would break other credentials sharing the same scope set.

Goals & Approach

  1. One Token Per [User, OauthClient, Exact Scope Set]
    • If a user tries to create multiple credentials for the same scope set, they share one token record in the DB.
  2. Decouple “Token Storage” from “Credential Record”
    • Migrate token data into a new table: oauth_tokens (fields: user_id, oauth_client_id, scopes, body).
    • credentials references oauth_tokens via oauth_token_id.
  3. Reauthorization Updates
    • When reauthorizing a credential, any new refresh token overwrites the old one in oauth_tokens.
    • All referencing credentials stay valid automatically.
  4. Migration
    • A migration script finds OAuth credentials, extracts their token data, and inserts/merges it into oauth_tokens.
    • The credentials.body is then cleared for OAuth creds, and we set oauth_token_id.

Result: Multiple credentials in the UI can share a single refresh token behind the scenes. If the refresh token is reissued, we update it once and all referencing credentials keep working.

Example Flow

  1. User Creates Credential “Google Sheets 1”

    • OAuth flow returns refresh_token=ABC123.
    • We create one oauth_tokens record with that refresh token, and one credentials record pointing to it.
  2. User Creates Credential “Google Sheets 2”

    • Same scopes, so Google might not return a new refresh token.
    • We detect that the user+client+scopes already exists in oauth_tokens.
    • We simply create a second credentials record pointing to the existing token row. Both credentials share the token.
  3. Reauthorization

    • A new refresh token is issued by Google, invalidating the old one.
    • We overwrite the oauth_tokens.body with the new refresh token.
    • All credentials that point to this oauth_tokens record automatically reference the new token.

Technical Implementation

  1. oauth_tokens Table

    • id, user_id, oauth_client_id, scopes, body (encrypted), timestamps.
    • Unique index on [user_id, oauth_client_id, scopes].
  2. credentials Table

    • New column oauth_token_id → references oauth_tokens.
    • Existing body column is set to hold other credential fields not directly related to OAuth for example the apiVersion and sandbox post-migration.
    • Removal of the old oauth_client_id column in credentials (it now lives in oauth_tokens).

One Token - Multiple Credentials

  1. Ecto Changes

    • OauthToken schema handles creation, updating, and retrieving of tokens.
    • Credential references OauthToken for any OAuth credentials.
    • Credentials.validate_oauth_token_data/5 ensures we either have a refresh token or detect an existing token for a given scope set.
  2. Migration Script

    • Iterates over existing OAuth credentials.
    • Extracts scope data from credentials.body, upserts into oauth_tokens.
    • Updates credentials to point to the newly inserted token record.
    • Drops oauth_client_id column from credentials once migration is verified.

Notes / Future Considerations

  • Partial Scope Overlap: This PR only handles exact scope matches. Overlapping scope sets remain separate tokens.
  • UI Warnings: We might add UI messages (“Creating another token with the same scopes will share a refresh token”).

Conclusion

By splitting out token storage into oauth_tokens keyed by [user_id, oauth_client_id, exact scopes], we fix edge cases around missing refresh tokens, and keep multiple credentials in sync upon reauthorization. This streamlines the OAuth process and prevents accidental breakage when providers reuse or invalidate tokens.

Closes #2908
Might close #2945 too

Validation steps

Additional notes for the reviewer

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

  • I have performed a self-review of my code.
  • I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@elias-ba elias-ba force-pushed the 2908-oauth branch 2 times, most recently from a99d4ef to c4cfe1b Compare February 25, 2025 14:11
Copy link

codecov bot commented Mar 2, 2025

Codecov Report

Attention: Patch coverage is 94.31818% with 10 lines in your changes missing coverage. Please review.

Project coverage is 91.63%. Comparing base (100e5af) to head (8a6b18f).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lib/lightning/credentials.ex 90.65% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2970      +/-   ##
==========================================
+ Coverage   91.61%   91.63%   +0.02%     
==========================================
  Files         346      347       +1     
  Lines       12744    12865     +121     
==========================================
+ Hits        11675    11789     +114     
- Misses       1069     1076       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@elias-ba elias-ba requested a review from jyeshe March 3, 2025 01:28
@elias-ba elias-ba marked this pull request as ready for review March 3, 2025 01:34
@elias-ba elias-ba requested a review from midigofrank March 3, 2025 09:35
Copy link
Member

@jyeshe jyeshe left a comment

Choose a reason for hiding this comment

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

This is awesome work @elias-ba . There are few details left only for future maintainability and it's a beautiful goal!

Copy link
Collaborator

@midigofrank midigofrank left a comment

Choose a reason for hiding this comment

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

Hey @elias-ba , this looks really great. I've left some comments regarding the migration timestamp and the script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remember to update the timestamp of the migration file to latest. This way we can always just mix ecto.rollback if there is a problem. I think there are migrations already in main which are later than 20250224

# then updates credentials to reference these tokens instead of storing the body directly.

# Make sure the application is started
Mix.Task.run("app.start")
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 not sure this can run in release mode given that mix won't be available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

GoogleSheet Oauth Credential Fails on first attempt (No Refresh token) One Token, Multiple Credentials
3 participants