Skip to content

E2E tests for OAuth token refresh #774

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

Merged
merged 11 commits into from
Jul 17, 2025
Merged

E2E tests for OAuth token refresh #774

merged 11 commits into from
Jul 17, 2025

Conversation

fomalhautb
Copy link
Contributor

@fomalhautb fomalhautb commented Jul 17, 2025


Important

Add E2E tests for OAuth token refresh, update OAuth provider for consent prompt, and enhance mock server with token revocation.

  • E2E Tests:
    • Add tests in connected-accounts.test.ts to verify access token usage, refresh on revocation, and re-authorization prompt on refresh token revocation.
  • OAuth Provider:
    • Add prompt: "consent" to getAuthorizationUrl() in base.tsx.
    • Update MockProvider in mock.tsx to include offline_access in baseScope.
  • Mock OAuth Server:
    • Add token revocation endpoints in index.ts for access and refresh tokens.
    • Implement in-memory storage for revoked tokens in index.ts.

This description was created by Ellipsis for abc2ea3. You can customize this summary. It will automatically update as commits are pushed.

Copy link

vercel bot commented Jul 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
stack-backend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2025 6:06pm
stack-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2025 6:06pm
stack-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2025 6:06pm
stack-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2025 6:06pm

Copy link

recurseml bot commented Jul 17, 2025

✨ No issues found! Your code is sparkling clean! ✨

🗒️ View all ignored comments in this repo
  • The constraint 'TokenStoreType extends string' is too restrictive. It should likely be 'TokenStoreType extends string | object' to match the condition check in line 113 where TokenStoreType is checked against {}
  • Return type mismatch - the interface declares useUsers() returning ServerUser[] but the Team interface that this extends declares useUsers() returning TeamUser[]
  • There is a syntax error in the super constructor call due to the ellipsis operator used incorrectly. Objects aren't being merged correctly. This syntax usage can lead to runtime errors when trying to pass the merged object to 'super()'. Verify that the intended alterations to the object occur before or outside of the super() call if needed.
  • Throwing an error when no active span is found is too aggressive. The log function should gracefully fallback to console.log or another logging mechanism when there's no active span, since not all execution contexts will have an active span. This makes the code less resilient and could break functionality in non-traced environments.

📚 Relevant Docs

  • Function sets backendContext with a new configuration but doesn't pass 'defaultProjectKeys'. Since defaultProjectKeys is required in the type definition and cannot be updated (throws error if tried to set), this will cause a type error.
  • The schema is using array syntax for pick() which is incorrect for Yup schemas. The pick() method in Yup expects individual arguments, not an array. Should be changed to: emailConfigSchema.pick('type', 'host', 'port', 'username', 'sender_name', 'sender_email')

📚 Relevant Docs

  • Creating a refresh token with current timestamp as expiration means it expires immediately. Should set a future date for token expiration.
  • The 'tools' object is initialized as an empty object, even though 'tools' is presumably expected to contain tool definitions. This could cause the server capabilities to lack necessary tool configurations, thus potentially impacting functionalities that depend on certain tool setups.

📚 Relevant Docs

  • 'STACK_SECRET_SERVER_KEY' is potentially being included in every request header without checking its existence again here. Although it's checked during initialization, this could lead to security issues as it's exposed in all communications where the header is logged or captured.

📚 Relevant Docs

  • When adding 'use client' directive at the beginning, it doesn't check if file.text already contains the 'use client' directive. This could lead to duplicate 'use client' directives if the file already has one.

📚 Relevant Docs

Need help? Join our Discord for support!
https://discord.gg/qEjHQk64Z9

@fomalhautb fomalhautb marked this pull request as ready for review July 17, 2025 17:34
@fomalhautb fomalhautb requested a review from N2D4 July 17, 2025 17:34
@fomalhautb fomalhautb assigned N2D4 and unassigned fomalhautb Jul 17, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR implements comprehensive end-to-end testing infrastructure for OAuth token refresh functionality. The changes span multiple components:

  1. Enhanced the mock OAuth provider to better simulate real provider behavior by:

    • Adding 'offline_access' scope support
    • Implementing actual token validation instead of always returning true
    • Adding proper refresh token handling
  2. Modified the base OAuth provider to always request consent screens via prompt: "consent" parameter, ensuring consistent refresh token behavior across providers

  3. Added a development environment flag STACK_ALLOW_SHARED_OAUTH_ACCESS_TOKENS to support testing scenarios

  4. Improved error handling and validation in the connected accounts token CRUD operations

  5. Implemented a mock OAuth server with token revocation capabilities and in-memory token storage for testing

The changes collectively enable thorough testing of OAuth token lifecycle management, including refresh flows and revocation scenarios.

Confidence score: 4/5

  1. This PR is reasonably safe to merge with proper testing
  2. Score of 4 because while the changes are well-structured and focused on testing infrastructure, the shared OAuth tokens feature could potentially impact production behavior if not properly isolated
  3. Files needing attention:
    • apps/backend/.env.development - Ensure the new environment variable is properly documented
    • apps/backend/src/oauth/providers/base.tsx - Verify the consent prompt doesn't affect production flows unintentionally

6 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile

fomalhautb and others added 2 commits July 17, 2025 19:35
…provider_id]/access-token/crud.tsx

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…t.ts

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
fomalhautb and others added 3 commits July 17, 2025 19:51
Copy link

zeropath-ai bot commented Jul 17, 2025

Some new issue(s) might be present. Please use the following link(s) to view them:

https://zeropath.com/app/issues/cfb0967e-d6d5-4918-af9e-9a6d36d2e7ea

Reply to this PR with @zeropath-ai followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.

Copy link

zeropath-ai bot commented Jul 17, 2025

Some new issue(s) might be present. Please use the following link(s) to view them:

https://zeropath.com/app/issues/b603df41-9420-414d-a76e-432176103444

Reply to this PR with @zeropath-ai followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.

@N2D4 N2D4 assigned fomalhautb and unassigned N2D4 Jul 17, 2025
@fomalhautb fomalhautb merged commit c3f6e4d into dev Jul 17, 2025
14 of 17 checks passed
@fomalhautb fomalhautb deleted the oauth-refresh-e2e branch July 17, 2025 18:30
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