feat(ILOC): update to more generic schema to support other Apple apps #105067
Conversation
| # expires_in should be a positive integer number of seconds until expiry | ||
| assert fragment_d["expires_in"] | ||
| assert fragment_d["token_type"] == ["bearer"] | ||
| assert int(fragment_d["expires_in"][0]) > 0 |
There was a problem hiding this comment.
Bug: The OAuth token response contains a negative expires_in value and an incorrect token_type case ("bearer" vs "Bearer"), which will cause new test assertions to fail.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The OAuth token response payload contains two errors. First, the expires_in value is calculated as timezone.now() - token.expires_at, which results in a negative integer. This will cause the new test assertion assert int(fragment_d["expires_in"][0]) > 0 to fail and violates the OAuth2 specification, which requires a positive integer. Second, the implementation returns a lowercase token_type of "bearer", while the updated test asserts the capitalized version "Bearer". Both issues stem from the implementation not being updated to match new test expectations.
💡 Suggested Fix
Update the expires_in calculation to be int((token.expires_at - timezone.now()).total_seconds()) to produce a positive value. Additionally, change the token_type value in the response dictionary from "bearer" to "Bearer" to match the test expectation.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: tests/sentry/web/frontend/test_oauth_authorize.py#L348
Potential issue: The OAuth token response payload contains two errors. First, the
`expires_in` value is calculated as `timezone.now() - token.expires_at`, which results
in a negative integer. This will cause the new test assertion `assert
int(fragment_d["expires_in"][0]) > 0` to fail and violates the OAuth2 specification,
which requires a positive integer. Second, the implementation returns a lowercase
`token_type` of `"bearer"`, while the updated test asserts the capitalized version
`"Bearer"`. Both issues stem from the implementation not being updated to match new test
expectations.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7607579
| assert fragment_d["expires_in"] | ||
| assert fragment_d["token_type"] == ["bearer"] | ||
| assert int(fragment_d["expires_in"][0]) > 0 | ||
| assert fragment_d["token_type"] == ["Bearer"] |
There was a problem hiding this comment.
Bug: Test expects capitalized token_type but production returns lowercase
The test now expects fragment_d["token_type"] == ["Bearer"] with a capital "B", but the production code in oauth_authorize.py at line 444 returns "token_type": "bearer" with a lowercase "b". This mismatch will cause the test to fail. The assertions appear on both lines 344 and 349 (duplicated).
| # expires_in should be a positive integer number of seconds until expiry | ||
| assert fragment_d["expires_in"] | ||
| assert fragment_d["token_type"] == ["bearer"] | ||
| assert int(fragment_d["expires_in"][0]) > 0 |
There was a problem hiding this comment.
Bug: Test expects positive expires_in but calculation yields negative
The new assertion int(fragment_d["expires_in"][0]) > 0 expects a positive value, but the production code in oauth_authorize.py calculates expires_in as timezone.now() - token.expires_at, which produces a negative number since expires_at is in the future. Interestingly, oauth_token.py has the correct calculation (token.expires_at - timezone.now()). This test will fail against the current production code.
We originally did this for a mobile app, but we may now also be adding a mac app, so this should be more generic (the old name was already out of date anyways)