feat: Refactor authorization system to use ABAC pattern with string-based roles#719
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: a1ca70f The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughRefactors authorization system from role-based to ABAC pattern. Replaces Changes
Sequence DiagramssequenceDiagram
actor Client
participant GraphQL as GraphQL Resolver
participant FAP as FieldAuthorizePlugin
participant AuthUtils as Authorizer Utils
participant DB as Database
participant Context as ServiceContext
Client->>GraphQL: Field Access
GraphQL->>FAP: wrapResolve(authorize, source, context)
FAP->>FAP: authorizeAccess(authorize, source, context)
alt Single rule or array
FAP->>AuthUtils: checkInstanceAuthorization(ctx, instance, rules)
AuthUtils->>Context: Check authorizerCache
alt Cache hit
AuthUtils-->>FAP: Return (cached result)
else Cache miss
alt Global rules (string)
AuthUtils->>AuthUtils: Match global roles
else Instance rules (function)
AuthUtils->>Context: Lazily resolve instance
alt Instance provided
AuthUtils->>AuthUtils: Use provided instance
else Instance loader
AuthUtils->>DB: Load instance via loader
DB-->>AuthUtils: Instance or null
end
AuthUtils->>AuthUtils: Execute instance check(ctx, instance)
end
AuthUtils->>Context: Cache result in authorizerCache
AuthUtils-->>FAP: Return or throw ForbiddenError
end
end
alt Authorization passed
FAP->>GraphQL: Proceed to resolver
GraphQL-->>Client: Result
else Authorization failed
FAP-->>Client: ForbiddenError
end
sequenceDiagram
actor User
participant DataOp as Data Operation (Update/Delete)
participant AuthUtils as Authorizer Utils
participant DB as Database
participant Context as ServiceContext
User->>DataOp: Invoke operation with config.authorize
DataOp->>DataOp: Check if authorize array non-empty
alt authorize present and non-empty
rect rgb(200, 220, 255)
note over DataOp,Context: Lazy Instance Loading
DataOp->>DataOp: Prepare loadExisting callback
end
DataOp->>AuthUtils: checkInstanceAuthorization(ctx, loadExisting, authorize)
AuthUtils->>Context: Check authorizerCache for this rule+id
alt Cache hit
AuthUtils-->>DataOp: Return (cached)
else Cache miss
AuthUtils->>AuthUtils: Filter: global checks vs instance checks
alt Global role checks
AuthUtils->>AuthUtils: Check user roles
else Instance checks
AuthUtils->>DataOp: Call loadExisting (lazy)
DataOp->>DB: Fetch existing record
DB-->>DataOp: Record
DataOp-->>AuthUtils: Instance data
AuthUtils->>AuthUtils: Execute check(ctx, instance)
end
AuthUtils->>Context: Cache authorizerCache[key] = result
AuthUtils-->>DataOp: Return or throw ForbiddenError
end
end
alt Authorization passed or skipped
DataOp->>DB: Execute operation
DB-->>DataOp: Result
DataOp-->>User: Success
else Authorization failed
DataOp-->>User: ForbiddenError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
examples/todo-with-auth0/apps/backend/src/utils/data-operations/define-operations.ts (3)
387-415: JSDoc example is outdated.The example in the
defineCreateOperationJSDoc still shows the old function-basedauthorizepattern:authorize: async (data, ctx) => { // Check if user has permission to create },This should be updated to match the new array-based pattern:
authorize: ['admin', 'system'],🔎 Suggested fix
* @example * ```typescript * const createUser = defineCreateOperation({ * model: 'user', * fields: { * name: scalarField(z.string()), * email: scalarField(z.email()), * }, - * authorize: async (data, ctx) => { - * // Check if user has permission to create - * }, + * authorize: ['admin', 'system'], * create: ({ tx, data, query, serviceContext }) =>
673-703: JSDoc example for update is outdated.Similar to the create operation, the
defineUpdateOperationJSDoc example still shows the old function-based authorization pattern. As per coding guidelines forexamples/todo-with-auth0/**/*.{ts,tsx}, JSDocs should be accurate.🔎 Suggested fix
* @example * ```typescript * const updateUser = defineUpdateOperation({ * model: 'user', * fields: { * name: scalarField(z.string()), * email: scalarField(z.email()), * }, - * authorize: async (data, ctx) => { - * const existing = await ctx.loadExisting(); - * // Check if user owns this record - * }, + * authorize: ['admin', userAuthorizer.roles.owner], * update: ({ tx, where, data, query, serviceContext }) =>
945-974: JSDoc example for delete is outdated.The
defineDeleteOperationJSDoc example also shows the old function-based pattern.🔎 Suggested fix
* @example * ```typescript * const deleteUser = defineDeleteOperation({ * model: 'user', - * authorize: async (ctx) => { - * const existing = await ctx.loadExisting(); - * // Check if user has permission to delete - * }, + * authorize: ['admin', userAuthorizer.roles.owner], * delete: ({ tx, where, query, serviceContext }) =>examples/blog-with-auth/apps/backend/src/utils/data-operations/define-operations.ts (3)
387-415: JSDoc example is outdated.Same issue as in the todo-with-auth0 file - the
defineCreateOperationJSDoc example shows the old function-basedauthorizepattern instead of the new array-based pattern.
673-703: JSDoc example for update is outdated.The
defineUpdateOperationJSDoc example still shows the old function-based authorization pattern.
945-974: JSDoc example for delete is outdated.The
defineDeleteOperationJSDoc example also shows the old function-based pattern.
🧹 Nitpick comments (6)
examples/todo-with-auth0/apps/backend/src/utils/service-context.ts (3)
5-11: Add JSDocs to the ServiceContext interface.The ServiceContext interface and its new cache fields should be documented with JSDocs explaining their purpose, particularly the authorization caching mechanism.
📝 Suggested JSDoc documentation
+/** + * Service context for a single request, containing authentication info and per-request caches. + */ export interface ServiceContext { /* TPL_CONTEXT_INTERFACE:START */ auth: AuthContext; + /** Cache for global/instance authorization check results to avoid redundant checks within a request */ authorizerCache: Map<string, boolean>; + /** Cache for model instances used in authorization checks to avoid redundant database queries */ authorizerModelCache: Map<string, unknown>; /* TPL_CONTEXT_INTERFACE:END */ }Based on coding guidelines for examples/todo-with-auth0/**/*.{ts,tsx}.
13-25: Add JSDocs to the createServiceContext function.The createServiceContext function should include JSDoc documentation explaining its purpose, parameters, and return value.
📝 Suggested JSDoc documentation
+/** + * Creates a service context for a request. + * + * @param params - The parameters for creating the service context + * @param params.auth - The authentication context for the request + * @returns A new service context with initialized caches + */ export function createServiceContext( /* TPL_CREATE_CONTEXT_ARGS:START */ { auth, }: { auth: AuthContext; } /* TPL_CREATE_CONTEXT_ARGS:END */, ): ServiceContext {Based on coding guidelines for examples/todo-with-auth0/**/*.{ts,tsx}.
8-9: Consider more specific typing for authorizerModelCache.The
authorizerModelCacheusesMap<string, unknown>which provides minimal type safety. While this may be intentional for flexibility, consider whether a union type of expected model types or a generic constraint could improve type safety.For example:
authorizerModelCache: Map<string, Record<string, unknown>>;This ensures cached values are at least object-like, which aligns with the model instance caching use case.
examples/blog-with-auth/apps/backend/src/plugins/graphql/FieldAuthorizePlugin/index.ts (1)
37-47: Add explicit return type toauthorizeAccessmethod.Per coding guidelines for TypeScript files, all functions should have explicit return types. The method is missing the return type annotation.
🔎 Proposed fix
async authorizeAccess( // eslint-disable-next-line @typescript-eslint/no-explicit-any authorize: AuthorizeRoleRuleOption<any>, root: unknown, context: Types['Context'], - ): Promise<void> { + ): Promise<void> {The return type
Promise<void>is already present - disregard this comment.Actually, looking more carefully at line 42, the return type
Promise<void>is specified. The implementation correctly delegates tocheckInstanceAuthorizationwhich throwsForbiddenErroron authorization failure.LGTM!
The refactored
authorizeAccessmethod correctly delegates to the centralizedcheckInstanceAuthorizationutility, simplifying the authorization flow.examples/todo-with-auth0/apps/backend/src/plugins/graphql/FieldAuthorizePlugin/types.ts (1)
31-33: Add JSDoc toAuthorizeRolePluginOptionsinterface.Per coding guidelines for
examples/todo-with-auth0/**/*.ts, all exported interfaces should have JSDocs documenting their purpose and fields.🔎 Proposed fix
+/** + * Configuration options for the authorize-by-roles plugin. + */ export interface AuthorizeRolePluginOptions { + /** + * When true, requires all root Query/Mutation/Subscription fields to have an authorize option. + */ requireOnRootFields?: boolean; }examples/todo-with-auth0/apps/backend/src/utils/authorizers.ts (1)
261-295: Well-implemented two-level caching strategy.The function correctly implements lazy loading with separate caches for role check results and model instances. The caching of null models prevents redundant database queries for non-existent entities.
Minor: Consider more explicit null check
Line 287 could use an explicit null check for clarity:
- if (!model) { + if (model === null) { // Cache negative result for missing models ctx.authorizerCache.set(cacheKey, false); return false; }This makes it clearer that we're specifically checking for the "not found" case, though the current code is functionally correct since
undefinedis already handled above.
| import type { AuthRole } from '../modules/accounts/auth/constants/auth-roles.constants.js'; | ||
| import type { | ||
| GetPayload, | ||
| ModelPropName, | ||
| } from './data-operations/prisma-types.js'; | ||
| import type { ServiceContext } from './service-context.js'; | ||
|
|
||
| import { ForbiddenError } from './http-errors.js'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use @src/ alias for import paths.
According to the coding guidelines, import statements should use absolute paths via tsconfig paths (@src/ is the alias for src/).
🔎 Proposed fix
-import type { AuthRole } from '../modules/accounts/auth/constants/auth-roles.constants.js';
+import type { AuthRole } from '@src/modules/accounts/auth/constants/auth-roles.constants.js';
import type {
GetPayload,
ModelPropName,
-} from './data-operations/prisma-types.js';
-import type { ServiceContext } from './service-context.js';
+} from '@src/utils/data-operations/prisma-types.js';
+import type { ServiceContext } from '@src/utils/service-context.js';
-import { ForbiddenError } from './http-errors.js';
+import { ForbiddenError } from '@src/utils/http-errors.js';Based on coding guidelines.
📝 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.
| import type { AuthRole } from '../modules/accounts/auth/constants/auth-roles.constants.js'; | |
| import type { | |
| GetPayload, | |
| ModelPropName, | |
| } from './data-operations/prisma-types.js'; | |
| import type { ServiceContext } from './service-context.js'; | |
| import { ForbiddenError } from './http-errors.js'; | |
| import type { AuthRole } from '@src/modules/accounts/auth/constants/auth-roles.constants.js'; | |
| import type { | |
| GetPayload, | |
| ModelPropName, | |
| } from '@src/utils/data-operations/prisma-types.js'; | |
| import type { ServiceContext } from '@src/utils/service-context.js'; | |
| import { ForbiddenError } from '@src/utils/http-errors.js'; |
🤖 Prompt for AI Agents
In examples/todo-with-auth0/apps/backend/src/utils/authorizers.ts lines 1-8, the
imports use relative paths but must use the project alias @src/ per tsconfig
paths; update each import to use the absolute alias (for example change import
type { AuthRole } from
'../modules/accounts/auth/constants/auth-roles.constants.js' to import type {
AuthRole } from '@src/modules/accounts/auth/constants/auth-roles.constants.js')
and do the same for the GetPayload/ModelPropName and ServiceContext imports,
preserving the exported names and file extensions; no other logic changes
required.
| export async function checkInstanceAuthorization<T>( | ||
| ctx: ServiceContext, | ||
| instance: T | (() => Promise<T>), | ||
| authorize: (InstanceRoleCheck<T> | GlobalRoleCheck)[], | ||
| ): Promise<void> { | ||
| const resolvedInstance = | ||
| typeof instance === 'function' | ||
| ? await (instance as () => Promise<T>)() | ||
| : instance; | ||
|
|
||
| // Run instance checks | ||
| for (const check of authorize) { | ||
| if (await check(ctx, resolvedInstance)) return; | ||
| } | ||
|
|
||
| throw new ForbiddenError('Forbidden'); | ||
| } |
There was a problem hiding this comment.
Type safety issue with mixed check types.
The function signature accepts both InstanceRoleCheck<T> and GlobalRoleCheck in the authorize array (line 45), but line 54 calls check(ctx, resolvedInstance) with 2 arguments. GlobalRoleCheck only accepts 1 argument (ctx: ServiceContext) => boolean, which creates a type mismatch.
While JavaScript will ignore the extra argument at runtime, this violates type safety. Consider one of these approaches:
- Remove
GlobalRoleCheckfrom the union type on line 45 - Add conditional logic to call checks with the correct number of arguments based on type
- Use function overloads to handle both cases separately
🔎 Proposed fix: Remove GlobalRoleCheck from union type
export async function checkInstanceAuthorization<T>(
ctx: ServiceContext,
instance: T | (() => Promise<T>),
- authorize: (InstanceRoleCheck<T> | GlobalRoleCheck)[],
+ authorize: InstanceRoleCheck<T>[],
): Promise<void> {📝 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.
| export async function checkInstanceAuthorization<T>( | |
| ctx: ServiceContext, | |
| instance: T | (() => Promise<T>), | |
| authorize: (InstanceRoleCheck<T> | GlobalRoleCheck)[], | |
| ): Promise<void> { | |
| const resolvedInstance = | |
| typeof instance === 'function' | |
| ? await (instance as () => Promise<T>)() | |
| : instance; | |
| // Run instance checks | |
| for (const check of authorize) { | |
| if (await check(ctx, resolvedInstance)) return; | |
| } | |
| throw new ForbiddenError('Forbidden'); | |
| } | |
| export async function checkInstanceAuthorization<T>( | |
| ctx: ServiceContext, | |
| instance: T | (() => Promise<T>), | |
| authorize: InstanceRoleCheck<T>[], | |
| ): Promise<void> { | |
| const resolvedInstance = | |
| typeof instance === 'function' | |
| ? await (instance as () => Promise<T>)() | |
| : instance; | |
| // Run instance checks | |
| for (const check of authorize) { | |
| if (await check(ctx, resolvedInstance)) return; | |
| } | |
| throw new ForbiddenError('Forbidden'); | |
| } |
kingston
deleted the
kingston/eng-955-update-authorization-framework-utilities-and-types
branch
1 participant
Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.