dynamic org resources #1
Conversation
Co-authored-by: Taesu <166604494+bytaesu@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: Bereket Engida <Bekacru@gmail.com> Co-authored-by: Bereket Engida <86073083+Bekacru@users.noreply.github.com>
…er-auth#6601) Co-authored-by: Bereket Engida <86073083+Bekacru@users.noreply.github.com> Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: Bereket Engida <86073083+Bekacru@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…er-auth#6454) Co-authored-by: Bereket Engida <86073083+Bekacru@users.noreply.github.com>
…-auth#6585)" This reverts commit 137863c.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: matteobadini <matteo.badini@gellify.com>
WalkthroughAdds per-organization custom RBAC resources: new CRUD endpoints, runtime utilities and caching for organization resources, integration into role creation (including auto-creation of missing resources), schema and type extensions, error codes, tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OrgAPI as Org Plugin API
participant AC as AccessControl
participant DB as Database
participant Cache as Cache
Client->>OrgAPI: POST /organization/create-resource (orgId, resource, permissions)
activate OrgAPI
OrgAPI->>AC: getOrganizationStatements(orgId)
AC->>OrgAPI: merged statements (defaults + custom)
OrgAPI->>OrgAPI: validate name, permissions, reserved, max limit
OrgAPI->>DB: check existing resource
alt exists
OrgAPI-->>Client: 400 RESOURCE_NAME_IS_ALREADY_TAKEN
else
OrgAPI->>DB: insert organizationResource
OrgAPI->>Cache: invalidateResourceCache(orgId)
OrgAPI-->>Client: 200 created
end
deactivate OrgAPI
sequenceDiagram
participant Client
participant OrgAPI as Org Plugin API
participant AC as AccessControl
participant DB as Database
participant Cache as Cache
Client->>OrgAPI: POST /organization/create-role (roleName, permissions)
activate OrgAPI
OrgAPI->>OrgAPI: call canCreateRole callback
alt denied
OrgAPI-->>Client: 403 FORBIDDEN
else allowed
OrgAPI->>AC: getOrganizationStatements(orgId)
AC->>OrgAPI: statements
OrgAPI->>OrgAPI: validate permissions against statements
alt missing resources
OrgAPI->>DB: create missing organizationResource (parallel)
OrgAPI->>Cache: invalidateResourceCache(orgId)
OrgAPI->>AC: reload statements
OrgAPI->>OrgAPI: revalidate permissions
end
OrgAPI->>DB: insert role
OrgAPI-->>Client: 200 role created
end
deactivate OrgAPI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts (1)
1118-1160: Wrap resource auto-creation in a transaction to prevent partial failures.The resource auto-creation loop (lines 1146–1210) executes multiple
adapter.create()calls without transactional protection. If one resource creation fails after others succeed, the organization will be left in an inconsistent state with partial data persisted.The framework provides
runWithTransactionutility for wrapping operations—use it to ensure all resources are created atomically, or implement rollback logic for partial failures.
🧹 Nitpick comments (9)
docs/content/docs/plugins/organization.mdx (1)
1835-1835: Simplify potentially wordy phrasing.Line 1835 uses "merged together" which could be more concise:
- 3. **Merging**: When checking permissions or creating roles, both default and custom resources are merged together. If a custom resource has the same name as a default resource, the custom one takes precedence. + 3. **Merging**: When checking permissions or creating roles, default and custom resources are merged. If a custom resource has the same name as a default resource, the custom one takes precedence.ORGANIZATION_PLUGIN_RBAC_RESEARCH.md (1)
90-90: Add language specifiers to fenced code blocks in research document.Several fenced code blocks lack language specifiers for proper syntax highlighting:
- Line 90: Should specify
yamlor similar for the directory structure- Line 348: Should specify language (appears to be pseudocode/diagram)
- Line 369: Should specify language (appears to be pseudocode/diagram)
- Line 392: Should specify language (appears to be pseudocode/diagram)
Example fix for line 90:
- ``` + ```yaml packages/better-auth/src/plugins/organization/Also applies to: 348-348, 369-369, 392-392
e2e/smoke/test/organization-custom-resources.test.ts (2)
66-78: Consider adding error handling for organization creation.If
org.datais undefined (e.g., creation fails), the non-null assertion on line 77 will cause a runtime error that may be difficult to debug. Consider adding an explicit assertion.beforeAll(async () => { // Create an organization for testing const org = await authClient.organization.create( { name: "Test Organization", slug: "test-org-resources", }, { headers, }, ); + expect(org.data).toBeDefined(); organizationId = org.data!.id; });
342-378: Test relies on execution order and prior test state.This test depends on resources created in previous tests (
MyCustomResource,task). The comment at line 343 says "10 total, already have defaults" but the limit applies to custom resources, not defaults. Consider either:
- Cleaning up resources before this test
- Clarifying the comment to reflect actual state
The current approach works but may be fragile if tests are reordered or run in isolation.
packages/better-auth/src/plugins/organization/load-resources.test.ts (1)
175-185: Cache tests only verify functions don't throw.The cache management tests are minimal—they only verify that the functions exist and don't throw. While the comment acknowledges this, consider adding tests that verify actual cache behavior (e.g., verifying that
invalidateResourceCacheactually removes cached data for a specific org).packages/better-auth/src/plugins/organization/load-resources.ts (1)
22-26: Cache returns stale data if resources are modified externally.There's no TTL on cached entries. If resources are modified directly in the database (e.g., via admin tools or another service), the cache will serve stale data indefinitely until explicitly invalidated.
Consider adding a TTL or a cache-through pattern for reads.
packages/better-auth/src/plugins/organization/routes/crud-resources.ts (3)
202-207: Redundant validation: Zod schema already enforces min(1) on permissions.The
baseCreateResourceSchemaat line 73 already specifies.min(1)for the permissions array. This runtime check is redundant since Zod validation happens before the endpoint handler.- // Validate permissions array - if (!permissions || permissions.length === 0) { - throw new APIError("BAD_REQUEST", { - message: ORGANIZATION_ERROR_CODES.INVALID_PERMISSIONS_ARRAY, - }); - }
831-840: Inconsistent type casting for default resource permissions.Default resources from
options.ac?.statementsmay have readonly string arrays, but they're cast tostring[]. This is safe but the type assertion could be more precise.
865-867: List endpoint returns mixed types for default vs custom resources.Default resources lack fields like
id,organizationId,createdAtthat custom resources have. Consider documenting this or normalizing the response shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
ORGANIZATION_PLUGIN_RBAC_RESEARCH.md(1 hunks)docs/content/docs/plugins/organization.mdx(1 hunks)e2e/smoke/test/organization-custom-resources.test.ts(1 hunks)package.json(1 hunks)packages/better-auth/src/plugins/organization/error-codes.ts(1 hunks)packages/better-auth/src/plugins/organization/has-permission.ts(3 hunks)packages/better-auth/src/plugins/organization/load-resources.test.ts(1 hunks)packages/better-auth/src/plugins/organization/load-resources.ts(1 hunks)packages/better-auth/src/plugins/organization/organization.ts(6 hunks)packages/better-auth/src/plugins/organization/routes/crud-access-control.ts(9 hunks)packages/better-auth/src/plugins/organization/routes/crud-resources.ts(1 hunks)packages/better-auth/src/plugins/organization/schema.ts(4 hunks)packages/better-auth/src/plugins/organization/types.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/better-auth/src/plugins/organization/has-permission.ts (2)
packages/better-auth/src/plugins/organization/load-resources.ts (1)
getOrganizationAccessControl(110-117)packages/better-auth/src/plugins/access/access.ts (1)
role(8-77)
e2e/smoke/test/organization-custom-resources.test.ts (4)
packages/better-auth/src/plugins/access/access.ts (1)
createAccessControl(79-88)packages/better-auth/src/plugins/organization/access/statement.ts (2)
ownerAc(21-27)memberAc(29-35)packages/better-auth/src/plugins/organization/organization.ts (1)
organization(440-1336)packages/better-auth/src/plugins/organization/client.ts (1)
organizationClient(81-279)
🪛 LanguageTool
ORGANIZATION_PLUGIN_RBAC_RESEARCH.md
[style] ~755-~755: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ion level? - Cache per request? - Cache in memory map? Answer: in memory map ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~769-~769: Ensure spelling is correct
Context: ...and let me specify on the plugin config reseved names. --- ## 13. Summary The organi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/content/docs/plugins/organization.mdx
[style] ~1835-~1835: ‘merged together’ might be wordy. Consider a shorter alternative.
Context: ..., both default and custom resources are merged together. If a custom resource has the same name...
(EN_WORDINESS_PREMIUM_MERGED_TOGETHER)
[grammar] ~2151-~2151: Use a hyphen to join words.
Context: ...c You can extend the default permission checking logic for role creation by prov...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
ORGANIZATION_PLUGIN_RBAC_RESEARCH.md
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
348-348: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
392-392: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (26)
docs/content/docs/plugins/organization.mdx (3)
1772-1835: Verify Custom Resources documentation does not appear elsewhere in the file.The AI summary indicates the Custom Resources section may be duplicated. Please verify this section (lines 1772-2267) appears only once in the file.
1799-1825: Verify custom resources configuration names are finalized.Lines 1799-1800 reference
enableCustomResourcesandmaximumResourcesPerOrganization. Confirm these exact option names match the implementation in the organization plugin configuration.
2138-2148: Verify permissions required for custom resource management are accurate.Lines 2138-2147 specify that users need
ac:create,ac:read,ac:update, andac:deletepermissions to manage custom resources. Confirm this is the intended permission model and that these are the only permissions required.package.json (1)
53-60: Verify React 19 type versions match actual React versions.Lines 54-59 define React 19 catalog with
@types/react@^19.2.0andreact@^19.2.1. Confirm the type versions (19.2.0) are compatible with the runtime versions (19.2.1) and align with the React 19 documentation requirements mentioned in the codebase.ORGANIZATION_PLUGIN_RBAC_RESEARCH.md (2)
1-100: Verify research document accurately reflects the implementation plan.This research document provides comprehensive architectural guidance for custom resources. Before implementation is finalized, confirm that:
- The database schema proposed (Section 8.1) matches the actual schema implementation
- The configuration options (Section 8.2) align with what's been coded
- The validation approach (Section 8.5) matches the actual validation logic
- The caching strategy described is implemented as intended
680-750: Confirm implementation decisions match documented answers.Section 12 "Questions to Consider" documents design decisions:
- Resource deletion blocked if in use (Option B)
- Resource renaming not allowed (Option C)
- Static resources paired with dynamic ones
- Explicit permission definitions required
- In-memory caching for performance
- Organization-level resources (not global)
- Naming restrictions with configurable reserved names
Verify that the implementation follows these documented decisions consistently throughout the codebase.
e2e/smoke/test/organization-custom-resources.test.ts (2)
427-438: Verify: MissingorganizationIdin request body.Unlike other tests in this file that explicitly pass
organizationId, this call omits it. If the API is expected to use the active organization from the session, this is fine. However, iforganizationIdis required, this test may fail for the wrong reason.
1-61: Well-structured e2e test setup.The test setup properly configures access control, auth instance, and client with matching configurations. Good use of
customFetchImplfor testing.packages/better-auth/src/plugins/organization/has-permission.ts (2)
36-39: LGTM: Organization-specific AC selection.The conditional logic correctly selects the organization-specific access control when custom resources are enabled, falling back to the global AC otherwise. This ensures roles can reference organization-specific custom resources.
74-75: Correct usage of organization-specific AC for role creation.Using
orgAc.newRole()ensures that roles referencing custom resources are properly validated against the organization's merged statement set.packages/better-auth/src/plugins/organization/schema.ts (3)
51-77: LGTM: Well-defined resource schema fields.The
OrganizationResourceDefaultFieldsinterface correctly defines the database schema with proper foreign key reference to organization.
380-387: Consistent with existing patterns.The Zod schema defines
permissionsas an array, matching the application-level type. This follows the same pattern asorganizationRoleSchemawhere complex types are serialized to JSON strings for database storage.
238-246: Correct conditional schema extension.The
organizationResourceschema is properly included only whenenableCustomResourcesis true, following the same pattern used fororganizationRole.packages/better-auth/src/plugins/organization/error-codes.ts (1)
91-102: Comprehensive error codes for resource management.The new error codes cover all expected failure scenarios for resource CRUD operations. Messages are clear and follow the existing naming conventions.
packages/better-auth/src/plugins/organization/load-resources.test.ts (3)
72-84: Potential inconsistency: Error message says "lowercase" but uppercase is accepted.Tests at lines 63-69 verify that uppercase names like
"Project","MyResource", and"UPPERCASE"are valid. However, the error message checked at lines 76 and 83 contains"lowercase alphanumeric". This suggests the error message in the implementation may be misleading since uppercase letters are actually accepted.Consider updating the implementation's error message to say "alphanumeric" instead of "lowercase alphanumeric" for accuracy.
1-14: Good test isolation with beforeEach cache clearing.Clearing the cache before each test ensures test isolation and prevents flaky tests due to shared state.
187-212: Thorough edge case coverage.Good coverage of edge cases including leading/trailing underscores, numeric-only names, and various case combinations.
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts (2)
177-232: Well-structured callback pattern for custom role creation authorization.The three-way return pattern (
"yes","default",{allowed: false, message}) provides clear semantics and good extensibility. The logic correctly short-circuits delegation checks when the callback explicitly allows creation.
1322-1334: Good defensive implementation for skipping delegation checks on auto-created resources.The logging and early
continuefor auto-created resources correctly prevents requiring permissions for resources that were just created by the same operation.packages/better-auth/src/plugins/organization/types.ts (2)
88-160: Well-documented type definitions for custom resources configuration.The JSDoc comments provide clear guidance, including code examples. The
canCreateRolecallback signature is flexible, supporting both sync and async implementations with clear semantics for each return value.
392-400: Schema extension for organizationResource is consistent with other schema definitions.The structure follows the established pattern used by
organizationRoleand other entities.packages/better-auth/src/plugins/organization/organization.ts (3)
90-96: Type definition for dynamic resource endpoints is well-structured.The pattern mirrors
DynamicAccessControlEndpointsand correctly usesReturnType<typeof ...>for type inference.
929-942: Runtime endpoint wiring is correct but consider consistency with dynamicAccessControl check.The resource endpoints are conditionally added when
enableCustomResourcesis true, but note this doesn't requiredynamicAccessControl.enabledto also be true. Verify this is intentional, as custom resources might be useful independently.Is it intentional that
enableCustomResourcescan be enabled withoutdynamicAccessControl.enabled? If custom resources require dynamic access control features, consider adding a runtime warning or enforcing the dependency.
1061-1108: Schema for organizationResource is complete and consistent.All necessary fields (organizationId, resource, permissions, createdAt, updatedAt) are defined with proper types, references, and indexing. The additionalFields extension follows the established pattern.
packages/better-auth/src/plugins/organization/load-resources.ts (1)
97-103: Custom resources override defaults with same name - verify this is intentional.The spread order means custom resources will shadow default resources with the same name. This could be a feature (allowing customization) or a security concern (allowing bypass of default resource constraints).
Is it intentional that custom resources can override default resource definitions? Consider documenting this behavior and any security implications.
packages/better-auth/src/plugins/organization/routes/crud-resources.ts (1)
670-700: Good defensive check: preventing deletion of resources in use.This check prevents orphaning role permissions by ensuring no roles reference the resource before deletion. The implementation correctly parses stored JSON permissions and filters roles.
| - Length limits? | ||
| - Reserved names? | ||
|
|
||
| Answer: add reasonable naming restrictions. and let me specify on the plugin config reseved names. |
There was a problem hiding this comment.
Fix spelling error in research document.
Line 769 contains a spelling error: "reseved" should be "reserved".
- Answer: add reasonable naming restrictions. and let me specify on the plugin config reseved names.
+ Answer: add reasonable naming restrictions. and let me specify on the plugin config reserved names.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Answer: add reasonable naming restrictions. and let me specify on the plugin config reseved names. | |
| Answer: add reasonable naming restrictions. and let me specify on the plugin config reserved names. |
🧰 Tools
🪛 LanguageTool
[grammar] ~769-~769: Ensure spelling is correct
Context: ...and let me specify on the plugin config reseved names. --- ## 13. Summary The organi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In ORGANIZATION_PLUGIN_RBAC_RESEARCH.md around line 769, fix the spelling
mistake "reseved" to "reserved" in the sentence "and let me specify on the
plugin config reseved names." so the line reads with correct spelling and
grammar (e.g., "reserved"); ensure surrounding punctuation and casing remain
consistent with the document style.
| /** | ||
| * In-memory cache for custom resources per organization | ||
| * Map<organizationId, Statements> | ||
| */ | ||
| const customResourcesCache = new Map<string, Statements>(); |
There was a problem hiding this comment.
In-memory cache has limitations in multi-instance deployments.
The customResourcesCache Map is process-local. In horizontally-scaled deployments, cache invalidation on one instance won't propagate to others, leading to stale data.
Consider documenting this limitation or offering an optional external cache adapter for production use.
Would you like me to help design a cache adapter interface that could support Redis or other distributed caches?
🤖 Prompt for AI Agents
In packages/better-auth/src/plugins/organization/load-resources.ts around lines
9 to 13, the current in-memory Map-based customResourcesCache is process-local
and will not invalidate across multiple instances; update the implementation to
be pluggable by introducing a CacheAdapter interface (get, set, delete, clear,
optionally ttl) and replace direct Map usage with calls to that adapter, provide
a default in-memory adapter that wraps the existing Map for local dev, and add a
note in the file README or comments describing the multi-instance limitation and
how to register a Redis (or other distributed) adapter for production.
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/better-auth/src/plugins/organization/load-resources.ts (1)
167-209: Case-insensitive comparison needed for reserved names check.The regex on line 180 allows uppercase letters, but the reserved names comparison on line 189 is case-sensitive. This allows bypassing reserved name protection by using different casing (e.g., "Organization" instead of "organization").
Apply this diff to normalize and enforce case-insensitivity:
export function validateResourceName( name: string, options: OrganizationOptions, ): { valid: boolean; error?: string } { + // Normalize to lowercase for consistent validation + const normalizedName = name.toLowerCase(); + // Length validation first - if (name.length < 1 || name.length > 50) { + if (normalizedName.length < 1 || normalizedName.length > 50) { return { valid: false, error: "Resource name must be between 1 and 50 characters", }; } // Basic format validation - if (!/^[a-zA-Z0-9_]+$/.test(name)) { + if (!/^[a-z0-9_]+$/.test(normalizedName)) { return { valid: false, - error: "Resource name must be alphanumeric with underscores only", + error: "Resource name must be lowercase alphanumeric with underscores only", }; } // Check reserved names const reservedNames = getReservedResourceNames(options); - if (reservedNames.includes(name)) { + if (reservedNames.map(n => n.toLowerCase()).includes(normalizedName)) { return { valid: false, - error: `Resource name "${name}" is reserved and cannot be used`, + error: `Resource name "${normalizedName}" is reserved and cannot be used`, }; } // Custom validation if provided if (options.dynamicAccessControl?.resourceNameValidation) { const customResult = - options.dynamicAccessControl.resourceNameValidation(name); + options.dynamicAccessControl.resourceNameValidation(normalizedName); if (typeof customResult === "boolean") { return customResult ? { valid: true } : { valid: false, error: "Resource name failed custom validation" }; } return customResult; } return { valid: true }; }Note: This also updates the JSDoc on line 162 to match the actual validation behavior.
🧹 Nitpick comments (1)
docs/content/docs/plugins/organization.mdx (1)
1772-2274: Documentation for Custom Resources looks comprehensive and well-structured.The documentation covers enabling custom resources, CRUD APIs, validation rules, configuration options, use cases, and best practices. A few minor observations:
- Line 1835: Consider simplifying "merged together" to just "merged" per the static analysis hint.
- Line 2151: Consider hyphenating "permission-checking" for consistency.
These are purely stylistic and don't affect functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/content/docs/plugins/organization.mdx(1 hunks)e2e/smoke/test/organization-custom-resources.test.ts(1 hunks)packages/better-auth/src/plugins/organization/error-codes.ts(1 hunks)packages/better-auth/src/plugins/organization/has-permission.ts(3 hunks)packages/better-auth/src/plugins/organization/load-resources.test.ts(1 hunks)packages/better-auth/src/plugins/organization/load-resources.ts(1 hunks)packages/better-auth/src/plugins/organization/organization.ts(6 hunks)packages/better-auth/src/plugins/organization/routes/crud-access-control.ts(8 hunks)packages/better-auth/src/plugins/organization/routes/crud-resources.ts(1 hunks)packages/better-auth/src/plugins/organization/schema.ts(4 hunks)packages/better-auth/src/plugins/organization/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/better-auth/src/plugins/organization/load-resources.test.ts
- packages/better-auth/src/plugins/organization/routes/crud-resources.ts
- packages/better-auth/src/plugins/organization/routes/crud-access-control.ts
🧰 Additional context used
🧬 Code graph analysis (4)
e2e/smoke/test/organization-custom-resources.test.ts (5)
packages/better-auth/src/plugins/access/access.ts (1)
createAccessControl(79-88)packages/better-auth/src/plugins/organization/access/statement.ts (2)
ownerAc(21-27)memberAc(29-35)packages/better-auth/src/plugins/admin/admin.ts (1)
admin(33-172)packages/better-auth/src/plugins/organization/organization.ts (1)
organization(440-1337)packages/better-auth/src/plugins/organization/client.ts (1)
organizationClient(81-279)
packages/better-auth/src/plugins/organization/schema.ts (2)
packages/core/src/db/index.ts (1)
BetterAuthPluginDBSchema(10-10)packages/core/src/db/plugin.ts (1)
BetterAuthPluginDBSchema(3-11)
packages/better-auth/src/plugins/organization/types.ts (2)
packages/better-auth/src/plugins/organization/schema.ts (2)
Member(390-390)OrganizationResource(400-400)packages/core/src/db/type.ts (1)
DBFieldAttribute(127-129)
packages/better-auth/src/plugins/organization/has-permission.ts (2)
packages/better-auth/src/plugins/organization/load-resources.ts (1)
getOrganizationAccessControl(114-125)packages/better-auth/src/plugins/access/access.ts (1)
role(8-77)
🪛 LanguageTool
docs/content/docs/plugins/organization.mdx
[style] ~1835-~1835: ‘merged together’ might be wordy. Consider a shorter alternative.
Context: ..., both default and custom resources are merged together. If a custom resource has the same name...
(EN_WORDINESS_PREMIUM_MERGED_TOGETHER)
[grammar] ~2151-~2151: Use a hyphen to join words.
Context: ...c You can extend the default permission checking logic for role creation by prov...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (18)
packages/better-auth/src/plugins/organization/has-permission.ts (2)
36-43: LGTM! Organization-specific AC retrieval logic is correct.The conditional logic properly branches to use
getOrganizationAccessControlwhenenableCustomResourcesis true, falling back to the staticinput.options.acotherwise. This ensures custom resources are merged with defaults when the feature is enabled.
78-79: Role creation correctly uses organization-specific AC.Using
orgAc.newRole(result.data)ensures that roles are created with the merged statements (default + custom resources) when custom resources are enabled.packages/better-auth/src/plugins/organization/error-codes.ts (1)
91-108: Error codes are comprehensive and well-structured.The new error codes cover all resource-related operations (CRUD permissions) and constraints (limits, naming validation, usage checks). The naming convention and message formats are consistent with existing error codes.
packages/better-auth/src/plugins/organization/schema.ts (3)
51-77: OrganizationResourceDefaultFields interface is well-defined.The interface follows the same pattern as
OrganizationRoleDefaultFieldswith appropriate field types and references. Thepermissionsfield correctly usestype: "string"for database storage (will be JSON stringified).
238-254: Schema type correctly includes organizationResource conditionally.The conditional type properly nests
organizationResourcewithin thedynamicAccessControl.enabled: truebranch, and only whenenableCustomResourcesis also true. The intersection types are used correctly to compose the schema.
380-387: Zod schema correctly handles permissions as array.The
organizationResourceSchemausesz.array(z.string())forpermissionswhich correctly represents the parsed form, while the database field stores it as a JSON string. This mirrors the pattern used inorganizationRoleSchemawherepermissionisz.record(...).e2e/smoke/test/organization-custom-resources.test.ts (4)
14-65: Test setup is well-configured.The test properly configures the organization plugin with dynamic access control and custom resources enabled, including a maximum of 10 resources per organization for limit testing. The client is configured to match.
85-175: CRUD operation tests are thorough.The tests properly verify:
- Resource creation with correct fields returned
- Listing includes both default (protected) and custom resources with correct metadata flags
- Getting specific resources returns expected data
- Updating permissions works correctly
196-234: Auto-creation test validates an important feature.This test verifies that when creating a role with a non-existent resource, the resource is automatically created with the permissions specified in the role. This is consistent with the documentation for "Auto-Creating Resources with Roles".
410-453: Permission test validates member restrictions correctly.The test properly verifies that a member (who lacks
ac:createpermission permemberAcstatements) cannot create resources. The test flow is correct: sign in new user, add as member, set active organization, then attempt unauthorized operation.packages/better-auth/src/plugins/organization/types.ts (3)
15-34: LGTM! Well-designed authorization result type.The discriminated union provides type-safe authorization results with clear intent. The documentation appropriately warns about the
bypassflag.
109-184: LGTM! Comprehensive custom resource configuration options.The new fields follow existing conventions and provide flexible configuration with good defaults. Documentation includes helpful examples.
417-425: LGTM! Schema definition follows established patterns.The
organizationResourceschema is consistent with other model definitions in the codebase.packages/better-auth/src/plugins/organization/organization.ts (5)
49-96: LGTM! Resource endpoints follow established patterns.The import and type definitions for dynamic resource endpoints are consistent with the existing
DynamicAccessControlEndpointspattern.
280-283: LGTM! Type composition correctly conditionally includes resource endpoints.The conditional type logic properly includes
DynamicResourceEndpointsonly whenenableCustomResourcesis true.
930-942: LGTM! Runtime endpoint wiring is correct.The dynamic resource endpoints are properly wired and conditionally included when
enableCustomResourcesis enabled, following the same pattern as dynamic access control endpoints.
1061-1109: LGTM! Schema definition is well-structured.The
organizationResourceSchemafollows the same pattern asorganizationRoleSchemaand properly defines all required fields with appropriate types and references.
1149-1149: LGTM! Schema merge follows established patterns.The
organizationResourceSchemais correctly spread into the main schema object, consistent with how other conditional schemas are merged.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts (1)
1116-1181: Mixing new resources and new actions on existing custom resources only partially auto-handled
checkForInvalidResourcescurrently chooses eitherautoCreateMissingResources(whenmissingResources.length > 0) orvalidateAndExpandPermissions(whenmissingResources.length === 0):
- If a request introduces brand‑new resources, it goes down
autoCreateMissingResources.- In that path, existing resources in the same
permissionobject are only validated against the refreshedupdatedStatements; they are not candidates for the auto‑expansion behavior implemented invalidateAndExpandPermissions.- So a single request that both (a) creates new custom resources and (b) adds new actions to already‑existing custom resources will see the new resources auto‑created, but will still get
BAD_REQUESTfor the new actions on the existing resources instead of auto‑expanding them.If the intended behavior is that “custom resources with
enableCustomResourcescan have their valid actions auto‑expanded even when mixed with new resources in the same payload”, thencheckForInvalidResourcesshould probably run both steps (e.g., callautoCreateMissingResourcesfirst, thenvalidateAndExpandPermissionswith refreshed statements), rather than an exclusive either/or. Otherwise, this limitation is worth calling out in docs.
♻️ Duplicate comments (1)
packages/better-auth/src/plugins/organization/routes/crud-resources.ts (1)
682-685: GuardJSON.parseon stored permission blobs to avoid crashing requestsAt several points you parse JSON coming from persisted fields:
- Line 683:
const permissions = JSON.parse(role.permission);- Line 857:
permissions: JSON.parse(r.permissions) as string[],- Line 1030:
permissions: JSON.parse(customResource.permissions as never as string),If any
permission(s)string in the database is malformed, theseJSON.parsecalls will throw and abort the whole request (delete/list/get), returning a 500 instead of a controlled error. This is especially brittle given these values can be mutated over time.Consider wrapping these in a small safe‑parse helper or local try/catch that:
- Logs a warning with organizationId and role/resource identifier.
- Treats invalid JSON as “no permissions” / skips that record, or converts it into a well‑defined API error.
For example, for the delete path:
- const rolesWithResource = rolesUsingResource.filter((role) => { - const permissions = JSON.parse(role.permission); - return resourceName in permissions; - }); + const rolesWithResource = rolesUsingResource.filter((role) => { + try { + const permissions = JSON.parse(role.permission); + return resourceName in permissions; + } catch { + ctx.context.logger.warn( + `[Dynamic Resources] Invalid JSON in role permission for role ${role.role}`, + { organizationId, role: role.role }, + ); + return false; + } + });Then apply the same pattern to the JSON.parse calls in
listOrgResourcesandgetOrgResourceso a single bad row cannot take down those endpoints.Also applies to: 855-858, 1029-1031
🧹 Nitpick comments (7)
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts (4)
1039-1056: Update flow mirrors create flow; consider future symmetry hookThe permission update path (
updateOrgRole) mirrors the create path: it runscheckForInvalidResourcesfirst and then callscheckIfMemberHasPermission, again skipping delegation checks for newly auto-created resources. This keeps behavior consistent across create and update.If, in the future, you need more granular control over updates (similar to
canCreateRole), this would be the natural place to introduce adynamicAccessControl.canUpdateRolehook, but that’s not required for this PR.
1183-1330: Auto‑creation pipeline is robust; consider tightening a couple of edge behaviorsThe
autoCreateMissingResourcesflow is generally solid:
- Validates all
missingResourcesnames viavalidateResourceNamebefore touching the DB.- De‑duplicates creation via
findOnechecks and parallelcreatecalls.- Invalidates the resource cache only when
resourcesToCreate.length > 0.- Reloads organization statements and then re‑validates that all requested permissions exist for every resource, throwing if anything is inconsistent.
Two small edge‑case improvements you might consider:
Existing custom resources with no statements backing them
After reload, ifupdatedStatements[resource]is still falsy for some resource, you treat it as an internal error (500) and fail. That’s good. However, this condition means “we couldn’t wire this resource into statements even after ensuring a DB row exists”; if this ever happens, you may want to logorganizationIdand the raw DB row for easier debugging rather than justresource.Reuse validation logic with
validateAndExpandPermissions
The “invalid permissions for existing resources” logic here is slightly different from whatvalidateAndExpandPermissionsdoes (there it can auto‑expand custom resources). If you later adjustcheckForInvalidResourcesto allow combined create+expand flows, it might be worth centralizing the “invalid permission” decision in one place to avoid divergence.Both are non‑blocking and mostly about diagnosability and future maintainability.
1332-1501: Handle impossible custom-resource state explicitly invalidateAndExpandPermissions
validateAndExpandPermissionscorrectly:
- Restricts auto‑expansion of permissions to custom resources when
enableCustomResourcesis on.- Updates
OrganizationResource.permissionsin parallel and invalidates the cache afterward.One subtle edge case: for any entry in
resourcesToExpand, you logically expect anOrganizationResourcerow to exist for that(organizationId, resource)pair (since it’s treated as a custom resource coming from org‑specific statements). IffindOnereturnsnull, the code currently justreturns from that iteration, silently skipping the expansion for that resource.This “custom resource present in statements but missing in DB” state is effectively impossible under the normal invariants; if it ever occurs, it’s a data‑integrity problem. Instead of silently ignoring it, consider:
- Logging an error with
organizationIdandresource, and/or- Throwing a 5xx so the caller sees a clear failure rather than silently not getting the expanded permissions.
That would make underlying data issues much easier to detect.
1511-1539: Delegation skipping for auto-created resources is reasonable; minor perf nit is optionalThe new
skipResourcesForDelegationCheckparameter incheckIfMemberHasPermissioncleanly avoids delegation checks for resources that were just auto-created in this operation, while still enforcing full delegation checks for all other resources. The logging also makes skipped resources auditable.If you ever expect large permission maps or skip lists, you could micro‑optimize:
- skipResourcesForDelegationCheck = [], + skipResourcesForDelegationCheck = [], @@ - for await (const [resource, permissions] of permissionEntries) { + const skipSet = new Set(skipResourcesForDelegationCheck); + for await (const [resource, permissions] of permissionEntries) { @@ - if (skipResourcesForDelegationCheck.includes(resource)) { + if (skipSet.has(resource)) {This is purely optional; for typical small maps the current implementation is fine.
packages/better-auth/src/plugins/organization/routes/crud-resources.ts (3)
86-100: AlignadditionalFieldsruntime schema with$InfertypingIn both create and update endpoints the Zod schema makes
additionalFieldsoptional:additionalFields: z.object({ ...additionalFieldsSchema.shape }).optional(),But your
$Infermetadata treats it as:
- Optional only when
AdditionalFieldsis exactly{}.- Required (
additionalFields: AdditionalFields) when there are configured fields.That gives a mismatch where TypeScript clients are told the field is required but the runtime validator allows it to be omitted. Consider choosing one contract and aligning both sides:
- If you want it required when non-empty, drop
.optional()from the Zod object in that case.- If you want it always optional, change the
$Inferunion to makeadditionalFieldsoptional regardless ofIsExactlyEmptyObject<AdditionalFields>.Right now this can lead to surprising compile‑time vs runtime behavior for consumers of these endpoints.
Also applies to: 329-345
209-216: Confirm whether top‑levelmaximumResourcesPerOrganizationshould be honoredHere you only read
options.dynamicAccessControl?.maximumResourcesPerOrganization(function or number), otherwise you fall back to the hardcoded default:const maximumResourcesPerOrganization = typeof options.dynamicAccessControl?.maximumResourcesPerOrganization === "function" ? await options.dynamicAccessControl.maximumResourcesPerOrganization( organizationId, ) : (options.dynamicAccessControl?.maximumResourcesPerOrganization ?? DEFAULT_MAXIMUM_RESOURCES_PER_ORGANIZATION);
OrganizationOptionsalso mentions a top‑levelmaximumResourcesPerOrganizationdefault in addition to the dynamicAccessControl field (seetypes.ts). If that top‑level option is meant to act as a fallback, it’s currently ignored here.If the intent is to support both, consider:
- : (options.dynamicAccessControl?.maximumResourcesPerOrganization ?? - DEFAULT_MAXIMUM_RESOURCES_PER_ORGANIZATION); + : (options.dynamicAccessControl?.maximumResourcesPerOrganization ?? + options.maximumResourcesPerOrganization ?? + DEFAULT_MAXIMUM_RESOURCES_PER_ORGANIZATION);(Adjust property path to whatever the canonical top‑level config is.)
If restricting this to dynamicAccessControl only is intentional, a short comment would help future readers.
124-181: Factor out shared membership + permission checks to reduce duplicationEach endpoint repeats very similar logic:
- Resolve
organizationId(body/query vssession.activeOrganizationId), 400 if missing.- Load
Memberfor{ organizationId, userId }, 403 if not found.- Call
hasPermissionwith{ organizationId, role: member.role, permissions: { ac: [...] } }, 403 if false.- Log consistent error messages.
The duplication across create/update/delete/list/get makes it easy for behavior or logging to drift between endpoints over time.
Consider extracting small helpers, e.g.:
ensureActiveOrganizationId(ctx, actionDescription): stringgetMemberOrThrow(ctx, organizationId, actionDescription): MemberassertHasPermission(ctx, { organizationId, member, acPermissions, actionDescription }): Promise<void>and reuse them in each handler. That would tighten the surface area for future fixes (including error code changes) and improve readability.
Also applies to: 362-419, 562-619, 772-829, 917-974
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts(8 hunks)packages/better-auth/src/plugins/organization/routes/crud-resources.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/better-auth/src/plugins/organization/routes/crud-resources.ts (3)
packages/better-auth/src/plugins/organization/types.ts (1)
OrganizationOptions(36-930)packages/better-auth/src/plugins/organization/schema.ts (3)
Member(390-390)OrganizationResource(400-400)OrganizationRole(399-399)packages/better-auth/src/plugins/organization/load-resources.ts (3)
validateResourceName(167-209)invalidateResourceCache(130-132)getReservedResourceNames(151-158)
🔇 Additional comments (4)
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts (3)
9-18: New imports align with dynamic access-control additionsThe added type-only imports for
AccessControl,Statements, and the organization entities, plus theload-resourceshelpers, are consistent with the new resource-validation and cache-invalidation logic below. Nothing to change here.
181-249: canCreateRole hook layering and bypass semantics look correctThe
dynamicAccessControl.canCreateRolehook is wired in cleanly: you (1) requireresult.allowbefore proceeding, (2) optionally setbypassDefaultCheckswhenresult.bypassis true, and (3) still always run resource validation later. This ensures a custom hook can only further restrict or relax the default permission/delegation checks without skipping core validation. No issues from a correctness standpoint.
284-305: Resource validation + delegation check ordering increateOrgRoleis soundCalling
checkForInvalidResourcesbeforecheckIfMemberHasPermissionensures that:
- Invalid or disallowed resources/permissions fail fast with a 4xx instead of a generic permission denial.
- Any auto-created resources are returned and then excluded via
skipResourcesForDelegationCheck, so delegation checks don’t immediately fail on resources that didn’t exist before this call.The separation between validation (including auto-create) and delegation checks is clear and matches the intent of dynamic org resources.
packages/better-auth/src/plugins/organization/routes/crud-resources.ts (1)
74-302: Overall CRUD flow, validation, and error handling look solidThe endpoint set is cohesive and consistent:
- Good validation on org context, membership, and action‑level permissions.
- Proper use of
validateResourceName, reserved names, and max‑resource enforcement.- Cache invalidation is wired after create/update/delete.
- Responses clearly distinguish default AC resources from custom ones.
Aside from the JSON.parse robustness and config/typing nits already called out, the overall design and control flow look well‑structured.
Also applies to: 317-517, 529-728, 739-871, 883-1042
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/app/enterprise/_components/enterprise-hero.tsx (1)
69-77: Fix non-standard SVG namespace on Google Labs logo
xmlnsis set tohttps://www.w3.org/2000/svg, but the SVG namespace should behttp://www.w3.org/2000/svg. Browsers are forgiving, but it’s better to use the spec-compliant value:- <svg - xmlns="https://www.w3.org/2000/svg" + <svg + xmlns="http://www.w3.org/2000/svg" width="1.3em" viewBox="0 0 18 18" fill="currentColor" >
🧹 Nitpick comments (20)
packages/better-auth/src/context/create-context.ts (1)
173-177: Defaulting logic correctly aligns storeStateStrategy with database presenceThe new fallback to
"database"only whenoptions.databaseis truthy and"cookie"otherwise is consistent with the stateless defaults set earlier increateAuthContext, and preserves behavior for both DB-backed and stateless setups while making the intent clearer.If you want to be extra explicit about intent and avoid any future issues with falsy-but-valid values, you could switch from
||to nullish coalescing:- storeStateStrategy: - options.account?.storeStateStrategy || - (options.database ? "database" : "cookie"), + storeStateStrategy: + options.account?.storeStateStrategy ?? + (options.database ? "database" : "cookie"),But this is optional; the current change looks good as-is.
docs/components/nav-mobile.tsx (1)
62-64: Consider simplifying the onClick handler.The arrow function wrapper is unnecessary here since you're just calling
toggleNavbarwithout any additional logic.Apply this diff:
- onClick={() => { - toggleNavbar(); - }} + onClick={toggleNavbar}packages/core/src/social-providers/kick.ts (1)
60-71: DefaultrefreshAccessTokenwiring looks correct; consider small cleanupsThe fallback correctly delegates to the shared OAuth2
refreshAccessTokenwith the Kick token endpoint and client credentials, while still allowing an override viaoptions.refreshAccessToken, which fits the/refresh-tokenpipeline expectations. You could optionally:
- Use nullish coalescing for brevity:
refreshAccessToken: options.refreshAccessToken ?? ((refreshToken) => refreshAccessToken({ ... })).- Drop the redundant
async/returnin the fallback arrow and just return the promise fromrefreshAccessToken.
If Kick always requires aclientSecret, you might also consider surfacing that explicitly onKickOptionsfor clearer typing.docs/app/enterprise/_components/enterprise-hero.tsx (3)
14-16: Hero title highlight looks good; optional padding for visual balanceThe inverted
bg-foreground text-backgroundtreatment on “BETTER AUTH.” works nicely. If you want the highlight to read more like a pill/tag, consider adding small horizontal padding (e.g.px-1/px-2) so the background doesn’t hug the glyphs quite as tightly.
32-187: Add accessible names to logo links in the Trusted-by sectionEach logo link is currently just an
<svg>inside an<a>; the tooltip label (“OpenAI”, “Google Labs”, etc.) won’t reliably be announced by screen readers. To improve a11y, addaria-label(or visually hidden text) on the<a>elements, e.g.:- <a - href="https://openai.com" + <a + href="https://openai.com" + aria-label="OpenAI" target="_blank" rel="noopener noreferrer" className="inline-block cursor-pointer hover:opacity-100 transition-opacity" >and repeat for the other brands.
150-152: Deel logo size change is fine; consider single-dimension sizing (optional)Reducing the logo height to
1.3embrings its vertical size in line with the other logos while keeping a wider width. If you spot distortion or wrapping in tighter layouts, consider specifying onlyheight="1.3em"(droppingwidth) to let the browser preserve the intrinsic aspect ratio automatically.packages/better-auth/src/oauth2/state.ts (1)
178-178: Inconsistent optional chaining usage.Line 178 still uses optional chaining (
c.context.oauthConfig?.skipStateCookieCheck) while lines 30 and 101 now omit it. If the removal on lines 30/101 was intentional becauseoauthConfigis now guaranteed to exist, this line should be updated for consistency. Otherwise, the changes on lines 30/101 need the optional chaining restored.packages/better-auth/src/plugins/username/username.test.ts (1)
471-471: Remove unused variable.The
cookieSetteris destructured but never used in any test within this describe block.Apply this diff to remove the unused variable:
- const { client, cookieSetter } = await getTestInstance( + const { client } = await getTestInstance(packages/better-auth/src/plugins/magic-link/index.ts (1)
320-323: LGTM! Consider standardizing error code naming conventions.The
redirectWithErrorhelper effectively centralizes error redirection logic and eliminates duplication across five call sites. Theneverreturn type correctly signals that the function always throws.However, error codes show inconsistent naming: uppercase format ("INVALID_TOKEN", "EXPIRED_TOKEN") versus lowercase with underscores ("failed_to_create_user", "new_user_signup_disabled", "failed_to_create_session"). Standardizing to a single convention (e.g., all uppercase or all lowercase snake_case) would improve maintainability.
Example standardization to uppercase:
function redirectWithError(error: string): never { errorCallbackURL.searchParams.set("error", error); throw ctx.redirect(errorCallbackURL.toString()); }Then update the call sites:
redirectWithError("FAILED_TO_CREATE_USER"); redirectWithError("NEW_USER_SIGNUP_DISABLED"); redirectWithError("FAILED_TO_CREATE_SESSION");packages/better-auth/src/plugins/two-factor/two-factor.test.ts (1)
349-356: Consider removing the misleadingonSuccesscallback.The test expects an error response (line 358 checks for "Invalid backup code"), so the
onSuccesscallback will never execute. Having it present is misleading and suggests the test expects both success and failure.Apply this diff to remove the unused callback:
const res = await client.twoFactor.verifyBackupCode({ code: "invalid-code", fetchOptions: { headers: headers2, - onSuccess(context) { - const parsed = parseSetCookieHeader( - context.response.headers.get("Set-Cookie") || "", - ); - const token = parsed.get("better-auth.session_token")?.value; - expect(token?.length).toBeGreaterThan(0); - }, }, });packages/core/src/social-providers/google.ts (1)
176-200: Consider caching Google's public keys.Fetching certs from Google on every token verification adds latency and unnecessary network calls. Google's public keys rotate infrequently and include cache-control headers (typically
max-age=19671or similar).Also, the error types are inconsistent:
APIErroron line 189 vsErroron line 196.Consider implementing a simple in-memory cache with TTL:
+let cachedKeys: { keys: Array<{ kid: string; alg: string; kty: string; use: string; n: string; e: string }>; expiresAt: number } | null = null; + export const getGooglePublicKey = async (kid: string) => { + const now = Date.now(); + if (!cachedKeys || cachedKeys.expiresAt < now) { const { data } = await betterFetch<{ keys: Array<{ kid: string; alg: string; kty: string; use: string; n: string; e: string; }>; }>("https://www.googleapis.com/oauth2/v3/certs"); if (!data?.keys) { - throw new APIError("BAD_REQUEST", { - message: "Keys not found", - }); + throw new Error("Google public keys not found"); } + cachedKeys = { keys: data.keys, expiresAt: now + 3600 * 1000 }; // 1 hour TTL + } - const jwk = data.keys.find((key) => key.kid === kid); + const jwk = cachedKeys.keys.find((key) => key.kid === kid); if (!jwk) { throw new Error(`JWK with kid ${kid} not found`); } return await importJWK(jwk, jwk.alg); };packages/better-auth/src/db/get-migration.ts (1)
281-395: Provider-basedtypeMapfor arrays looks good; alignmatchTypeto avoid false mismatchesThe refactor to a provider-scoped
typeMap(including"string[]"and"number[]") is solid and keeps id/foreignKey handling consistent across dialects.One follow-up:
matchTypestill treats array fields as “must be JSON-typed”:if (fieldType === "string[]" || fieldType === "number[]") { return columnDataType.toLowerCase().includes("json"); }But
getTypenow generates:
- sqlite:
"text"for"string[]"/"number[]"- mssql:
"varchar(8000)"for arraysOn re-introspection, sqlite will report
TEXTand mssqlvarchar(8000), which won’t contain"json", so existing array columns you just created will look “mismatched” and trigger warnings every run.You can reuse the existing
map’sjsonentries to makematchTypeprovider-aware for arrays and avoid those false positives:function matchType( columnDataType: string, fieldType: DBFieldType, dbType: KyselyDatabaseType, ) { function normalize(type: string) { return type.toLowerCase().split("(")[0]!.trim(); } - if (fieldType === "string[]" || fieldType === "number[]") { - return columnDataType.toLowerCase().includes("json"); - } + if (fieldType === "string[]" || fieldType === "number[]") { + const types = map[dbType]!; + const expectedJsonTypes = types.json.map((t) => t.toLowerCase()); + return expectedJsonTypes.includes(normalize(columnDataType)); + } const types = map[dbType]!; const expected = Array.isArray(fieldType) ? types["string"].map((t) => t.toLowerCase()) : types[fieldType]!.map((t) => t.toLowerCase()); return expected.includes(normalize(columnDataType)); }This keeps the creation and introspection logic in sync for all providers, including sqlite and mssql.
As a minor optional nit:
typeMapinsidegetTypeis static and could be hoisted to module scope to avoid recreating it per field, but that’s more of a micro-optimization/readability tweak than a blocker.Also applies to: 77-93
docs/components/theme-toggle.tsx (1)
10-13: Consider removing the redundant themeMap.The
themeMapconstant has keys that match values exactly. You can simplify by using string literals directly ("light","dark") throughout the code.Apply this diff to simplify:
-const themeMap = { - light: "light", - dark: "dark", -} as const; - function renderThemeIcon(theme: string | undefined) { switch (theme) { - case themeMap.light: + case "light": return <LightThemeIcon />; - case themeMap.dark: + case "dark": return <DarkThemeIcon />; default: return null; } }And update the toggle logic:
onClick={() => { setTheme( - resolvedTheme === themeMap.dark ? themeMap.light : themeMap.dark, + resolvedTheme === "dark" ? "light" : "dark", ); }}packages/better-auth/src/plugins/two-factor/verify-two-factor.ts (1)
63-74: Optional: Consider PR scope.These two-factor authentication cleanup improvements are valuable, but they appear unrelated to the PR's stated objective of "dynamic org resources." While the enriched summary mentions this aligns with a broader cleanup pattern, consider whether such changes might be better suited for a separate PR focused on security improvements or session management enhancements.
packages/better-auth/src/plugins/admin/routes.ts (1)
1008-1036: Impersonation guard is correct, but confirmadminRolesdefaults are applied consistentlyThe new guard correctly:
- Loads the target user,
- Normalizes
adminRolesfromopts.adminRoles,- Derives the target’s roles (including comma‑separated multi‑roles), and
- Blocks impersonation of admins/adminUserIds unless
allowImpersonatingAdmins === true.One thing to double‑check: if
opts.adminRolesis ever left undefined (i.e., defaults aren’t merged intooptsbefore this route), then:const adminRoles = ( Array.isArray(opts.adminRoles) ? opts.adminRoles : opts.adminRoles?.split(",") || [] ).map((role) => role.trim());yields
[], so the guard will only protectadminUserIds, not roles like"admin", even though the docs say the default admin role is"admin".If defaults aren’t already baked into
opts, consider aligning this with the same defaulting logic you use inhasPermission, e.g.:const rawAdminRoles = Array.isArray(opts.adminRoles) ? opts.adminRoles : opts.adminRoles?.split(",") ?? ["admin"]; const adminRoles = rawAdminRoles.map((role) => role.trim());That keeps the impersonation protection consistent with whatever is treated as an “admin” elsewhere.
packages/better-auth/src/adapters/tests/basic.ts (1)
2902-2986: Array/JSON adapter tests are solid; consider dropping the debug logThe new
"create - should support arrays"and"create - should support json"tests nicely cover round-trippingstring[]/number[]andjsonfields through the adapter. They’ll help catch regressions in the newsupportsArrays/JSON behavior.Only minor nit:
console.log(findResult);in the json test will spam test output; it can be safely removed once debugging is done..github/workflows/ci.yml (1)
162-166: Coverage runs tests but artifacts aren't persisted.The switch to
pnpm coverageensures tests run with coverage instrumentation. However, the coverage results aren't uploaded or reported anywhere. Consider adding a step to upload coverage artifacts or integrate with a coverage service (e.g., Codecov, Coveralls) if tracking coverage trends is desired.packages/better-auth/src/oauth2/link-account.test.ts (1)
256-256: Consider clarifying the test name.The test name "should deny account linking when provider is not trusted and email is not verified" could be clearer. It's testing that the provider's
email_verifiedclaim is false (line 275), not the user'semailVerifiedfield (which is true, line 265). Consider:- it("should deny account linking when provider is not trusted and email is not verified", async () => { + it("should deny account linking when provider is not trusted and provider reports email as unverified", async () => {docs/content/docs/plugins/2fa.mdx (1)
359-359: Docs wording and hyphenation nitThe new sentences read clearly. Tiny style nit: at line 438, consider “two‑factor authentication data” (with a hyphen) to match “Two‑Factor Authentication (2FA)” above and the linter hint.
Also applies to: 387-387, 438-438
docs/content/docs/plugins/sso.mdx (1)
710-713: Domain-verified linking explanation looks goodThe added paragraph accurately reflects the behavior where a domain‑verified provider plus matching email domain enables automatic account linking. Optionally, you could reference the
accountLinkingsection below so readers see how domain‑verified trust fits into the broader linking configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/better-auth/src/context/__snapshots__/create-context.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (72)
.cspell/custom-words.txt(1 hunks).cspell/third-party.txt(1 hunks).github/workflows/ci.yml(1 hunks)biome.json(1 hunks)context7.json(1 hunks)docs/app/enterprise/_components/enterprise-form.tsx(2 hunks)docs/app/enterprise/_components/enterprise-hero.tsx(4 hunks)docs/components/mobile-search-icon.tsx(1 hunks)docs/components/nav-bar.tsx(1 hunks)docs/components/nav-mobile.tsx(1 hunks)docs/components/sidebar-content.tsx(3 hunks)docs/components/theme-toggle.tsx(1 hunks)docs/components/theme-toggler.tsx(0 hunks)docs/content/docs/guides/workos-migration-guide.mdx(1 hunks)docs/content/docs/plugins/2fa.mdx(3 hunks)docs/content/docs/plugins/admin.mdx(1 hunks)docs/content/docs/plugins/community-plugins.mdx(1 hunks)docs/content/docs/plugins/creem.mdx(1 hunks)docs/content/docs/plugins/last-login-method.mdx(1 hunks)docs/content/docs/plugins/organization.mdx(2 hunks)docs/content/docs/plugins/sso.mdx(1 hunks)package.json(1 hunks)packages/better-auth/package.json(2 hunks)packages/better-auth/src/adapters/drizzle-adapter/drizzle-adapter.ts(1 hunks)packages/better-auth/src/adapters/kysely-adapter/kysely-adapter.ts(3 hunks)packages/better-auth/src/adapters/prisma-adapter/prisma-adapter.ts(2 hunks)packages/better-auth/src/adapters/tests/basic.ts(1 hunks)packages/better-auth/src/context/create-context.ts(1 hunks)packages/better-auth/src/cookies/index.ts(2 hunks)packages/better-auth/src/cookies/session-store.ts(1 hunks)packages/better-auth/src/db/get-migration.ts(4 hunks)packages/better-auth/src/oauth2/link-account.test.ts(1 hunks)packages/better-auth/src/oauth2/link-account.ts(2 hunks)packages/better-auth/src/oauth2/state.ts(2 hunks)packages/better-auth/src/plugins/admin/admin.test.ts(1 hunks)packages/better-auth/src/plugins/admin/error-codes.ts(1 hunks)packages/better-auth/src/plugins/admin/routes.ts(1 hunks)packages/better-auth/src/plugins/admin/types.ts(1 hunks)packages/better-auth/src/plugins/magic-link/index.ts(3 hunks)packages/better-auth/src/plugins/magic-link/magic-link.test.ts(1 hunks)packages/better-auth/src/plugins/organization/load-resources.ts(1 hunks)packages/better-auth/src/plugins/organization/organization.ts(7 hunks)packages/better-auth/src/plugins/organization/routes/crud-access-control.ts(8 hunks)packages/better-auth/src/plugins/organization/routes/crud-resources.ts(1 hunks)packages/better-auth/src/plugins/two-factor/two-factor.test.ts(1 hunks)packages/better-auth/src/plugins/two-factor/verify-two-factor.ts(1 hunks)packages/better-auth/src/plugins/username/index.ts(2 hunks)packages/better-auth/src/plugins/username/username.test.ts(2 hunks)packages/cli/package.json(2 hunks)packages/cli/src/generators/drizzle.ts(1 hunks)packages/cli/src/generators/prisma.ts(2 hunks)packages/core/package.json(2 hunks)packages/core/src/db/adapter/factory.ts(3 hunks)packages/core/src/db/adapter/index.ts(1 hunks)packages/core/src/social-providers/google.ts(3 hunks)packages/core/src/social-providers/kick.ts(2 hunks)packages/expo/package.json(4 hunks)packages/expo/src/plugins/index.ts(1 hunks)packages/expo/src/plugins/last-login-method.ts(1 hunks)packages/expo/test/last-login-method.test.ts(1 hunks)packages/expo/tsdown.config.ts(1 hunks)packages/passkey/package.json(2 hunks)packages/scim/package.json(2 hunks)packages/sso/package.json(2 hunks)packages/sso/src/oidc.test.ts(1 hunks)packages/sso/src/routes/sso.ts(2 hunks)packages/sso/src/types.ts(1 hunks)packages/stripe/package.json(2 hunks)packages/telemetry/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)test/package.json(2 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (1)
- docs/components/theme-toggler.tsx
✅ Files skipped from review due to trivial changes (6)
- docs/content/docs/plugins/creem.mdx
- packages/telemetry/package.json
- context7.json
- docs/content/docs/plugins/last-login-method.mdx
- packages/sso/src/types.ts
- docs/components/sidebar-content.tsx
🧰 Additional context used
🧬 Code graph analysis (15)
packages/better-auth/src/plugins/magic-link/index.ts (1)
packages/better-auth/src/api/routes/error.ts (1)
error(373-442)
packages/better-auth/src/oauth2/link-account.test.ts (2)
packages/better-auth/src/plugins/jwt/sign.ts (1)
signJWT(64-144)packages/better-auth/src/utils/constants.ts (1)
DEFAULT_SECRET(1-1)
packages/sso/src/oidc.test.ts (3)
packages/better-auth/src/test-utils/test-instance.ts (1)
getTestInstance(31-325)packages/sso/src/index.ts (1)
sso(73-148)packages/sso/src/client.ts (1)
ssoClient(12-25)
packages/core/src/social-providers/kick.ts (1)
packages/better-auth/src/api/routes/account.ts (1)
refreshToken(608-791)
packages/better-auth/src/plugins/two-factor/verify-two-factor.ts (1)
packages/better-auth/src/cookies/index.ts (1)
setSessionCookie(235-294)
packages/better-auth/src/plugins/organization/routes/crud-resources.ts (4)
packages/better-auth/src/plugins/organization/types.ts (1)
OrganizationOptions(36-930)packages/better-auth/src/plugins/organization/error-codes.ts (1)
ORGANIZATION_ERROR_CODES(3-109)packages/better-auth/src/plugins/organization/schema.ts (3)
Member(390-390)OrganizationResource(400-400)OrganizationRole(399-399)packages/better-auth/src/plugins/organization/load-resources.ts (3)
validateResourceName(167-209)invalidateResourceCache(130-132)getReservedResourceNames(151-158)
packages/better-auth/src/context/create-context.ts (1)
packages/better-auth/src/plugins/oidc-provider/index.ts (1)
options(1766-1768)
packages/core/src/social-providers/google.ts (1)
packages/better-auth/src/index.ts (1)
APIError(31-31)
packages/better-auth/src/plugins/two-factor/two-factor.test.ts (1)
packages/better-auth/src/cookies/cookie-utils.ts (1)
parseSetCookieHeader(13-75)
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts (7)
packages/better-auth/src/api/index.ts (1)
APIError(372-372)packages/better-auth/src/index.ts (1)
APIError(31-31)packages/better-auth/src/plugins/organization/error-codes.ts (1)
ORGANIZATION_ERROR_CODES(3-109)packages/better-auth/src/plugins/organization/has-permission.ts (1)
hasPermission(11-89)packages/better-auth/src/plugins/organization/types.ts (1)
OrganizationOptions(36-930)packages/better-auth/src/plugins/organization/load-resources.ts (2)
getOrganizationStatements(80-108)invalidateResourceCache(130-132)packages/better-auth/src/plugins/organization/schema.ts (1)
OrganizationResource(400-400)
packages/better-auth/src/plugins/admin/routes.ts (2)
packages/better-auth/src/plugins/admin/types.ts (1)
UserWithRole(5-10)packages/better-auth/src/plugins/admin/error-codes.ts (1)
ADMIN_ERROR_CODES(4-32)
packages/better-auth/src/oauth2/link-account.ts (1)
e2e/smoke/test/fixtures/cloudflare/src/auth-schema.ts (1)
account(26-46)
packages/better-auth/src/cookies/index.ts (1)
packages/better-auth/src/cookies/session-store.ts (1)
createAccountStore(232-232)
packages/better-auth/src/plugins/organization/load-resources.ts (5)
packages/better-auth/src/plugins/access/types.ts (2)
Statements(17-19)AccessControl(21-22)packages/better-auth/src/plugins/organization/schema.ts (1)
OrganizationResource(400-400)packages/better-auth/src/plugins/oidc-provider/index.ts (1)
options(1766-1768)packages/better-auth/src/plugins/organization/types.ts (1)
OrganizationOptions(36-930)packages/better-auth/src/plugins/access/access.ts (1)
createAccessControl(79-88)
packages/core/src/db/adapter/factory.ts (1)
packages/core/src/db/adapter/types.ts (1)
AdapterFactoryConfig(18-33)
🪛 LanguageTool
docs/content/docs/guides/workos-migration-guide.mdx
[grammar] ~567-~567: Use a hyphen to join words.
Context: ...e middleware applies to }; ``` ### Page based auth In WorkOS, if resources were...
(QB_NEW_EN_HYPHEN)
[style] ~698-~698: Consider replacing this word to strengthen your wording.
Context: ...g WorkOS is a managed service and keeps your data in sync with your serve...
(AND_THAT)
.cspell/custom-words.txt
[grammar] ~15-~15: Wähle ein passenderes Wort
Context: ...ai merch prefs uncompromised myapp Neue user_01KBT4BMFF7ASGRDD0WZ6W63FF
(QB_NEW_DE_OTHER_ERROR_IDS_REPLACEMENT_NOUN)
docs/content/docs/plugins/2fa.mdx
[grammar] ~438-~438: Use a hyphen to join words.
Context: ... and 1 additional table to store the two factor authentication data. Table: `use...
(QB_NEW_EN_HYPHEN)
docs/content/docs/plugins/organization.mdx
[style] ~1835-~1835: ‘merged together’ might be wordy. Consider a shorter alternative.
Context: ..., both default and custom resources are merged together. If a custom resource has the same name...
(EN_WORDINESS_PREMIUM_MERGED_TOGETHER)
[grammar] ~2151-~2151: Use a hyphen to join words.
Context: ...c You can extend the default permission checking logic for role creation by prov...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (88)
.cspell/third-party.txt (1)
37-37: LGTM!The addition of "WorkOS" to the spell-check dictionary is appropriately aligned with the PR's migration and documentation focus.
.cspell/custom-words.txt (1)
16-16: Clarify the purpose of the user ID in the spell-check dictionary.Line 16 adds
user_01KBT4BMFF7ASGRDD0WZ6W63FF, which appears to be a specific user identifier in Bun ID format. Spell-check dictionaries typically contain project-specific terms and proper nouns, but this looks like a test fixture or placeholder ID.Please clarify:
- Why this specific ID needs to be in the spell-check dictionary
- Whether this is test data that should be excluded from version control
- If this relates to test fixtures in the PR's test suite
docs/components/nav-mobile.tsx (1)
59-67: Nice accessibility and structural improvements!The addition of
aria-label="Toggle navigation menu"improves screen reader support, and simplifying to a single button element makes the component cleaner.packages/core/src/social-providers/kick.ts (1)
3-7: Importing sharedrefreshAccessTokenhelper is appropriateReusing the centralized OAuth2
refreshAccessTokenhelper for Kick keeps token-refresh behavior consistent across providers and avoids provider-specific duplication. No issues here.docs/app/enterprise/_components/enterprise-hero.tsx (2)
28-30: Trusted-by section spacing/readability improvements LGTMSwitching to
py-8withspace-y-4and consistent text sizing/alignment makes this block read more clearly and separates it better from the hero copy without affecting layout structure.
96-104: Vercel SVG simplification and theming look correctUsing the canonical triangular path with a
viewBox="0 0 256 222"andfill="currentColor"is a solid cleanup: smaller markup, better theming, and consistent sizing with the other 1.3em icons.packages/better-auth/src/plugins/magic-link/magic-link.test.ts (1)
106-134: Test implementation looks good, but verify PR scope.The test is well-structured and comprehensively verifies the error redirect behavior:
- Properly validates 302 redirect status
- Confirms preservation of original query parameters
- Verifies addition of the error parameter
However, this test appears unrelated to the PR objectives ("dynamic org resources" for RBAC). While the enriched summary mentions centralized error redirection in magic-link, it's unclear why magic-link changes are included in an org resources PR.
Please confirm whether this magic-link test change should be part of this PR, or if it was included unintentionally.
packages/better-auth/src/plugins/username/username.test.ts (1)
503-521: LGTM!The test correctly verifies that the custom validator is enforced during both sign-up and sign-in flows, ensuring consistent validation behavior across authentication endpoints.
packages/better-auth/src/plugins/username/index.ts (2)
278-279: LGTM!Correctly handles async validators by awaiting the result. This change is necessary since the
usernameValidatortype allowsPromise<boolean>, and ensures consistency with the middleware handler on line 517.
449-450: LGTM!Correctly handles async validators by awaiting the result, consistent with the sign-in endpoint and middleware handler.
packages/better-auth/src/plugins/magic-link/index.ts (5)
336-338: LGTM!Correct application of the
redirectWithErrorhelper for invalid token scenarios.
339-344: LGTM!Correctly handles expired tokens by cleaning up the verification value before redirecting with the error.
357-372: LGTM!Properly handles both user creation failure and disabled signup scenarios with appropriate error codes.
380-386: LGTM!Correctly handles session creation failure with the centralized error redirection pattern.
388-409: LGTM!The successful verification flow remains intact, with a clean separation between error paths (routed through
redirectWithError) and success paths (direct redirects). This makes the code's intent clearer.packages/better-auth/src/plugins/two-factor/two-factor.test.ts (1)
325-348: LGTM! Excellent test isolation.This change properly isolates the invalid backup code test by creating a fresh 2FA session with independent headers. After the successful backup code verification above (lines 304-316), the original 2FA session would be consumed, so testing with a fresh session ensures the invalid backup code scenario is tested correctly.
docs/content/docs/guides/workos-migration-guide.mdx (1)
1-710: Comprehensive migration guide with well-designed migration script.This is a thorough, well-structured documentation file that effectively guides users through migrating from WorkOS to Better Auth. The migration script (lines 193–459) demonstrates solid defensive coding practices:
- The organization plugin provides a flexible access control system where permissions are defined based on user roles, and the guide appropriately references this for the roles/permissions feature mapping (line 33).
- Rate limiter implementation using timestamp filtering is correct and respects WorkOS API limits.
- Helper functions (
safeDateConversion,safeNameConversion) handle edge cases gracefully with reasonable fallbacks.- Per-user error handling allows batch migration to continue despite individual failures.
- Pagination logic correctly uses the WorkOS
aftercursor.The step-by-step structure, comparative code examples (WorkOS vs. Better Auth), and practical considerations at the end make this valuable for users planning migrations. The notes sections provide context for rate limits and script customization.
packages/core/src/social-providers/google.ts (1)
2-3: LGTM!The new imports are appropriate for the JWT verification flow using Google's public keys.
packages/better-auth/src/db/get-migration.ts (1)
10-17: New type-only Kysely imports are consistent
ColumnDataTypeandRawBuilderare correctly imported as types and only used in annotations fortypeMap; this keeps runtime bundles clean and matches howsqlreturns raw builders. No changes needed here.docs/components/nav-bar.tsx (1)
3-3: LGTM! Import path correctly updated.The import path change aligns with the new theme toggle component location.
docs/components/theme-toggle.tsx (6)
1-9: LGTM! Proper setup for client-side theme toggle.The client directive and imports are appropriate for a theme toggle component with animations.
15-24: LGTM! Proper icon selection logic.The function correctly maps theme values to icon components and returns null for undefined themes, which is appropriate for SSR handling.
26-55: LGTM! Solid implementation with proper SSR handling.The component follows best practices:
- Uses
mountedstate to prevent hydration mismatches with next-themes- Includes proper accessibility with
aria-label- AnimatePresence with
mode="wait"ensures smooth icon transitions- Props spreading order allows className customization
57-185: LGTM! Well-implemented animated light theme icon.The component uses framer-motion correctly for smooth transitions, with properly configured animation props and complex SVG animations for visual polish.
187-207: LGTM! Consistent animated dark theme icon.The component follows the same animation pattern as
LightThemeIcon, ensuring consistent transitions between theme states.
1-207: Major inconsistency between PR summary and actual changes.The PR is titled "dynamic org resources" and the summary describes adding "per-organization custom RBAC resources: new CRUD endpoints, runtime utilities and caching for organization resources, integration into role creation..."
However, this file implements a theme toggle UI component with animations for the documentation site, which is completely unrelated to RBAC or organization resources.
Please verify:
- Are these the correct files for this PR?
- If not, has the theme toggle been included in the wrong PR?
- Is the PR summary incorrect or outdated?
packages/better-auth/src/plugins/two-factor/verify-two-factor.ts (2)
71-74: LGTM - improved cookie cleanup consistency.The two-factor cookie is now always cleared after successful verification, regardless of the trust-device flow. This prevents duplication (previously cleared inside the trust-device branch) and ensures consistent cleanup of temporary authentication state.
63-66: This change follows the established codebase pattern for verification token cleanup. Token deletion without explicit error handling is consistent across all plugins (email-otp, magic-link, phone-number, etc.), and verification tokens include expiration times as a compensating control. No changes needed.packages/better-auth/src/cookies/session-store.ts (1)
232-232: LGTM!The export of
createAccountStorefollows the same pattern ascreateSessionStoreand enables proper account cookie chunking in the cleanup logic.packages/better-auth/src/cookies/index.ts (3)
24-24: LGTM!The import of
createAccountStoreis necessary for the account data cleanup logic added todeleteSessionCookie.
310-324: LGTM!The account data cleanup logic correctly mirrors the session data cleanup pattern. It properly clears both the main cookie and all chunked cookies when account storage is enabled.
326-332: The code at lines 326-332 is correct and requires no changes.
ctx.context.oauthConfigis always defined andstoreStateStrategyis a required property on theoauthConfigobject (not optional). Direct property access without optional chaining is appropriate throughout the codebase, as evidenced by consistent patterns in oauth2/state.ts, oauth-proxy/index.ts, and other modules. Thectx.context.createAuthCookie()method exists and is the appropriate way to generate OAuth state cookie configuration. Unlike session and account cookies which use chunking stores for large payloads (>4093 bytes),oauth_stateis a single cookie that does not require chunk cleanup—the maxAge: 0 cleanup pattern shown here is correct and complete.Likely an incorrect or invalid review comment.
packages/better-auth/src/plugins/admin/types.ts (1)
79-84: NewallowImpersonatingAdminsoption looks consistent with route logicFlag name, typing, and documented default (
false) line up with the guard inimpersonateUser, so this extension ofAdminOptionslooks good.docs/content/docs/plugins/admin.mdx (1)
823-832: Documentation forallowImpersonatingAdminsmatches implementationThe option description, default (
false), and example usage line up with the new impersonation guard in the admin routes.packages/better-auth/src/plugins/admin/error-codes.ts (1)
31-31: New admin error code is consistent with existing conventions
YOU_CANNOT_IMPERSONATE_ADMINSfollows the established naming/message pattern and is correctly wired for the impersonation guard.biome.json (1)
84-84: LGTM!The coverage directory exclusion aligns with the broader coverage infrastructure being added across the repository.
packages/sso/package.json (1)
4-4: LGTM!The version bump and coverage script addition are consistent with the repository-wide coverage infrastructure updates.
Also applies to: 34-34
docs/components/mobile-search-icon.tsx (1)
26-26: Verify this change is intentional.The size adjustment from
size-9tosize-8appears unrelated to the PR's objective of adding dynamic organization resources. Please confirm this change is intentional and not an accidental modification.packages/scim/package.json (1)
4-4: LGTM!The version bump and coverage script addition align with the repository-wide coverage infrastructure updates.
Also applies to: 25-25
packages/cli/package.json (1)
3-3: LGTM!The version bump and coverage script addition are consistent with the repository-wide updates.
Also applies to: 20-20
packages/core/package.json (1)
3-3: LGTM!The version bump and coverage script addition align with the repository-wide coverage infrastructure updates.
Also applies to: 113-114
packages/better-auth/package.json (1)
3-3: LGTM!The version bump and coverage script addition are consistent with the repository-wide coverage infrastructure updates.
Also applies to: 29-29
pnpm-workspace.yaml (1)
11-11: Dependency versions confirmed to exist and are stable.The updates include:
tsdownbump from^0.17.0to^0.17.2(v0.17.2 exists)- Restructuring
vitestinto a catalog with coverage tooling (@vitest/coverage-v8: ^4.0.15andvitest: ^4.0.15) (both v4.0.15 exist)All specified versions are available on npm and stable.
packages/passkey/package.json (1)
3-3: Version bump and coverage script look consistent with repo toolingThe new beta version and
coveragescript (vitest run --coverage) align with the broader coverage setup; nothing concerning here.Also applies to: 14-14
test/package.json (1)
7-7: Test package coverage wiring is consistentAdding
coverageand pointing Vitest tocatalog:vitestmatches the repo-wide coverage pattern; just ensure the catalog entry exists at the root (it likely already does).Also applies to: 16-16
packages/core/src/db/adapter/index.ts (1)
115-122:supportsArraysflag is well-aligned with existing adapter configThe new
supportsArrays?: booleanoption and its JSDoc mirror the existingsupportsJSON/supportsBooleanspattern and integrate cleanly intoDBAdapterFactoryConfig.packages/better-auth/src/adapters/drizzle-adapter/drizzle-adapter.ts (1)
582-587: Adapter config flags for JSON/arrays match Drizzle+DB capabilitiesSetting
supportsJSONandsupportsArraysbased solely onconfig.provider === "pg"is consistent with Drizzle’s native JSON/array support and the stringification strategy for mysql/sqlite.docs/app/enterprise/_components/enterprise-form.tsx (1)
55-55: Styling tweaks are self-contained and preserve behaviorThe switch to
bg-linear-*utilities and updatedButtonclasses only affect presentation; form behavior, submission flow, and disabled handling stay intact.Also applies to: 57-57, 161-165
packages/better-auth/src/adapters/prisma-adapter/prisma-adapter.ts (1)
275-278: PrismafindFirstusage andsupportsArraysflag look correctSwitching
findOneto usedb[model]!.findFirst({ where, select })aligns with Prisma’s single-row semantics and keeps the return type (record ornull) consistent. The newsupportsArraysflag beingtrueonly forpostgresql/mongodbmatches Prisma’s native array support.Also applies to: 414-417
packages/stripe/package.json (1)
1-54: LGTM!The version bump, coverage script addition, and exports configuration are consistent with the repository-wide coverage tooling standardization.
package.json (1)
21-26: LGTM! Coverage infrastructure well-structured.The coverage pipeline properly chains collection, merging, and reporting steps. The shell script in
coverage:collectcorrectly uses package directory names to prefix coverage files, avoiding naming collisions.One minor note: the
for f in $(find ...)pattern can have issues with filenames containing spaces, though this is unlikely in a monorepo with conventional naming.turbo.json (1)
31-34: LGTM!The coverage task correctly depends on
buildand caches thecoverageoutput directory. This enables efficient local development while CI can still run tests fresh when needed.packages/core/src/db/adapter/factory.ts (3)
59-72: LGTM! Clean separation of array support from JSON support.The new
supportsArraysconfig flag correctly defaults tofalse, enabling databases like PostgreSQL (which supports native arrays) to opt-in while SQLite/MySQL fall back to JSON serialization. This aligns with the Drizzle schema generator changes.
255-261: Correct transformation for array serialization.The input transformation properly checks
supportsArraysinstead ofsupportsJSONwhen determining whether to stringify arrays. This ensures arrays are serialized to JSON strings only when the database lacks native array support.
350-355: Output transformation correctly deserializes array fields whensupportsArraysis false.The logic mirrors the input transformation, parsing JSON strings back to arrays (for
string[]andnumber[]types) when the adapter doesn't support native arrays. This maintains data integrity across the serialization round-trip, consistent with similar patterns for JSON and date handling.packages/better-auth/src/adapters/kysely-adapter/kysely-adapter.ts (3)
586-589: Consistent withupdateManypattern.The implementation correctly mirrors the
updateManyapproach for returning the count of deleted rows.
613-617: LGTM - Adapter capability flags correctly updated.The change correctly limits
supportsJSONto PostgreSQL only, as other databases require stringified JSON. The addition ofsupportsArrays: falseis consistent with the comment explaining that arrays must be stringified.
539-542: The implementation is correct. In Kysely,UPDATEandDELETEqueries withexecuteTakeFirst()always return a result object withnumUpdatedRowsandnumDeletedRowsrespectively—neverundefined. This differs fromSELECTqueries whereexecuteTakeFirst()can returnundefined. The code is properly tested with comprehensive test coverage, including the edge case of updating all rows when the where clause is empty.Likely an incorrect or invalid review comment.
packages/better-auth/src/oauth2/link-account.ts (2)
10-20: Clean refactor to options object pattern.The change from multiple parameters to a single
optsobject improves readability and makes optional parameters more manageable. The destructuring on lines 19-20 maintains backward compatibility with the rest of the function.
48-50: LGTM - Flexible trust determination.The logic correctly allows callers to explicitly mark a provider as trusted via
opts.isTrustedProvider, falling back to thetrustedProvidersconfiguration array. This supports SSO and other flows where trust is determined externally.packages/better-auth/src/oauth2/link-account.test.ts (2)
271-320: Well-structured test for denied linking scenario.The test correctly mocks the OAuth token endpoint, verifies the error redirect, and confirms no Google account was linked. Good use of the existing test patterns.
322-392: LGTM - Good coverage of successful linking with verified email.This test correctly validates that account linking succeeds when the provider reports the email as verified, even without any trusted providers configured. The assertions properly verify both the redirect and the database state.
packages/expo/src/plugins/index.ts (1)
1-1: LGTM - Clean barrel export for Expo plugins.Simple and correct re-export pattern for exposing the last-login-method plugin.
docs/content/docs/plugins/community-plugins.mdx (1)
24-24: LGTM - New community plugin entry follows existing format and all links are valid.The entry for
better-auth-universityfollows the established table format with consistent styling. Both the repository link and author profile are accessible and correctly formatted.packages/expo/tsdown.config.ts (1)
6-6: Plugin barrel correctly added to build entriesIncluding
./src/plugins/index.tsinentrykeeps the new Expo plugins export in sync with the published./pluginspath.packages/sso/src/routes/sso.ts (1)
1352-1356: isTrustedProvider computation is consistent and well-scopedDeriving
isTrustedProvideronly whendomainVerifiedis true andvalidateEmailDomain(userInfo.email, provider.domain)passes keeps automatic linking restricted to verified, domain-matching providers and mirrors the SAML trusted‑provider logic. No changes needed here.Also applies to: 1378-1381
packages/expo/test/last-login-method.test.ts (1)
1-105: Good coverage for Expo last-login-method pluginThe mock storage matches the plugin’s storage interface, and the three tests exercise the key behaviors: default email detection, custom
customResolveMethod, andstoragePrefixkeying. This gives solid confidence in the new Expo plugin.packages/expo/package.json (1)
3-3: Expo plugins export and coverage script wired correctlyThe version bump,
coveragescript, new./pluginsexport, and correspondingtypesVersions["plugins"]entry are consistent with the newsrc/plugins/index.tsentry point and the updated tsdown config, so the Expo plugins surface should build and type‑resolve correctly.Also applies to: 17-17, 33-38, 45-50
packages/expo/src/plugins/last-login-method.ts (1)
1-96: Last-login-method plugin implementation looks solidThe plugin cleanly derives a last login method from common auth routes, persists it under a configurable
storagePrefix, and exposes simple helpers (getLastUsedLoginMethod,clearLastUsedLoginMethod,isLastUsedLoginMethod). The shape matches the tests and BetterAuth client plugin expectations; no changes needed.docs/content/docs/plugins/organization.mdx (2)
1772-2274: Custom Resources documentation is comprehensive and well-structured.The documentation thoroughly covers the new custom resources feature including configuration options, CRUD operations, validation rules, use cases, and best practices. The examples are clear and the callouts appropriately highlight important considerations like migration requirements and resource protection.
Minor note from static analysis: Line 1835 uses "merged together" which could be simplified to just "merged", though this is a minor stylistic preference.
2222-2223: Member role description clarification is accurate.The updated description correctly reflects that members have read-only access and cannot modify resources.
packages/better-auth/src/plugins/organization/routes/crud-access-control.ts (5)
181-219: Well-structured canCreateRole hook implementation.The discriminated union pattern for the hook result (
allow: true,allow: true + bypass,allow: false + message) is clean and provides flexibility. The logging for each path aids debugging.
1116-1181: checkForInvalidResources refactoring is well-structured.The function properly handles three scenarios:
- Missing resources with custom resources disabled → throws error
- Missing resources with custom resources enabled → auto-creates
- All resources exist → validates/expands permissions
The return value of auto-created resource names enables proper delegation check skipping downstream.
1248-1287: Resource creation and cache invalidation is correct.The cache invalidation now correctly occurs only when resources are actually created (
resourcesToCreate.length > 0), addressing the previous review feedback. The parallel creation pattern is appropriate since each resource is independent.
1365-1396: Permission expansion logic correctly distinguishes default vs custom resources.The check on line 1366 properly identifies custom resources by verifying they're not in
defaultStatements. Only custom resources can have their permissions auto-expanded, which is the correct behavior to protect default resource definitions.
1527-1540: Delegation check skip for auto-created resources is appropriate.When resources are auto-created during role creation, the user by definition doesn't have pre-existing permissions for them. Skipping the delegation check for these resources is the correct behavior, and the logging provides good observability.
packages/better-auth/src/plugins/organization/organization.ts (3)
90-96: DynamicResourceEndpoints type definition is well-structured.The type correctly maps all five CRUD resource endpoints with proper generic typing, maintaining consistency with the existing
DynamicAccessControlEndpointspattern.
929-942: Runtime endpoint wiring correctly mirrors type-level conditional.The conditional check for
enableCustomResourceson line 937 properly gates the resource CRUD endpoints, consistent with the type definition on lines 281-283.
1061-1109: organizationResource schema is complete and well-designed.The schema includes:
- Proper foreign key reference to organization
- Indexes on
organizationIdandresourcefor efficient lookups- Standard
createdAt/updatedAttimestamps with auto-update- Support for additional fields via
additionalFieldsThis aligns well with the CRUD operations and caching logic in the related files.
packages/better-auth/src/plugins/organization/routes/crud-resources.ts (5)
35-45: getAdditionalFields properly clones to avoid mutation.The previous review concern about mutating
options.schema.organizationResource.additionalFieldshas been addressed. The function now creates a new object viaObject.fromEntrieswhenshouldBePartialis true, keeping the original options immutable.
120-127: Error code for missing organization is correct.The previous review noted the wrong error code was used. This has been fixed to use
NO_ACTIVE_ORGANIZATIONwhich is appropriate for the context.
687-693: Safe JSON parsing with proper null check.The previous review concern about unhandled JSON parse errors has been addressed. Using
safeJSONParsewith a null check on line 692 ensures malformed permission data doesn't crash the request.
839-876: Resource listing correctly merges default and custom resources.The endpoint properly:
- Retrieves default resources from
options.ac?.statementswithisCustom: false, isProtected: true- Retrieves custom resources from the database with
isCustom: true, isProtected: false- Uses
safeJSONParsefor safe permission parsing- Returns a unified list with clear metadata flags
986-999: Default resource lookup is efficient and correct.The endpoint first checks if the resource exists in the default
ac.statementsbefore querying the database, avoiding unnecessary DB calls for built-in resources.packages/better-auth/src/plugins/organization/load-resources.ts (4)
11-15: In-memory cache has multi-instance limitations.As noted in a previous review, this
Map-based cache is process-local. In horizontally-scaled deployments, cache invalidation on one instance won't propagate to others.Consider:
- Documenting this limitation in the code or user docs
- Offering an optional external cache adapter (Redis) for production
- Adding a TTL to reduce stale data duration
This is acceptable for the initial implementation but should be addressed for production-scale deployments.
50-68: JSON parsing and validation is properly handled.The implementation uses
safeJSONParsefor safe JSON handling and Zod for validation. The error logging includes the permissions data, which is acceptable since these are action names (like "create", "read") rather than sensitive user data.
160-209: Resource name validation is comprehensive and well-documented.The validation covers:
- Length constraints (1-50 characters)
- Character format (alphanumeric + underscore, case-insensitive)
- Reserved name protection
- Custom validation hook for additional rules
The JSDoc accurately describes the regex behavior, and the documentation examples in organization.mdx correctly show both lowercase and uppercase names as valid.
100-108: Statement merging correctly handles override semantics.The spread order on lines 102-105 ensures custom resources override defaults with the same name. This is the documented behavior and allows organizations to customize permission sets for their needs.
| }; | ||
| ``` | ||
|
|
||
| ### Page based auth |
There was a problem hiding this comment.
Fix compound adjective hyphenation.
The heading "Page based auth" should use a hyphen: "Page-based auth". Compound adjectives preceding a noun require hyphenation in standard English.
-### Page based auth
+### Page-based auth📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Page based auth | |
| ### Page-based auth |
🧰 Tools
🪛 LanguageTool
[grammar] ~567-~567: Use a hyphen to join words.
Context: ...e middleware applies to }; ``` ### Page based auth In WorkOS, if resources were...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In docs/content/docs/guides/workos-migration-guide.mdx around line 567, the
heading "Page based auth" uses an unhyphenated compound adjective; change it to
"Page-based auth" to correctly hyphenate the compound adjective preceding a
noun. Update the heading text only, keeping surrounding formatting and
punctuation intact.
| const state = generateRandomString(32); | ||
| const storeStateStrategy = | ||
| c.context.oauthConfig?.storeStateStrategy || "cookie"; | ||
| const storeStateStrategy = c.context.oauthConfig.storeStateStrategy; |
There was a problem hiding this comment.
Removal of optional chaining and default value introduces runtime risk and behavioral change.
Two issues with this change:
-
Potential TypeError: If
c.context.oauthConfigis undefined, accessing.storeStateStrategywill throw. The original?.guarded against this. -
Silent default change: Previously, an undefined
storeStateStrategydefaulted to"cookie". Now it will beundefined, failing the=== "cookie"check and falling through to the database strategy — a breaking behavioral change for deployments relying on the cookie default.
- const storeStateStrategy = c.context.oauthConfig.storeStateStrategy;
+ const storeStateStrategy = c.context.oauthConfig?.storeStateStrategy || "cookie";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const storeStateStrategy = c.context.oauthConfig.storeStateStrategy; | |
| const storeStateStrategy = c.context.oauthConfig?.storeStateStrategy || "cookie"; |
🤖 Prompt for AI Agents
In packages/better-auth/src/oauth2/state.ts around line 30, reverting the
optional chaining removed a guard and the implicit "cookie" default; restore a
safe read from c.context.oauthConfig and ensure storeStateStrategy defaults to
"cookie" when missing (e.g., use a guarded access plus nullish-coalescing or
default assignment) so you do not throw if oauthConfig is undefined and behavior
remains the same as before.
| const state = c.query.state || c.body.state; | ||
| const storeStateStrategy = | ||
| c.context.oauthConfig.storeStateStrategy || "cookie"; | ||
| const storeStateStrategy = c.context.oauthConfig.storeStateStrategy; |
There was a problem hiding this comment.
Same issue: missing optional chaining and default value.
Mirrors the problem on line 30 — parseState is also vulnerable to a TypeError if oauthConfig is undefined, and will silently switch to database strategy when storeStateStrategy is absent.
- const storeStateStrategy = c.context.oauthConfig.storeStateStrategy;
+ const storeStateStrategy = c.context.oauthConfig?.storeStateStrategy || "cookie";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const storeStateStrategy = c.context.oauthConfig.storeStateStrategy; | |
| const storeStateStrategy = c.context.oauthConfig?.storeStateStrategy || "cookie"; |
🤖 Prompt for AI Agents
In packages/better-auth/src/oauth2/state.ts around line 101, accessing
c.context.oauthConfig.storeStateStrategy can throw if oauthConfig is undefined
and will silently default to the DB strategy when storeStateStrategy is missing;
change the access to use optional chaining and supply a safe default (e.g.,
const storeStateStrategy = c.context?.oauthConfig?.storeStateStrategy ??
'database' or the intended default) so parseState handles undefined oauthConfig
and absent storeStateStrategy predictably.
| it("should not allow to impersonate admins", async () => { | ||
| const userToImpersonate = await client.signUp.email({ | ||
| email: "impersonate-admin@mail.com", | ||
| password: "password", | ||
| name: "Impersonate Admin User", | ||
| }); | ||
| const userId = userToImpersonate.data?.user.id || ""; | ||
| await client.admin.setRole({ | ||
| userId, | ||
| role: "admin", | ||
| fetchOptions: { | ||
| headers: adminHeaders, | ||
| }, | ||
| }); | ||
| const res = await client.admin.impersonateUser( | ||
| { | ||
| userId, | ||
| }, | ||
| { headers: adminHeaders }, | ||
| ); | ||
|
|
||
| expect(res.error?.status).toBe(403); | ||
| }); |
There was a problem hiding this comment.
Fix setRole call to pass headers as second argument and assert the new error code
In this test you’re calling setRole with fetchOptions embedded in the body, unlike every other usage in this file:
await client.admin.setRole({
userId,
role: "admin",
fetchOptions: {
headers: adminHeaders,
},
});Elsewhere the client is used as (body, { headers }), so this call likely:
- Fails type‑checking due to the extra
fetchOptionsproperty, and/or - Sends the request without
adminHeaders, meaning you don’t actually exercise the “admin impersonating another admin” guard.
You probably want:
- await client.admin.setRole({
- userId,
- role: "admin",
- fetchOptions: {
- headers: adminHeaders,
- },
- });
+ await client.admin.setRole(
+ {
+ userId,
+ role: "admin",
+ },
+ {
+ headers: adminHeaders,
+ },
+ );Since this test is covering the new YOU_CANNOT_IMPERSONATE_ADMINS path, it would also be good to assert the error code, not just the status:
- expect(res.error?.status).toBe(403);
+ expect(res.error?.status).toBe(403);
+ expect(res.error?.code).toBe("YOU_CANNOT_IMPERSONATE_ADMINS");| "number[]": { | ||
| sqlite: `integer('${name}').array()`, | ||
| sqlite: `text('${name}', { mode: "json" })`, | ||
| pg: field.bigint | ||
| ? `bigint('${name}', { mode: 'number' }).array()` | ||
| : `integer('${name}').array()`, | ||
| mysql: field.bigint | ||
| ? `bigint('${name}', { mode: 'number' }).array()` | ||
| : `int('${name}').array()`, | ||
| mysql: `text('${name}', { mode: 'json' })`, | ||
| }, | ||
| "string[]": { | ||
| sqlite: `text('${name}').array()`, | ||
| sqlite: `text('${name}', { mode: "json" })`, | ||
| pg: `text('${name}').array()`, | ||
| mysql: `text('${name}').array()`, | ||
| mysql: `text('${name}', { mode: "json" })`, | ||
| }, | ||
| json: { | ||
| sqlite: `text('${name}')`, | ||
| sqlite: `text('${name}', { mode: "json" })`, | ||
| pg: `jsonb('${name}')`, | ||
| mysql: `json('${name}')`, | ||
| mysql: `json('${name}', { mode: "json" })`, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
drizzle-orm mysql-core json column mode option documentation
💡 Result:
Short answer: There is no "mode" option for the mysql-core json column in Drizzle ORM. The mysql-core json column accepts no { mode: ... } option — you create it with json() and can use .$type() for TypeScript inference. [1][2][3]
References:
- Official Drizzle MySQL column types (json usage and .$type). [1]
- npm / package docs showing json('name') usage. [3]
- Type definitions (json column builder signature) confirming no mode option. [2]
If you meant JSON parsing behavior when reading from the driver (string ↔ object), see the community patch that adds mapFromDriverValue as a workaround. [4]
Sources:
[1] Drizzle ORM — MySQL column types documentation.
[2] drizzle-orm mysql-core d.ts (json column builder).
[3] drizzle-orm-mysql npm page (column list and json examples).
[4] Patch: "Patch Drizzle MySQL JSON" (community snippet).
Remove mode option from MySQL json column at line 156
The MySQL json() function in drizzle-orm does not accept a mode option. Line 156 should be:
mysql: `json('${name}')`
Remove the { mode: "json" } parameter, which is only valid for text() columns used for JSON serialization.
🤖 Prompt for AI Agents
In packages/cli/src/generators/drizzle.ts around lines 141 to 157, the MySQL
json column is being generated with an invalid option ({ mode: "json" }); update
the MySQL branch for the json type to call the drizzle-orm json() helper without
the mode option (i.e., use json('${name}') for MySQL) — remove the { mode:
"json" } parameter so the generated schema uses the correct mysql json(...)
signature.
| const provider: "sqlite" | "postgresql" | "mysql" | "mongodb" = | ||
| adapter.options?.provider || "postgresql"; |
There was a problem hiding this comment.
Fix default-value generation for string[]/number[] fields on sqlite/mysql
The code maps array types to String on sqlite/mysql (lines 136-139 and 143-157), but the default-value generation (lines 245-281) still emits default([]) or default([...]) syntax, which is invalid for scalar String fields. For sqlite/mysql, array defaults need to be JSON-encoded strings: default("[]") or default("[1,2,3]").
Apply the proposed fix to special-case array defaults when the underlying Prisma type is String:
- Empty array default: emit
default("[]")instead ofdefault([]) - Non-empty string[]/number[] defaults: JSON-encode and escape the value, e.g.,
default("[\"a\",\"b\"]") - Preserve existing
default([...])syntax for postgresql/mongodb where the field type is String[]/Int[]
Additionally, the provider type union ("sqlite" | "postgresql" | "mysql" | "mongodb") excludes valid Prisma datasources like cockroachdb and sqlserver. Consider whether these should be treated as postgresql-compatible (using array syntax) or require explicit handling.
🤖 Prompt for AI Agents
In packages/cli/src/generators/prisma.ts around lines 17 and 136-281, the code
maps array types to Prisma String for sqlite/mysql but still emits array-default
syntax like default([])/default([..]) which is invalid for scalar String fields;
change default-generation to special-case when the underlying Prisma type is
String and the provider is sqlite or mysql: emit default("[]") for empty arrays
and JSON-encode (and escape) non-empty string[]/number[] values into a single
string literal (e.g. default("[\"a\",\"b\"]") or default("[1,2,3]")) instead of
array syntax; keep existing array-default syntax for providers that support
native arrays (postgresql, mongodb and any others you treat as array-capable);
also update the provider type union (line ~17) to include other valid Prisma
datasources such as cockroachdb and sqlserver or add logic to classify them as
postgres-compatible so they use native-array defaults rather than JSON strings.
|
|
||
| // Verify JWT integrity | ||
| // See https://developers.google.com/identity/sign-in/web/backend-auth#verify-the-integrity-of-the-id-token | ||
|
|
||
| const { kid, alg: jwtAlg } = decodeProtectedHeader(token); | ||
| if (!kid || !jwtAlg) return false; | ||
|
|
||
| const publicKey = await getGooglePublicKey(kid); | ||
| const { payload: jwtClaims } = await jwtVerify(token, publicKey, { | ||
| algorithms: [jwtAlg], | ||
| issuer: ["https://accounts.google.com", "accounts.google.com"], | ||
| audience: options.clientId, | ||
| maxTokenAge: "1h", | ||
| }); | ||
|
|
||
| if (nonce && jwtClaims.nonce !== nonce) { | ||
| return false; | ||
| } | ||
| const isValid = | ||
| tokenInfo.aud === options.clientId && | ||
| (tokenInfo.iss === "https://accounts.google.com" || | ||
| tokenInfo.iss === "accounts.google.com"); | ||
| return isValid; | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Missing error handling for JWT verification failures.
decodeProtectedHeader, getGooglePublicKey, and jwtVerify all throw exceptions on failure (e.g., malformed token, expired token, invalid signature). Without a try-catch, these exceptions will propagate instead of returning false as expected.
Apply this diff to handle verification errors gracefully:
-
- // Verify JWT integrity
- // See https://developers.google.com/identity/sign-in/web/backend-auth#verify-the-integrity-of-the-id-token
-
- const { kid, alg: jwtAlg } = decodeProtectedHeader(token);
- if (!kid || !jwtAlg) return false;
-
- const publicKey = await getGooglePublicKey(kid);
- const { payload: jwtClaims } = await jwtVerify(token, publicKey, {
- algorithms: [jwtAlg],
- issuer: ["https://accounts.google.com", "accounts.google.com"],
- audience: options.clientId,
- maxTokenAge: "1h",
- });
-
- if (nonce && jwtClaims.nonce !== nonce) {
- return false;
- }
-
- return true;
+ try {
+ // Verify JWT integrity
+ // See https://developers.google.com/identity/sign-in/web/backend-auth#verify-the-integrity-of-the-id-token
+
+ const { kid, alg: jwtAlg } = decodeProtectedHeader(token);
+ if (!kid || !jwtAlg) return false;
+
+ const publicKey = await getGooglePublicKey(kid);
+ const { payload: jwtClaims } = await jwtVerify(token, publicKey, {
+ algorithms: [jwtAlg],
+ issuer: ["https://accounts.google.com", "accounts.google.com"],
+ audience: options.clientId,
+ maxTokenAge: "1h",
+ });
+
+ if (nonce && jwtClaims.nonce !== nonce) {
+ return false;
+ }
+
+ return true;
+ } catch {
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Verify JWT integrity | |
| // See https://developers.google.com/identity/sign-in/web/backend-auth#verify-the-integrity-of-the-id-token | |
| const { kid, alg: jwtAlg } = decodeProtectedHeader(token); | |
| if (!kid || !jwtAlg) return false; | |
| const publicKey = await getGooglePublicKey(kid); | |
| const { payload: jwtClaims } = await jwtVerify(token, publicKey, { | |
| algorithms: [jwtAlg], | |
| issuer: ["https://accounts.google.com", "accounts.google.com"], | |
| audience: options.clientId, | |
| maxTokenAge: "1h", | |
| }); | |
| if (nonce && jwtClaims.nonce !== nonce) { | |
| return false; | |
| } | |
| const isValid = | |
| tokenInfo.aud === options.clientId && | |
| (tokenInfo.iss === "https://accounts.google.com" || | |
| tokenInfo.iss === "accounts.google.com"); | |
| return isValid; | |
| return true; | |
| try { | |
| // Verify JWT integrity | |
| // See https://developers.google.com/identity/sign-in/web/backend-auth#verify-the-integrity-of-the-id-token | |
| const { kid, alg: jwtAlg } = decodeProtectedHeader(token); | |
| if (!kid || !jwtAlg) return false; | |
| const publicKey = await getGooglePublicKey(kid); | |
| const { payload: jwtClaims } = await jwtVerify(token, publicKey, { | |
| algorithms: [jwtAlg], | |
| issuer: ["https://accounts.google.com", "accounts.google.com"], | |
| audience: options.clientId, | |
| maxTokenAge: "1h", | |
| }); | |
| if (nonce && jwtClaims.nonce !== nonce) { | |
| return false; | |
| } | |
| return true; | |
| } catch { | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/social-providers/google.ts around lines 130 to 149, the JWT
verification sequence (decodeProtectedHeader, getGooglePublicKey, jwtVerify) can
throw on malformed/expired/invalid tokens but is not wrapped, causing exceptions
to bubble instead of returning false; wrap the entire verification block in a
try-catch, return false from the catch branch (optionally log the error/debug
info) so any verification failure results in a graceful false instead of
throwing.
| describe("OIDC account linking with domainVerified", async () => { | ||
| const { auth, signInWithTestUser, customFetchImpl, cookieSetter } = | ||
| await getTestInstance({ | ||
| account: { | ||
| accountLinking: { | ||
| enabled: true, | ||
| trustedProviders: [], | ||
| }, | ||
| }, | ||
| plugins: [ | ||
| sso({ | ||
| domainVerification: { | ||
| enabled: true, | ||
| }, | ||
| }), | ||
| ], | ||
| }); | ||
|
|
||
| const authClient = createAuthClient({ | ||
| plugins: [ssoClient()], | ||
| baseURL: "http://localhost:3000", | ||
| fetchOptions: { | ||
| customFetchImpl, | ||
| }, | ||
| }); | ||
|
|
||
| beforeAll(async () => { | ||
| await server.issuer.keys.generate("RS256"); | ||
| await server.start(8080, "localhost"); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await server.stop().catch(() => {}); | ||
| }); | ||
|
|
||
| async function simulateOAuthFlow(authUrl: string, headers: Headers) { | ||
| let location: string | null = null; | ||
| await betterFetch(authUrl, { | ||
| method: "GET", | ||
| redirect: "manual", | ||
| onError(context) { | ||
| location = context.response.headers.get("location"); | ||
| }, | ||
| }); | ||
|
|
||
| if (!location) throw new Error("No redirect location found"); | ||
|
|
||
| let callbackURL = ""; | ||
| const newHeaders = new Headers(); | ||
| await betterFetch(location, { | ||
| method: "GET", | ||
| customFetchImpl, | ||
| headers, | ||
| onError(context) { | ||
| callbackURL = context.response.headers.get("location") || ""; | ||
| cookieSetter(newHeaders)(context); | ||
| }, | ||
| }); | ||
|
|
||
| return { callbackURL, headers: newHeaders }; | ||
| } | ||
|
|
||
| it("should allow account linking when domain is verified and email domain matches", async () => { | ||
| const testEmail = "linking-test@verified-oidc.com"; | ||
| const testDomain = "verified-oidc.com"; | ||
|
|
||
| server.service.on("beforeTokenSigning", (token) => { | ||
| token.payload.email = testEmail; | ||
| token.payload.email_verified = false; | ||
| token.payload.name = "Domain Verified User"; | ||
| token.payload.sub = "oidc-domain-verified-user"; | ||
| }); | ||
|
|
||
| const { headers } = await signInWithTestUser(); | ||
|
|
||
| const provider = await auth.api.registerSSOProvider({ | ||
| body: { | ||
| providerId: "domain-verified-oidc", | ||
| issuer: server.issuer.url!, | ||
| domain: testDomain, | ||
| oidcConfig: { | ||
| clientId: "test", | ||
| clientSecret: "test", | ||
| authorizationEndpoint: `${server.issuer.url}/authorize`, | ||
| tokenEndpoint: `${server.issuer.url}/token`, | ||
| jwksEndpoint: `${server.issuer.url}/jwks`, | ||
| discoveryEndpoint: `${server.issuer.url}/.well-known/openid-configuration`, | ||
| mapping: { | ||
| id: "sub", | ||
| email: "email", | ||
| emailVerified: "email_verified", | ||
| name: "name", | ||
| }, | ||
| }, | ||
| }, | ||
| headers, | ||
| }); | ||
|
|
||
| expect(provider.domainVerified).toBe(false); | ||
|
|
||
| const ctx = await auth.$context; | ||
| await ctx.adapter.update({ | ||
| model: "ssoProvider", | ||
| where: [{ field: "providerId", value: provider.providerId }], | ||
| update: { | ||
| domainVerified: true, | ||
| }, | ||
| }); | ||
|
|
||
| const updatedProvider = await ctx.adapter.findOne<{ | ||
| domainVerified: boolean; | ||
| domain: string; | ||
| }>({ | ||
| model: "ssoProvider", | ||
| where: [{ field: "providerId", value: provider.providerId }], | ||
| }); | ||
| expect(updatedProvider?.domainVerified).toBe(true); | ||
|
|
||
| await ctx.adapter.create({ | ||
| model: "user", | ||
| data: { | ||
| id: "existing-oidc-domain-user", | ||
| email: testEmail, | ||
| name: "Existing User", | ||
| emailVerified: true, | ||
| createdAt: new Date(), | ||
| updatedAt: new Date(), | ||
| }, | ||
| forceAllowId: true, | ||
| }); | ||
|
|
||
| const newHeaders = new Headers(); | ||
| const res = await authClient.signIn.sso({ | ||
| providerId: "domain-verified-oidc", | ||
| callbackURL: "/dashboard", | ||
| fetchOptions: { | ||
| throw: true, | ||
| onSuccess: cookieSetter(newHeaders), | ||
| }, | ||
| }); | ||
|
|
||
| expect(res.url).toContain("http://localhost:8080/authorize"); | ||
|
|
||
| const { callbackURL } = await simulateOAuthFlow(res.url, newHeaders); | ||
|
|
||
| expect(callbackURL).toContain("/dashboard"); | ||
| expect(callbackURL).not.toContain("error"); | ||
|
|
||
| const accounts = await ctx.adapter.findMany<{ | ||
| providerId: string; | ||
| accountId: string; | ||
| userId: string; | ||
| }>({ | ||
| model: "account", | ||
| where: [{ field: "userId", value: "existing-oidc-domain-user" }], | ||
| }); | ||
| const linkedAccount = accounts.find( | ||
| (a) => a.providerId === "domain-verified-oidc", | ||
| ); | ||
| expect(linkedAccount).toBeTruthy(); | ||
| expect(linkedAccount?.accountId).toBe("oidc-domain-verified-user"); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the test file and check its size
wc -l packages/sso/src/oidc.test.tsRepository: decocms/better-auth
Length of output: 96
🏁 Script executed:
cat -n packages/sso/src/oidc.test.tsRepository: decocms/better-auth
Length of output: 25169
🏁 Script executed:
# Verify EventEmitter behavior by checking if oauth2-mock-server uses standard EventEmitter
# and whether handlers accumulate
rg -A5 "beforeTokenSigning" packages/sso/src/oidc.test.ts | head -30Repository: decocms/better-auth
Length of output: 1065
🏁 Script executed:
# Check if there are any handler cleanup mechanisms in afterAll
rg -B2 -A5 "afterAll" packages/sso/src/oidc.test.ts | grep -A5 "afterAll"Repository: decocms/better-auth
Length of output: 854
Isolate the beforeTokenSigning handler to prevent cross-test contamination
This suite registers a beforeTokenSigning handler inside the test function (line 641–646), while earlier suites register handlers at the describe-block scope (lines 48, 293, 485). All handlers accumulate on the shared server.service instance with no cleanup; when tokens are signed, all registered listeners fire in order, with each potentially overwriting token payload fields. This causes the new handler here to override the email for subsequent test executions if handlers persist across suite boundaries, breaking the provisioning suite's expectation of email: "sso-user@localhost:8000.com" (line 550).
Move the handler registration into beforeAll and clear previous listeners before attaching the new one:
beforeAll(async () => {
await server.issuer.keys.generate("RS256");
server.service.removeAllListeners("beforeTokenSigning");
server.service.on("beforeTokenSigning", (token) => {
token.payload.email = testEmail;
token.payload.email_verified = false;
token.payload.name = "Domain Verified User";
token.payload.sub = "oidc-domain-verified-user";
});
await server.start(8080, "localhost");
});This ensures each suite has predictable token payloads without interference from other suites' handlers.
🤖 Prompt for AI Agents
packages/sso/src/oidc.test.ts lines 575-736: the test registers a server.service
"beforeTokenSigning" handler inside the test body which accumulates with other
suite-level handlers and can contaminate other tests; fix by removing any
existing "beforeTokenSigning" listeners and registering this handler in the
suite's beforeAll (after generating keys and before server.start) so it is
isolated to this describe block and does not persist across suites.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.