feat(js): add clerk.oauthApplication.getConsentInfo#8275
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: d623ef1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
as the FAPI client will set it automatically
dc2379d to
46c893c
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OAuth consent support: new OAuthApplication resource with fetchConsentInfo(params) that GETs /me/oauth/consent/{oauthClientId} (optional scope) and returns normalized consent metadata (application name/logo/url, clientId, state, scopes). Introduces shared TypeScript types (oauthApplication.ts) and re-exports them, exposes an oauthApplication namespace on the Clerk core and IsomorphicClerk, adds Vitest tests covering success, envelope normalization, error, and offline cases, and includes a changeset for a minor release. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/few-stamps-retire.md:
- Line 2: The changeset currently marks '@clerk/clerk-js' as a patch release but
this PR adds new public API (e.g., clerk.oauthApplication.fetchConsentInfo and
its public types), so update the changeset to use a minor bump instead of patch
by replacing the entry value 'patch' with 'minor' for the '@clerk/clerk-js'
package in the .changeset file so the release is versioned correctly.
In `@packages/clerk-js/src/core/resources/__tests__/OAuthApplication.test.ts`:
- Around line 30-63: The tests reference an outdated request contract for
OAuthApplication.fetchConsentInfo and should be updated: change the first test
that asserts BaseResource._fetch was not called to instead assert the current
behavior of OAuthApplication.fetchConsentInfo when BaseResource.clerk.session is
undefined (i.e., that BaseResource._fetch is invoked with the current request
shape rather than not being called); and update the second test's expected
BaseResource._fetch call (the expect on fetchSpy) to match the current
implementation of OAuthApplication.fetchConsentInfo (remove the explicit
sessionId assertion and adjust the call args to whatever the function now
sends—preserve method, encoded path, search/scope and { skipUpdateClient: true }
but do not assert sessionId if the implementation no longer includes it). Ensure
you reference OAuthApplication.fetchConsentInfo and BaseResource._fetch when
making the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bf7a26bb-fb9a-49c6-b5ac-05f2ea9f3b06
📒 Files selected for processing (9)
.changeset/few-stamps-retire.mdpackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/core/resources/OAuthApplication.tspackages/clerk-js/src/core/resources/__tests__/OAuthApplication.test.tspackages/clerk-js/src/core/resources/internal.tspackages/react/src/isomorphicClerk.tspackages/shared/src/types/clerk.tspackages/shared/src/types/index.tspackages/shared/src/types/oauthApplication.ts
packages/clerk-js/src/core/resources/__tests__/OAuthApplication.test.ts
Outdated
Show resolved
Hide resolved
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
| const envelope = json; | ||
| const data = envelope.response ?? json; | ||
| return { | ||
| oauth_application_name: data.oauth_application_name, |
There was a problem hiding this comment.
I think this is deviating from the standard convention of having camelCase property being returned for SDK consumers
|
|
||
| BaseResource.clerk = {} as any; | ||
|
|
||
| await OAuthApplication.fetchConsentInfo({ oauthClientId: 'my/client id', scope: 'openid email' }); |
There was a problem hiding this comment.
I thought this was an odd client id but I think it's actually good to verify that we url escape this to avoid path traversal issues.
| } | ||
|
|
||
| const envelope = json; | ||
| const data = envelope.response ?? json; |
There was a problem hiding this comment.
Seems to be the common pattern that callers to _fetch use .response. I assume that is for client piggybacking. We don't do that for this endpoint (it just uses json directly). But I guess it's good to make sure it will work either way. I added a unit test so we cover both cases.
| export type FetchOAuthConsentInfoParams = { | ||
| /** OAuth `client_id` from the authorize request. */ | ||
| oauthClientId: string; | ||
| /** Optional normalized scope string (e.g. space-delimited). */ |
There was a problem hiding this comment.
"e.g." means "for example". Is space-delimited just an example of a scope string or is this something we should enforce?
wobsoriano
left a comment
There was a problem hiding this comment.
looks good already! left a tiny comment
| return this; | ||
| } | ||
|
|
||
| static async fetchConsentInfo(params: FetchOAuthConsentInfoParams): Promise<OAuthConsentInfo> { |
There was a problem hiding this comment.
Small nit: we don't use a fetch* prefix elsewhere in resources (closest precedents for non-standard verbs are Waitlist.join and Passkey.registerPasskey).
Not blocking, just wondering if getConsentInfo / retrieveConsentInfo or similar would sit better with the rest of the codebase. Happy to keep as-is if you prefer.
There was a problem hiding this comment.
Oh cool, good catch. Looks like get is more common e.g. getSessions, getToken, getDomains, etc. I'll update it!
Co-authored-by: Robert Soriano <sorianorobertc@gmail.com>
Description
Part of USER-3968
This is the first step in packaging/componentizing our OAuth Consent component. We have a couple primary SDK functions we need: fetching consent info, and posting the user's consent choice.
Separate PRs will add the hooks and other necessary functions. I wanted to start with something small to make sure I am using a good pattern.
I also did a manual test of this function in a local project. I can share that if needed.
Considerations
Is
OAuthApplicationa good name for the Resource? Or should it be justOAuthas was also suggested?Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change