-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(oauth): Implement OAuth 2.0 Device Authorization Flow (RFC 8628) #105675
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
base: master
Are you sure you want to change the base?
Conversation
|
abc32ef to
a25ea15
Compare
|
This PR has a migration; here is the generated SQL for for --
-- Create model ApiDeviceCode
--
CREATE TABLE "sentry_apidevicecode" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "device_code" varchar(64) NOT NULL UNIQUE, "user_code" varchar(16) NOT NULL UNIQUE, "application_id" bigint NOT NULL, "user_id" bigint NULL, "organization_id" bigint NULL, "scope_list" text[] NOT NULL, "expires_at" timestamp with time zone NOT NULL, "status" varchar(20) NOT NULL, "date_added" timestamp with time zone NOT NULL);
ALTER TABLE "sentry_apidevicecode" ADD CONSTRAINT "sentry_apidevicecode_application_id_cf8361a8_fk_sentry_ap" FOREIGN KEY ("application_id") REFERENCES "sentry_apiapplication" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_apidevicecode" VALIDATE CONSTRAINT "sentry_apidevicecode_application_id_cf8361a8_fk_sentry_ap";
ALTER TABLE "sentry_apidevicecode" ADD CONSTRAINT "sentry_apidevicecode_user_id_ec448031_fk_auth_user_id" FOREIGN KEY ("user_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_apidevicecode" VALIDATE CONSTRAINT "sentry_apidevicecode_user_id_ec448031_fk_auth_user_id";
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_device_code_6d4da78d_like" ON "sentry_apidevicecode" ("device_code" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_user_code_90955a60_like" ON "sentry_apidevicecode" ("user_code" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_application_id_cf8361a8" ON "sentry_apidevicecode" ("application_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_user_id_ec448031" ON "sentry_apidevicecode" ("user_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_organization_id_c2717dcf" ON "sentry_apidevicecode" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_expires_at_1f1b6c16" ON "sentry_apidevicecode" ("expires_at"); |
| if scopes: | ||
| pending_scopes = set(scopes) | ||
| matched_sets = set() | ||
| for scope_set in settings.SENTRY_SCOPE_SETS: | ||
| for scope, description in scope_set: | ||
| if scope_set in matched_sets and scope in pending_scopes: | ||
| pending_scopes.remove(scope) | ||
| elif scope in pending_scopes: | ||
| permissions.append(description) | ||
| matched_sets.add(scope_set) | ||
| pending_scopes.remove(scope) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
actually this shows almost 6 hours, so i guess the cost includes the previous cost... |
408d1d2 to
0267f8d
Compare
|
This PR has a migration; here is the generated SQL for for --
-- Create model ApiDeviceCode
--
CREATE TABLE "sentry_apidevicecode" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "device_code" varchar(64) NOT NULL UNIQUE, "user_code" varchar(16) NOT NULL UNIQUE, "application_id" bigint NOT NULL, "user_id" bigint NULL, "organization_id" bigint NULL, "scope_list" text[] NOT NULL, "expires_at" timestamp with time zone NOT NULL, "status" varchar(20) NOT NULL, "date_added" timestamp with time zone NOT NULL);
ALTER TABLE "sentry_apidevicecode" ADD CONSTRAINT "sentry_apidevicecode_application_id_cf8361a8_fk_sentry_ap" FOREIGN KEY ("application_id") REFERENCES "sentry_apiapplication" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_apidevicecode" VALIDATE CONSTRAINT "sentry_apidevicecode_application_id_cf8361a8_fk_sentry_ap";
ALTER TABLE "sentry_apidevicecode" ADD CONSTRAINT "sentry_apidevicecode_user_id_ec448031_fk_auth_user_id" FOREIGN KEY ("user_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_apidevicecode" VALIDATE CONSTRAINT "sentry_apidevicecode_user_id_ec448031_fk_auth_user_id";
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_device_code_6d4da78d_like" ON "sentry_apidevicecode" ("device_code" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_user_code_90955a60_like" ON "sentry_apidevicecode" ("user_code" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_application_id_cf8361a8" ON "sentry_apidevicecode" ("application_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_user_id_ec448031" ON "sentry_apidevicecode" ("user_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_organization_id_c2717dcf" ON "sentry_apidevicecode" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_expires_at_1f1b6c16" ON "sentry_apidevicecode" ("expires_at"); |
BYK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipped the tests (for review) -- assuming you have tested the flow at least once everything looks sound.
I added some comments but none are major or critical IMO. 🚢
src/sentry/models/apidevicecode.py
Outdated
|
|
||
| def generate_device_code(): | ||
| """Generate a cryptographically secure device code (256-bit entropy).""" | ||
| return secrets.token_hex(nbytes=32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend moving the 32 into its own module-level constant like USER_CODE_LENGTH
src/sentry/models/apidevicecode.py
Outdated
| Reference: RFC 8628 §5.1 | ||
| """ | ||
| chars = [secrets.choice(USER_CODE_ALPHABET) for _ in range(USER_CODE_LENGTH)] | ||
| return f"{''.join(chars[:4])}-{''.join(chars[4:])}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The splitting index 4 here is actually dependant on USER_CODE_LENGTH. We should pick either dividing USER_CODE_LENGTH by 2 or have a MAX_CHARS=4 constant where each char group consists of fixed-length (4 in this case) chars.
| # Device code: secret, high-entropy code used for token polling (RFC 8628 §3.2) | ||
| device_code = models.CharField(max_length=64, unique=True, default=generate_device_code) | ||
|
|
||
| # User code: human-readable code for user entry (RFC 8628 §3.2) | ||
| # Format: "XXXX-XXXX" using base-20 alphabet | ||
| # Must be unique since users look up by this code | ||
| user_code = models.CharField(max_length=16, unique=True, default=generate_user_code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some actual uniqueness needs we have:
device_codeonly needs to be unique in combination with the client id (guessingapplicationbelow is that)user_codeonly needs to be unique in combination withuser
So these fields might be overly strict with their current definitions of uniqueness.
src/sentry/models/apidevicecode.py
Outdated
| db_table = "sentry_apidevicecode" | ||
|
|
||
| def __str__(self) -> str: | ||
| return f"device_code={self.id}, application={self.application.id}, status={self.status}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add user_code too?
| else: | ||
| debug_output("Removing expired values for ApiDeviceCode") | ||
| models_attempted.add(ApiDeviceCode.__name__.lower()) | ||
| ApiDeviceCode.objects.filter(expires_at__lt=timezone.now()).delete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any value in "soft deletion" for auditing purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think we do that with grants either, so dont think we want to do it here. if it comes up we can add it later, but we have metrics and logs to some degree for abuse/diagnostics
| # Re-check expiration inside lock (could have expired during lock wait) | ||
| if device_code.is_expired(): | ||
| device_code.delete() | ||
| return self.error( | ||
| request=request, | ||
| name="expired_token", | ||
| reason="device code expired", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit problematic as if I approved, and locking and refetching took a bit longer than it should, I may get a weird error?
| ERR_INVALID_CODE = "Invalid or expired code. Please check the code and try again." | ||
| ERR_EXPIRED_CODE = "This code has expired. Please request a new code from your device." | ||
| ERR_RATE_LIMITED = "Too many attempts. Please wait a minute and try again." | ||
| ERR_NO_ORG_ACCESS = ( | ||
| "This application requires organization-level access. " | ||
| "You must be a member of an organization to authorize this application." | ||
| ) | ||
| ERR_SESSION_EXPIRED = "Your session has expired. Please start over." | ||
| ERR_INVALID_REQUEST = "Invalid request. Please start over." | ||
| ERR_INVALID_OP = "Invalid operation." | ||
| ERR_SELECT_ORG = "Please select an organization." | ||
| ERR_INVALID_ORG = "Invalid organization selection." | ||
| ERR_NO_ORG_PERMISSION = "You don't have access to the selected organization." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these are translatable I guess? Do we care?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my understanding is we're likely to back out all localization in favor of future better solutions given we have done a poor job of maintaining it anyways, so yeah dont care atm
| Handles case variations, missing dashes, and extra whitespace. | ||
| """ | ||
| normalized = user_code.replace("-", "").upper().strip() | ||
| if len(normalized) == 8: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those hard-coded 8 and 4 values again.
| request.session[session_key] = { | ||
| "device_code_id": device_code.id, | ||
| "user_id": request.user.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we index by user + user_code, do we still need to store device_code_id still?
|
|
||
| # Build the verification URIs | ||
| verification_uri = absolute_uri("/oauth/device/") | ||
| verification_uri_complete = f"{verification_uri}?user_code={device_code.user_code}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dangerous URL construction?
Add support for the Device Authorization Grant, enabling headless clients (CLIs, CI/CD pipelines, containers) to obtain OAuth tokens by having users authorize on a separate device with a browser. Key components: - ApiDeviceCode model with secure device/user code generation - Device authorization endpoint (POST /oauth/device_authorization) - User verification pages (GET/POST /oauth/device) - Token endpoint support for device_code grant type - Automatic cleanup of expired device codes
Security improvements: - Add locking to device code token exchange to prevent race conditions - Key sessions by user_code to support multiple parallel authorizations and prevent multi-tab session overwrite attacks - Add TODO comment about similar issue in oauth_authorize.py Code refactoring: - Extract _error_response() helper to eliminate duplicate error patterns - Extract _get_validated_device_code() to centralize validation logic - Extract _handle_approve() and _handle_deny() for cleaner flow - Add _normalize_user_code() for consistent code formatting - Standardize error messages as module-level constants - Simplify post() method from ~180 lines to ~15 lines
The SetRemoteAddrFromForwardedFor middleware already handles X-Forwarded-For and sets REMOTE_ADDR correctly. This aligns with how the rest of the codebase handles client IP retrieval for rate limiting.
…roval Use filter().update() instead of save() to atomically mark device code as approved only if status is still PENDING. This prevents a TOCTOU race where two users entering the same user_code could both pass validation and race to set the user_id, with the last write winning.
- Use .id instead of _id FK attributes for mypy compatibility - Add UnableToAcquireLock exception handling in token exchange - Add type assertion for request.user.id in approval flow - Fix timedelta import in tests
Use the same atomic filter().update() pattern as approval to prevent race condition where denial could overwrite an already-approved device code, causing inconsistent state (ApiAuthorization exists but client receives access_denied).
Add ApiDeviceCode to the create_exhaustive_sentry_app method so that backup/export tests properly include the new model in scoping tests.
Add the new oauth/device/ and oauth/device_authorization/ routes to the controlsiloUrlPatterns.ts file to fix the test_no_missing_urls test.
Align with industry conventions (GitHub, Google) by using the shorter /oauth/device/code path instead of /oauth/device_authorization. - /oauth/device/code - device authorization endpoint (POST) - /oauth/device - user verification page (GET/POST)
Move the device code status check before authorization creation and wrap both operations in a transaction. This prevents orphaned authorizations if the device code update fails (e.g., due to race condition where another request already processed the device code). Previously, the authorization was created first, then the device code status was checked. If the status check failed, the authorization would persist even though no token could be issued.
Wrap token creation and device code deletion in a transaction to prevent duplicate tokens if delete fails after token creation. This follows the same pattern as grant_exchanger.py.
PostgreSQL aborts transactions on IntegrityError, preventing subsequent DB operations. Move try/except outside atomic block to allow the scope-merging code in the except block to run correctly.
Add expiration check after re-fetching device code inside the lock to prevent a race condition where a code could expire during lock wait.
Migration 1015 was taken by backfill_self_hosted_sentry_app_emails on master, so rename to 1016 and update dependencies.
- Extract magic numbers into named constants (DEVICE_CODE_BYTES, USER_CODE_GROUP_LENGTH) - Add user_code to ApiDeviceCode.__str__ for better debugging - Add "You can now close this tab" UX message to completion page - Use constants in _normalize_user_code instead of hardcoded values
2426197 to
836a149
Compare
|
This PR has a migration; here is the generated SQL for for --
-- Create model ApiDeviceCode
--
CREATE TABLE "sentry_apidevicecode" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "device_code" varchar(64) NOT NULL UNIQUE, "user_code" varchar(16) NOT NULL UNIQUE, "application_id" bigint NOT NULL, "user_id" bigint NULL, "organization_id" bigint NULL, "scope_list" text[] NOT NULL, "expires_at" timestamp with time zone NOT NULL, "status" varchar(20) NOT NULL, "date_added" timestamp with time zone NOT NULL);
ALTER TABLE "sentry_apidevicecode" ADD CONSTRAINT "sentry_apidevicecode_application_id_cf8361a8_fk_sentry_ap" FOREIGN KEY ("application_id") REFERENCES "sentry_apiapplication" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_apidevicecode" VALIDATE CONSTRAINT "sentry_apidevicecode_application_id_cf8361a8_fk_sentry_ap";
ALTER TABLE "sentry_apidevicecode" ADD CONSTRAINT "sentry_apidevicecode_user_id_ec448031_fk_auth_user_id" FOREIGN KEY ("user_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_apidevicecode" VALIDATE CONSTRAINT "sentry_apidevicecode_user_id_ec448031_fk_auth_user_id";
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_device_code_6d4da78d_like" ON "sentry_apidevicecode" ("device_code" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_user_code_90955a60_like" ON "sentry_apidevicecode" ("user_code" varchar_pattern_ops);
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_application_id_cf8361a8" ON "sentry_apidevicecode" ("application_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_user_id_ec448031" ON "sentry_apidevicecode" ("user_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_organization_id_c2717dcf" ON "sentry_apidevicecode" ("organization_id");
CREATE INDEX CONCURRENTLY "sentry_apidevicecode_expires_at_1f1b6c16" ON "sentry_apidevicecode" ("expires_at"); |
Remove `# for type checker` and similar comments that aren't used elsewhere in the codebase. The assertions are self-explanatory.
| ) | ||
| continue | ||
|
|
||
| raise UserCodeCollisionError("Unable to generate unique device code") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code after retry loop
Low Severity
The raise UserCodeCollisionError("Unable to generate unique device code") statement at line 214 is unreachable. The for loop on line 200 iterates through attempts 0 through 9. If create() succeeds, the function returns. If create() raises IntegrityError on the last attempt (when attempt == MAX_CODE_GENERATION_RETRIES - 1), the exception is raised inside the loop at line 209. There's no execution path that allows the loop to complete normally and reach line 214. This is dead code that could confuse future maintainers.
🔬 Verification Test
Why verification test was not possible: This is a static code analysis finding about unreachable code. The code path can be verified by logical analysis of the control flow:
MAX_CODE_GENERATION_RETRIES = 10(line 56)- Loop iterates attempts 0-9 (line 200)
- On success:
returnat line 202 (exits function) - On IntegrityError with attempt == 9:
raiseat line 209 (exits function) - On IntegrityError with attempt < 9:
continue(next iteration)
No path exists that completes the loop normally. This can be confirmed by running a Python linter that detects unreachable code (e.g., pylint with "unreachable" check or examining the bytecode).
Summary
Adds support for the OAuth 2.0 Device Authorization Grant (RFC 8628), enabling headless clients (CLIs, CI/CD pipelines, Docker containers) to obtain OAuth tokens by having users authorize on a separate device with a browser.
Key Components
POST /oauth/device_authorization) - Returns device_code, user_code, and verification URLsGET/POST /oauth/device) - Where users enter the code and approve/deny accessurn:ietf:params:oauth:grant-type:device_codegrant typeFlow
POST /oauth/device_authorizationdevice_code(secret) anduser_code(human-readable likeABCD-EFGH)user_codeandverification_urito userPOST /oauth/tokenwithdevice_codeRefs #99002
Refs getsentry/sentry-mcp#546