Authentication Identity facade service
#3437
Conversation
In ~/core. Migrate SessionHost consumers to this facade.
This moves all session creations into the service.
This should be replaced with something else later. Other entry points (migrations, etc) should declare something explicit.
📝 Walkthrough## Walkthrough
This change refactors the codebase to centralize session and identity management through a new `Identity` service, replacing direct usage of `SessionHost` and related session utilities. All session access, user context checks, and role-based execution now use the `Identity` abstraction. Supporting code, dependency injection, and method signatures are updated accordingly across modules and services.
## Changes
| Files/Paths (grouped) | Change Summary |
|---------------------------------------------------------------------------------------------------------------|---------------|
| `src/core/authentication/identity.service.ts`, `src/core/authentication/index.ts` | Introduced and exported a new `Identity` service class, providing centralized session and identity access, role/user context execution, and admin/self checks. |
| `src/common/session.ts` | Removed all session-related runtime logic, decorators, and utility functions (`loggedInSession`, `SessionPipe`, `LoggedInSession`, `AnonSession`, `addScope`, `isAdmin`); now only exports the `Session` interface and types. |
| `src/components/authentication/authentication.module.ts` | Added `Identity` to providers and exports arrays, making it injectable and available across modules; removed `SessionPipe` provider. |
| `src/components/authentication/authentication.service.ts` | Added `asRole<R>(role, fn)` method to execute functions under a synthetic session for a specific role. |
| `src/components/authorization/policy/executor/edge-privileges.ts`, `src/components/authorization/policy/executor/resource-privileges.ts` | Changed session context for permission checks from `SessionHost` to `Identity`. |
| `src/components/authorization/policy/executor/policy-dumper.ts` | Refactored to use `identity.asRole` for permission resolution; updated constructor to inject `Identity`. |
| `src/components/authorization/policy/executor/policy-executor.ts`, `src/components/authorization/policy/executor/privileges.ts` | Replaced all `SessionHost` usage with `Identity` for session context; updated constructors accordingly. |
| `src/components/comments/comment.service.ts` | Replaced `SessionHost` and `isAdmin` with `Identity` for thread security and admin checks. |
| `src/components/engagement/engagement.rules.ts` | Switched from `SessionHost` to `Identity` for session context in engagement rules. |
| `src/components/file/file.service.ts` | Added `@Inject(forwardRef(() => MediaService))` to `mediaService` parameter for circular dependency resolution. |
| `src/components/file/media/media.service.ts` | Changed import path for `IEventBus` to `~/core/events`. |
| `src/components/notification-system/system-notification.resolver.ts` | Replaced session-based admin check with privilege-based authorization; injected `Privileges` service; removed session parameter from mutation. |
| `src/components/periodic-report/periodic-report.resolver.ts` | Removed session parameters and admin/anonymous checks from resolver signatures; injected `Identity` and enforce admin check internally. |
| `src/components/periodic-report/periodic-report.service.ts` | Injected `Identity`; no other logic changes. |
| `src/components/post/post.service.ts` | Injected `Identity`; added anonymous user check to return redacted list early. |
| `src/components/post/postable.resolver.ts` | Removed session parameter and anonymous session decorator; removed anonymous user check. |
| `src/components/progress-report/media/handlers/update-media-metadata-check.handler.ts` | Removed `SessionHost` dependency from constructor. |
| `src/components/progress-report/workflow/handlers/progress-report-workflow-notification.handler.ts` | Replaced `AuthenticationService` with `Identity` for user context execution. |
| `src/components/project/project.service.ts` | Switched from `SessionHost`/`isAdmin` to `Identity` for impersonator admin checks and session context. |
| `src/components/project/workflow/handlers/project-workflow-notification.handler.ts` | Unified user/session context management under `Identity`, replacing `AuthenticationService` and `SessionHost`. |
| `src/components/prompts/prompt-variant-response.service.ts` | Replaced `SessionHost` with `Identity` for session context. |
| `src/components/user/education/education.resolver.ts` | Changed class decorator from `@Injectable()` to `@Resolver()`. |
| `src/components/user/education/education.service.ts`, `src/components/user/unavailability/unavailability.service.ts` | Replaced `SessionHost` with `Identity`; updated self-check logic in update methods. |
| `src/components/user/user.service.ts` | Changed injected session context from `SessionHost` to `Identity`. |
| `src/components/workflow/permission.serializer.ts`, `src/components/workflow/workflow.service.ts` | Refactored privilege checks and serialization to use `Identity` instead of `SessionHost` and direct session objects. |
| `src/core/data-loader/data-loader.config.ts` | Changed session context injection from `SessionHost` to `Identity`. |
| `src/core/database/database.service.ts` | Replaced `SessionHost` with injected `Identity` (using `forwardRef`); updated user context in queries. |
| `src/core/gel/gel.service.ts` | Changed session context and cloning logic from `SessionHost` to `Identity`. |
| `src/core/graphql/graphql-tracing.plugin.ts` | Changed session context injection from `SessionHost` to `Identity`. |
| `src/core/resources/resource.loader.ts` | Replaced `AuthenticationService` with `Identity`; updated CLI readiness logic. |
| `src/components/authentication/session.interceptor.ts` | Replaced `loggedInSession` verification with `Identity.verifyLoggedIn` method; injected `Identity`. |
| `src/components/comments/comment-thread.resolver.ts` | Removed session parameter and decorator; replaced logged-in verification with `Identity.verifyLoggedIn`. |
| `src/components/file/file-url.controller.ts` | Replaced session verification function with `Identity.verifyLoggedIn`; injected `Identity`. |
| `src/components/authentication/extra-info.resolver.ts` | Removed session parameter and decorator from method signature. |
| `src/components/authentication/login.resolver.ts` | Removed session parameter from logout method; injected `SessionHost` to retrieve current session internally. |
| `src/components/changeset/changeset-aware.resolver.ts`, `src/components/changeset/changeset.resolver.ts` | Removed session parameters; injected `Identity`; replaced anonymous checks with `Identity.isAnonymous`. |
| `src/components/comments/commentable.resolver.ts` | Injected `Identity`; removed session parameter; replaced anonymous check with `Identity.isAnonymous`. |
| `src/components/file/file-node.resolver.ts` | Removed session parameter from `parents` resolver method. |
| `src/components/notifications/notification.resolver.ts` | Injected `Identity`; removed session parameter; replaced anonymous check with `Identity.isAnonymous`. |
| `src/components/pin/pin.resolver.ts` | Injected `Identity`; removed session parameter; replaced anonymous check with `Identity.isAnonymous`. |
| `src/components/product-progress/create-product-connection.resolver.ts` | Injected `Identity`; removed session parameter; replaced anonymous check with `Identity.isAnonymous`. |
| `src/components/project/financial-approver/financial-approver.resolver.ts` | Injected `Identity`; removed session parameter; replaced anonymous check with `Identity.isAnonymous`. |
| `src/components/user/assignable-roles.resolver.ts` | Removed session parameter from resolver method. |
| `src/components/user/user.resolver.ts` | Injected `Identity`; removed session parameter; replaced anonymous check with `Identity.isAnonymous`. |
| `src/common/index.ts` | Removed exports of `LoggedInSession` and `AnonSession` from the session barrel; only `Session` type exported. |
## Possibly related PRs
- [SeedCompany/cord-api-v3#3427](https://github.com/SeedCompany/cord-api-v3/pull/3427): Introduces the `SessionHost` service using AsyncLocalStorage for global session context, which is directly refactored and superseded by the new `Identity` abstraction in this PR.Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/core/authentication/identity.service.ts (5)
12-15: Consider simplifying theAuthenticationServiceinjectionThe
& {}empty intersection in the type definition forauthseems unnecessary and could be removed for clarity.-private readonly auth: AuthenticationService & {}, +private readonly auth: AuthenticationService,
29-61: Consider using@deprecatedfor discouraged APIsWhile you explicitly document that these properties are "not the best API" and "use is discouraged", it would be more effective to mark them with the
@deprecateddecorator. This would provide IDE warnings to developers using these properties and make the migration path clearer./** * Is the current requestor anonymous (not logged in)? * * Not the best API, use is discouraged. * Prefer using Auth Policies / {@link Privileges}`.for.can()` */ +@deprecated('Use Auth Policies or Privileges.for.can() instead') get isAnonymous() { return this.current.anonymous; }
77-82: Fix grammatical error in commentThe method comment has a small grammatical error.
/** - * Run this function with the current user as an ephemeral one this role + * Run this function with the current user as an ephemeral one with this role */ asRole<R>(role: Role, fn: () => R): R { return this.auth.asRole(role, fn); }
1-4: Consider consistent import pathsThe file mixes relative paths (
'../../components/authentication') with absolute paths ('~/common'). Consider using absolute paths consistently for better maintainability.import { forwardRef, Inject, Injectable } from '@nestjs/common'; import type { ID, Role } from '~/common'; -import { type AuthenticationService } from '../../components/authentication'; +import { type AuthenticationService } from '~/components/authentication'; import { SessionHost } from '../../components/authentication/session.host';
1-91: Overall feedback on the Identity service designThe Identity service effectively centralizes authentication functionality while enabling dependency injection and breaking circular dependencies. The facade pattern helps decouple the rest of the codebase from authentication implementation details.
Note that you're creating properties that are immediately documented as discouraged. Consider whether these should be included at all, or if they should be protected/private with alternative public methods that encourage the policy-based approach.
src/components/project/workflow/handlers/project-workflow-notification.handler.ts (2)
35-36: Mismatched logger nameThe logger name
'progress-report:status-change-notifier'doesn't match the component (project workflow). Consider using a more appropriate name like'project:workflow:notification-handler'.-@Logger('progress-report:status-change-notifier') +@Logger('project:workflow:notification-handler')
109-109: Consider more explicit handling of external email recipientsThe current pattern of using the root user ID as a fallback when
notifier.idis null creates an implicit design where external email recipients are processed differently from user recipients. Consider creating a more explicit design, perhaps with a dedicated type for email recipients.type EmailRecipient = { type: 'user' | 'external'; id?: ID<'User'>; email: string; // other properties }This would make the distinction between user and external recipients more explicit in the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
src/common/session.ts(2 hunks)src/components/authentication/authentication.module.ts(3 hunks)src/components/authentication/authentication.service.ts(1 hunks)src/components/authorization/policy/executor/edge-privileges.ts(1 hunks)src/components/authorization/policy/executor/policy-dumper.ts(3 hunks)src/components/authorization/policy/executor/policy-executor.ts(4 hunks)src/components/authorization/policy/executor/privileges.ts(4 hunks)src/components/authorization/policy/executor/resource-privileges.ts(1 hunks)src/components/comments/comment.service.ts(3 hunks)src/components/engagement/engagement.rules.ts(5 hunks)src/components/file/file.service.ts(2 hunks)src/components/file/media/media.service.ts(1 hunks)src/components/notification-system/system-notification.resolver.ts(2 hunks)src/components/periodic-report/periodic-report.resolver.ts(1 hunks)src/components/periodic-report/periodic-report.service.ts(3 hunks)src/components/post/post.service.ts(3 hunks)src/components/post/postable.resolver.ts(1 hunks)src/components/progress-report/media/handlers/update-media-metadata-check.handler.ts(0 hunks)src/components/progress-report/workflow/handlers/progress-report-workflow-notification.handler.ts(3 hunks)src/components/project/project.service.ts(5 hunks)src/components/project/workflow/handlers/project-workflow-notification.handler.ts(5 hunks)src/components/prompts/prompt-variant-response.service.ts(3 hunks)src/components/user/education/education.resolver.ts(2 hunks)src/components/user/education/education.service.ts(3 hunks)src/components/user/unavailability/unavailability.service.ts(3 hunks)src/components/user/user.service.ts(3 hunks)src/components/workflow/permission.serializer.ts(2 hunks)src/components/workflow/workflow.service.ts(3 hunks)src/core/authentication/identity.service.ts(1 hunks)src/core/authentication/index.ts(1 hunks)src/core/data-loader/data-loader.config.ts(2 hunks)src/core/database/database.service.ts(4 hunks)src/core/gel/gel.service.ts(4 hunks)src/core/graphql/graphql-tracing.plugin.ts(2 hunks)src/core/resources/resource.loader.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/components/progress-report/media/handlers/update-media-metadata-check.handler.ts
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/components/prompts/prompt-variant-response.service.ts (1)
src/components/authentication/session.resolver.ts (1)
session(42-96)
src/components/authorization/policy/executor/privileges.ts (1)
src/components/authentication/session.resolver.ts (1)
session(42-96)
src/components/authorization/policy/executor/policy-executor.ts (1)
src/components/authentication/session.resolver.ts (1)
session(42-96)
src/components/periodic-report/periodic-report.service.ts (1)
src/common/exceptions/unauthorized.exception.ts (1)
UnauthorizedException(9-102)
src/components/authentication/authentication.service.ts (2)
src/components/authentication/session.resolver.ts (1)
session(42-96)src/common/id-field.ts (1)
ID(24-25)
src/components/project/project.service.ts (1)
src/components/authentication/session.resolver.ts (1)
session(42-96)
src/components/engagement/engagement.rules.ts (1)
src/components/authentication/session.resolver.ts (1)
session(42-96)
src/components/project/workflow/handlers/project-workflow-notification.handler.ts (1)
src/components/authentication/session.resolver.ts (1)
session(42-96)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: Generate (head)
- GitHub Check: lint
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Generate (base)
- GitHub Check: Clean
- GitHub Check: Unit
🔇 Additional comments (70)
src/components/file/media/media.service.ts (1)
12-12: Import path updated to use more specific moduleThis change correctly updates the import path for
IEventBusfrom the general core module to the more specific events module. This aligns with the overall refactoring goal of improving modularity.src/core/authentication/index.ts (1)
1-1: Good facade pattern implementationThis export statement properly exposes the new
Identityservice through the authentication module, following the facade pattern described in the PR objectives. This allows other parts of the application to use theIdentityservice without knowing its implementation details.src/components/user/education/education.resolver.ts (1)
1-1: Improved GraphQL resolver patternThe change from
@Injectable()to@Resolver()aligns with NestJS best practices for GraphQL resolvers. The@Resolver()decorator is more specific and semantically correct for this use case, properly designating this class as a GraphQL resolver.Also applies to: 16-16
src/components/authorization/policy/executor/edge-privileges.ts (1)
60-60: Updated to use Identity service instead of SessionHostThis change follows the PR objective of decoupling authentication by using the new
Identityfacade instead of directly accessing theSessionHost. This centralization of session management will improve maintainability.src/components/prompts/prompt-variant-response.service.ts (3)
19-19: Import change reflects architecture refactoringThe import change from SessionHost to Identity aligns with the PR objective of centralizing authentication functionality through the new Identity service facade.
56-56: DI updated to use new Identity facadeDependency injection correctly updated to use the new Identity service instead of SessionHost.
233-233: Session access refactored to use Identity serviceAccess to the current session has been properly migrated from SessionHost to the Identity facade, maintaining the same functionality while improving architectural organization.
src/components/authorization/policy/executor/resource-privileges.ts (1)
109-109: Session access refactored through Identity serviceThe change maintains the same behavior while using the new Identity facade to access the current session. This aligns with the authentication centralization effort.
src/components/post/post.service.ts (3)
15-15: Import updated to use Identity facadeThe import change aligns with the PR objective of centralizing authentication functionality through the Identity service.
28-28: DI updated to use Identity serviceDependency injection correctly updated to use the new Identity service.
95-98: New anonymous user check using Identity facadeThis adds a valuable security check using the new Identity service's
isAnonymousproperty. The TODO comment suggests this should eventually be moved to an authorization policy for better separation of concerns.Consider implementing this as an authorization policy rule as noted in the TODO comment, which would:
- Maintain a cleaner separation of concerns
- Centralize authorization logic
- Apply the rule consistently across similar scenarios
src/components/user/user.service.ts (3)
12-12: Import updated to use Identity facadeThe import change aligns with the PR objective of centralizing authentication functionality.
64-64: DI updated to use Identity serviceDependency injection correctly updated to use the new Identity service instead of SessionHost.
80-80: Session presence check refactored to use IdentityThe change maintains the same behavior while leveraging the Identity service's API. This check is important for the role assignment security check that follows.
src/components/workflow/workflow.service.ts (1)
4-4: Appropriate migration to Identity service.The changes correctly replace the SessionHost dependency with the new Identity service from the core authentication module. This aligns with the PR objective of centralizing authentication functionality.
Also applies to: 23-23, 146-146
src/core/graphql/graphql-tracing.plugin.ts (1)
7-7: Successfully migrated from SessionHost to Identity.The tracing plugin correctly updates both the dependency injection and usage of the identity context. The reference to
currentMaybe?.userIdhas been properly maintained for user tracking in segments.Also applies to: 15-15, 50-50
src/components/authorization/policy/executor/policy-executor.ts (1)
5-5: Properly implemented Identity service across policy execution.The PolicyExecutor class has been successfully updated to use the new Identity service. All references to the current session have been updated, which is critical for this class that handles permission resolution.
Also applies to: 43-43, 67-67, 190-190
src/components/authorization/policy/executor/privileges.ts (1)
9-9: Access control successfully migrated to Identity service.The Privileges class properly adapts to the new Identity facade. All session references are correctly updated, maintaining the same authorization behavior while using the centralized authentication service.
Also applies to: 26-26, 93-93, 105-105
src/components/authentication/authentication.service.ts (1)
259-268: New method looks good and aligns with service responsibilitiesThe implementation of
asRolefollows patterns similar to the existingasUsermethod but is designed for role-based execution context rather than user-based. The synthetic session setup correctly includes the role with global scope.src/components/progress-report/workflow/handlers/progress-report-workflow-notification.handler.ts (3)
12-12: Import updated to use the new Identity serviceThe import change correctly brings in the new
Identityfacade service from the core authentication module.
39-39: Constructor dependency updated from AuthenticationService to IdentityThe constructor parameter has been changed to use the new
Identityservice, which centralizes authentication functionality.
116-116: Method call updated to use Identity serviceThe method call has been updated to use
this.identity.asUserinstead ofthis.auth.asUser, maintaining the same functionality while leveraging the new facade service.src/components/authentication/authentication.module.ts (3)
5-5: Import added for Identity serviceCorrectly imports the new
Identityservice from the core authentication module.
38-38: Added Identity service to providersIdentity service is now registered as a provider in the module, making it available for dependency injection.
49-49: Exported Identity service from moduleThe Identity service is now exported from the module, allowing other modules that import AuthenticationModule to access it.
src/core/database/database.service.ts (3)
1-1: Imports updated for Identity serviceThe imports have been updated to include forwardRef and Inject from NestJS, and the Identity service from the authentication module.
Also applies to: 20-20
67-68: Constructor dependency changed from SessionHost to IdentityThe constructor now injects the Identity service using forward reference instead of SessionHost. The forward reference pattern is correctly used to avoid circular dependencies.
128-128: Updated session access to use Identity serviceThe code now uses
this.identity.currentIfInCtx?.userIdinstead of the previous SessionHost implementation to access the current user ID, maintaining the same functionality.src/components/file/file.service.ts (1)
5-5: Circular dependency resolution with forwardRef is appropriate.Adding the
forwardRefin the dependency injection decorator addresses a circular dependency between FileService and MediaService. This matches NestJS best practices for resolving circular dependencies.Also applies to: 63-64
src/common/session.ts (1)
10-10: Good migration from SessionHost to Identity.The changes to use the new
Identityservice instead ofSessionHostalign with the PR objective of centralizing authentication functionality. The Identity service acts as a facade for authentication operations, making session management more modular and maintainable.Also applies to: 44-44, 47-47
src/components/user/education/education.service.ts (1)
4-4: Improved abstraction using Identity.isSelf()The replacement of direct session user ID comparison with
identity.isSelf()is a cleaner approach. This abstraction encapsulates the user identity verification logic, making the code more maintainable and the authentication checks more consistent across the codebase.Also applies to: 19-19, 50-50
src/components/engagement/engagement.rules.ts (2)
12-12: Successfully migrated session access to Identity service.All the instance accesses to
sessionHost.currenthave been properly migrated to useidentity.current. This change aligns with the PR objective of centralizing authentication logic in the Identity facade service.Also applies to: 38-38, 320-320, 360-360, 372-372
320-336:✅ Verification successful
Verify session logic for available transitions is preserved.
The session checking logic to determine available transitions now uses the Identity service. Since this is workflow-critical code, ensure the behavior remains consistent under the new implementation.
🏁 Script executed:
#!/bin/bash # Verify all sessionHost.current usages have been replaced with identity.current rg "sessionHost\.current" --type tsLength of output: 896
Session logic unchanged via IdentityService proxy
IdentityService.current (in src/core/authentication/identity.service.ts) directly returns sessionHost.current, so using this.identity.current in engagement.rules.ts preserves the original session behavior. No further changes are required.
src/core/data-loader/data-loader.config.ts (3)
7-7: Import change looks good!The import has been updated from
SessionHosttoIdentityas part of the authentication facade refactoring.
14-14: Constructor parameter change is correctThe dependency injection has been properly updated to use the new
Identityservice instead ofSessionHost.
30-30: Session access updated correctlyThe session retrieval logic has been successfully migrated from
sessionHost.currentMaybetoidentity.currentMaybe, maintaining the same functionality while using the new abstraction.src/components/user/unavailability/unavailability.service.ts (3)
4-4: Import updated correctlyThe import has been updated to use the new
Identityservice from the core authentication module.
19-19: Constructor dependency properly updatedThe service now correctly injects the
Identityservice instead ofSessionHost.
49-49: Good authorization improvementThe authorization check has been refactored to use the
isSelfmethod from theIdentityservice, which is cleaner and more maintainable than the previous direct comparison.src/components/periodic-report/periodic-report.service.ts (3)
10-10: New imports are appropriateThe imports for
UnauthorizedExceptionandIdentityhave been correctly added to support the new authorization check.Also applies to: 14-14
40-40: Constructor dependency properly addedThe
Identityservice is now correctly injected into the constructor.
119-123: Admin check implementation looks goodThe implementation correctly enforces that only admins can list periodic reports by checking
identity.isAdmin. The TODO comment acknowledges that this should eventually be moved to an auth policy.src/core/gel/gel.service.ts (4)
9-9: Import updated correctlyThe import has been updated to use the new
Identityservice.
26-26: Constructor updated properlyThe service now correctly injects the
Identityservice instead ofSessionHost.
37-37: Clone method updated correctlyThe
clonemethod has been updated to pass theidentityservice to the new instance.
118-118: Session access updated correctlyThe code now correctly retrieves the current user ID through
identity.currentIfInCtx?.userIdwhile maintaining the same behavior and UUID check.src/components/workflow/permission.serializer.ts (1)
2-3: Well-implemented Identity service integrationThe changes effectively replace direct session manipulation with the new Identity facade, using
identity.asRole()to execute privilege checks in the context of different roles. This aligns perfectly with the PR's goal of centralizing authentication functionality and improves the code's clarity.Also applies to: 14-18
src/components/post/postable.resolver.ts (2)
3-3: Clean removal of session-related importsThe import statement has been simplified, removing unused imports related to session handling, which aligns with the move toward the centralized Identity service approach.
17-38: Improved separation of concernsThe resolver has been simplified by removing session handling logic. The authorization check for anonymous users has likely been moved to the service layer, which is a better separation of concerns. This makes the resolver more focused on its primary responsibility of orchestrating data fetching.
src/components/notification-system/system-notification.resolver.ts (1)
10-10: Effective privilege-based authorization implementationThe code has been improved by replacing direct session-based admin checks with a more declarative privilege-based approach. This change:
- Makes the authorization intent clearer (
verifyCan('create')vs checking admin status)- Improves separation of concerns by delegating authorization logic to the Privileges service
- Maintains the same security characteristics while simplifying the resolver
Also applies to: 25-28, 34-34
src/core/resources/resource.loader.ts (2)
1-1: Appropriate dependency injection changeThe constructor now properly injects the Identity service instead of the previous AuthenticationService, aligning with the PR's goal of centralizing authentication through the Identity facade.
Also applies to: 8-8, 47-47
114-119: Clean implementation of Identity service methodsThe code effectively adapts to the new Identity service interface:
identity.readyForCli()replaces the previous authentication preparation for CLI mode- Context retrieval logic is refactored for clarity while maintaining the same functionality
This change supports the broader refactoring goal while ensuring the ResourceLoader continues to function correctly in both CLI and normal operation modes.
src/components/periodic-report/periodic-report.resolver.ts (2)
9-9: Import dependencies updated to match service layer authorization changes.The resolver now imports only what it needs after moving authorization logic to the service layer.
41-47: Authorization handling moved to service layer.The
periodicReportsquery method signature was simplified by removing the session parameter, aligning with the architectural change that moves authorization checks from resolvers to services. This improves separation of concerns and reduces coupling.src/components/project/project.service.ts (5)
24-24: Migrated to Identity facade for authentication.The service now imports the new
Identityfacade for authentication functionality.
91-91: Injected Identity service instead of SessionHost.Constructor dependency updated to use the new
Identityservice, centralizing authentication and session management.
138-146: Simplified admin authorization check.The admin check is now performed using the more expressive
this.identity.isImpersonatorAdminmethod instead of manually checking session roles.
159-159: Simplified session access.Now using
this.identity.currentto access the current session, which provides a cleaner API than direct session host access.
256-256: Consistent admin authorization pattern.Using
this.identity.isImpersonatorAdminfor admin checks ensures consistent authorization patterns throughout the codebase.src/components/comments/comment.service.ts (3)
15-15: Migrated to Identity facade for authentication.The service now imports the new
Identityfacade for authentication functionality.
41-41: Injected Identity service instead of SessionHost.Constructor dependency updated to use the new
Identityservice, centralizing authentication and session management.
124-124: Improved readability of access control logic.The thread deletion permission check now uses more semantic methods
identity.isSelf()andidentity.isAdmin, making the code's intent clearer than direct session property checks.src/components/authorization/policy/executor/policy-dumper.ts (3)
22-26: Updated imports to use Identity facade.Streamlined imports to use the new
Identityservice while removing now-unused session-related imports.
42-42: Injected Identity service for role-based execution.Constructor now injects the
Identityservice to support contextual execution of permission checks.
171-209: Simplified role-based execution using Identity.Refactored to use
identity.asRole()instead of manually constructing session objects with specific roles. This centralizes the logic for role-based execution and reduces the chance of inconsistencies in how sessions are created across the codebase.src/components/project/workflow/handlers/project-workflow-notification.handler.ts (5)
12-12: LGTM! Good replacement of AuthenticationService with IdentityThe import and injection of the new Identity service looks correct, replacing direct use of AuthenticationService.
Also applies to: 29-29
43-43: Session access through Identity looks goodCorrectly retrieving the current session through the Identity facade instead of directly from SessionHost.
76-84: Execution as root user looks correctThe switch to using Identity for user context switching is properly implemented.
110-136: Identity asUser implementation looks goodThe switch to using Identity for template property resolution looks correct.
57-63:❌ Incorrect review comment
Consider checking for anonymous sessions
When filtering notifyees, the code assumes
session.userIdis present, but doesn't check if the session is anonymous. Consider adding a check:.filter( (n) => // Not current user - n.id !== session.userId && + (!session.anonymous && n.id !== session.userId) && // Only email notifications right now n.email, );
🏁 Script executed:
#!/bin/bash # Check if anonymous sessions are handled elsewhere or if this is a potential issue # Search for anonymous session handling in notification handlers echo "Checking for anonymous session handling in notification handlers..." rg -A 2 "session\.anonymous" --type ts src/components/project/workflow/handlers/ # Search for similar patterns in related notification handlers echo "Checking similar patterns in other notification handlers..." fd "notification.handler.ts" --type f --exec rg -A 5 "filter.*session.*userId" {} \;Length of output: 444
I’d like to inspect how
sessionis defined and where it comes from to confirm whether ananonymousflag exists or is handled elsewhere. Let’s pull in the file context and look for aSessiontype:
🏁 Script executed:
#!/bin/bash # Show imports and Session usage in the handler echo "=== File head ===" sed -n '1,100p' src/components/project/workflow/handlers/project-workflow-notification.handler.ts # Search globally for a Session interface/type definition echo "=== Searching for Session definition ===" rg --context=3 "interface Session" -t ts rg --context=3 "type Session" -t ts # Search the entire codebase for any 'anonymous' mentions echo "=== Searching for 'anonymous' ===" rg --context=3 "anonymous" -t tsLength of output: 34333
No anonymous‐session guard needed in this handler
After digging in:
- The
Sessioninterface does include ananonymousflag, but this event handler only ever runs under authenticated (or system/root) contexts—anonymous sessions are blocked at the GraphQL layer or run as a non‐anonymous “root” insideidentity.asUser.- No other handlers in
*.handler.tsfiles check forsession.anonymous. Adding this guard here would be inconsistent with the rest of our event‐handler patterns.You can safely ignore the suggestion to filter on
session.anonymous.Likely an incorrect or invalid review comment.
These were chaos. NodeJS just exited, no error or logs. This crash happened in `Injector.loadInstance` on the returning the `instanceHost.donePromise`. I believe the cycle check right above it should have caught this, but it didn't. I'm clueless on how node just exited too... somehow a deadlock or empty event loop somehow?
CarsonF
force-pushed
the
refactor/identity
branch
from
d79b81f to
14b8a78
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/components/file/file-url.controller.ts (2)
41-41: Verify if session parameter is necessaryThe session parameter is being passed to
verifyLoggedIn(), but the Identity service likely already has access to the current session context.Consider if this can be simplified to:
- this.identity.verifyLoggedIn(session); + this.identity.verifyLoggedIn();This would be more consistent with the usage in other files and better encapsulate session management within the Identity service.
44-44: TODO could be addressed with the Identity serviceThis TODO comment about authorization using session could potentially be addressed now using the new Identity service's authorization capabilities.
Would you like me to suggest an implementation approach for handling authorization using the Identity service?
src/components/authentication/session.interceptor.ts (1)
38-90: Consider refactoring session handling for better separation of concernsThe SessionInterceptor is still doing a lot of complex session management in conjunction with the Identity service. Now that the Identity facade is in place, consider if some of this session hydration and management logic should be migrated into the Identity service for better encapsulation.
This would simplify the interceptor and further consolidate authentication logic in the Identity service, reinforcing its role as the central facade for authentication.
src/components/comments/comment-thread.resolver.ts (1)
47-48: Consider implementing the TODO to move authentication to policyNow that the Identity service is available, consider implementing the TODO comment about moving this authentication check to an auth policy, which would better separate concerns.
This aligns with your efforts to centralize authentication and would help decouple authorization logic from business logic in the resolver.
src/common/session.ts (1)
34-60: Consider updating deprecation notices with migration guidanceSince the decorators are marked as deprecated, consider enhancing the JSDoc comments to explain how developers should use the Identity service instead.
For example:
- /** @deprecated */ + /** + * @deprecated Use dependency injection with Identity service instead. + * Example: constructor(private readonly identity: Identity) {} + * Then use this.identity.verifyLoggedIn() where needed. + */ export const LoggedInSession = () => AnonSession();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/common/session.ts(2 hunks)src/components/authentication/session.interceptor.ts(3 hunks)src/components/comments/comment-thread.resolver.ts(3 hunks)src/components/file/file-url.controller.ts(3 hunks)src/core/authentication/identity.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/authentication/identity.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/file/file-url.controller.ts (1)
src/components/authentication/session.resolver.ts (1)
session(42-96)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: Unit
- GitHub Check: Generate (head)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: Clean
- GitHub Check: Generate (base)
- GitHub Check: lint
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
src/components/file/file-url.controller.ts (2)
13-13: Appropriate import for the new Identity facadeThe import of the Identity facade from the core authentication module shows good organization, aligning with the PR's goal of centralizing authentication functionality.
26-26: Good dependency injection patternProperly injecting the Identity service as a private readonly member follows best DI practices and allows for easier testing.
src/components/authentication/session.interceptor.ts (3)
25-25: Appropriate import for the new Identity facadeThe import of the Identity facade from the core authentication module shows good organization, aligning with the PR's goal of centralizing authentication functionality.
39-39: Good dependency injection patternProperly injecting the Identity service as a private readonly member follows best DI practices and allows for easier testing.
89-89: Concise usage of Identity serviceThe removal of any explicit session parameter in this call to
verifyLoggedIn()is good - it uses the Identity service's internal session context management as intended.src/components/comments/comment-thread.resolver.ts (3)
5-5: Appropriate import for the new Identity facadeThe import of the Identity facade from the core authentication module shows good organization, aligning with the PR's goal of centralizing authentication functionality.
26-26: Good dependency injection patternProperly injecting the Identity service as a private readonly member follows best DI practices and allows for easier testing.
48-48: Concise usage of Identity serviceThe use of
this.identity.verifyLoggedIn()without parameters correctly leverages the Identity service's internal session context management.src/common/session.ts (4)
10-10: Appropriate import for the new Identity facadeThe import of the Identity facade from the core authentication module shows good organization, aligning with the PR's goal of centralizing authentication functionality.
36-36: Good dependency injection patternProperly injecting the Identity service as a private readonly member follows best DI practices and allows for easier testing.
39-39: Leveraging Identity's session managementUsing
identity.currentMaybeproperly delegates session access to the Identity service.
44-44: Clean deprecation approachThe approach of having
LoggedInSessionjust delegate toAnonSessionis clean and maintains backward compatibility while encouraging migration to the new Identity service.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/components/user/user.resolver.ts (1)
136-142: 💡 Verification agent🧩 Analysis chain
Good implementation of session-less authentication check.
The method signature has been simplified by removing the session parameter, and now correctly uses the injected Identity service to check if the user is anonymous. Consider moving this authorization check to an auth policy as mentioned in the TODO comment.
🏁 Script executed:
#!/bin/bash # Search for similar authentication checks that should also be migrated to auth policies rg -A 1 -B 1 "this.identity.isAnonymous" --glob "*.ts" | grep "TODO"Length of output: 804
Consolidate Inline Auth Checks into a Central Policy/Guard
Great work simplifying the
userByEmailmethod and leveraging the injectedIdentityservice. To reduce duplication and improve maintainability, please migrate all scatteredthis.identity.isAnonymouschecks into a shared auth policy or guard. Inline TODOs currently exist in the following files:
- src/components/pin/pin.resolver.ts // TODO move to DB layer?
- src/components/user/user.resolver.ts // TODO move to auth policy?
- src/components/project/financial-approver/financial-approver.resolver.ts // TODO move to auth policy
- src/components/product-progress/create-product-connection.resolver.ts // TODO move to auth policy
- src/components/post/post.service.ts // TODO move to auth policy
- src/components/comments/commentable.resolver.ts // TODO move to auth policy
- src/components/notifications/notification.resolver.ts // TODO move to DB layer?
- src/components/changeset/changeset.resolver.ts // TODO move to auth policy
- src/components/changeset/changeset-aware.resolver.ts // TODO move to auth policy
By centralizing this logic, you’ll ensure consistent behavior across all resolvers/services and make future updates far easier.
src/components/notifications/notification.resolver.ts (1)
19-28: 💡 Verification agent🧩 Analysis chain
Good implementation of session-less authentication check.
The method signature has been simplified by removing the session parameter, and now correctly uses the injected Identity service to check if the user is anonymous. Consider moving this check to the database layer as mentioned in the TODO comment on line 23.
🏁 Script executed:
#!/bin/bash # Check for any other similar TODOs that might need to be addressed in a future PR rg -A 1 -B 1 "TODO move to DB layer" --glob "src/**/*.ts"Length of output: 520
Refactor anonymous‐user check into the data layer for consistency
The session-less check here looks good, but as noted by the TODO on line 23, this logic belongs in the DB/repository layer rather than in each resolver. We also discovered the identical TODO and
isAnonymousguard insrc/components/pin/pin.resolver.ts. To keep our code DRY and centralize authentication concerns, please extract and apply this filter in the data-access layer and remove the per-resolver checks.Files to update:
- src/components/notifications/notification.resolver.ts (around line 23)
- src/components/pin/pin.resolver.ts (around the
// TODO move to DB layer?comment)Once the checks live in the DB layer, you can safely delete these resolver guards.
🧹 Nitpick comments (3)
src/components/project/financial-approver/financial-approver.resolver.ts (1)
35-38: Consider following up on the TODO commentThe TODO comment indicates a future improvement to move the anonymous user check to an auth policy. This would be a good follow-up task after this PR is complete to further separate authorization concerns.
src/components/product-progress/create-product-connection.resolver.ts (1)
20-22: Note the recurring TODO patternSimilar to other files in this PR, there's a TODO about moving the anonymous check to an auth policy. Consider creating a follow-up task to implement these auth policies across all affected components.
src/components/comments/commentable.resolver.ts (1)
40-43: Consider a reusable anonymous user guardMultiple files in this PR show the same pattern of checking for anonymous users and returning empty/redacted results. Consider creating a reusable guard or policy for this common pattern after completing this refactoring.
// Example of a potential reusable guard @Injectable() export class AnonymousGuard implements CanActivate { constructor(private readonly identity: Identity) {} canActivate(): boolean { return !this.identity.isAnonymous; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/common/session.ts(0 hunks)src/components/authentication/authentication.module.ts(2 hunks)src/components/authentication/extra-info.resolver.ts(2 hunks)src/components/authentication/login.resolver.ts(2 hunks)src/components/changeset/changeset-aware.resolver.ts(2 hunks)src/components/changeset/changeset.resolver.ts(3 hunks)src/components/comments/commentable.resolver.ts(2 hunks)src/components/file/file-node.resolver.ts(1 hunks)src/components/notifications/notification.resolver.ts(2 hunks)src/components/pin/pin.resolver.ts(1 hunks)src/components/product-progress/create-product-connection.resolver.ts(1 hunks)src/components/project/financial-approver/financial-approver.resolver.ts(3 hunks)src/components/user/assignable-roles.resolver.ts(2 hunks)src/components/user/user.resolver.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/common/session.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/authentication/authentication.module.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/notifications/notification.resolver.ts (1)
src/common/pagination.input.ts (1)
ListArg(118-132)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: Generate (head)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: Generate (base)
- GitHub Check: lint
- GitHub Check: Unit
- GitHub Check: Clean
🔇 Additional comments (24)
src/components/file/file-node.resolver.ts (1)
35-35: LGTM: Session parameter successfully removedThe removal of the
@AnonSession()decorator and session parameter aligns with the PR objectives of centralizing authentication through theIdentityservice facade.src/components/user/assignable-roles.resolver.ts (2)
2-2: LGTM: Import cleanupSimplified import by removing unused
AnonSessionandSessiontypes.
13-14: LGTM: Session parameter successfully removedThe
assignableRolesresolver method no longer requires an explicit session parameter, aligning with the centralized authentication approach using theIdentityservice.src/components/authentication/login.resolver.ts (4)
17-17: LGTM: Import updatedReplaced session-related imports from common with direct import of the
SessionHostservice.
23-23: LGTM: Session access moved to constructor injectionProperly injected
SessionHostservice in the constructor to replace session parameter.
47-48: LGTM: Session access refactoredSuccessfully refactored to access the session via the injected
SessionHostservice rather than through a parameter.
47-48: Verify if this should use the new Identity serviceWhile other components have been migrated to use the
Identityservice, this resolver still usesSessionHostdirectly. Confirm if this is intentional or if it should also be updated to use theIdentityservice for consistency.src/components/authentication/extra-info.resolver.ts (2)
3-3: LGTM: Import cleanupSimplified import by removing unused
AnonSessionandSessiontypes.
14-14: LGTM: Session parameter successfully removedThe
betaFeaturesmethod no longer requires an explicit session parameter, aligning with the centralized authentication approach using theIdentityservice.src/components/project/financial-approver/financial-approver.resolver.ts (1)
10-10: Successfully implemented Identity service facadeThe changes properly integrate the new Identity service for authentication checks, replacing direct session handling. This aligns well with the PR's goal of centralizing authentication functionality.
Also applies to: 23-23, 36-36
src/components/product-progress/create-product-connection.resolver.ts (1)
2-3: Correctly implemented Identity service patternThe resolver has been properly updated to use the Identity service instead of session parameters, maintaining consistent patterns with other resolvers in the codebase.
Also applies to: 9-12, 21-21
src/components/pin/pin.resolver.ts (2)
2-3: Successfully migrated to Identity serviceThe resolver has been properly updated to use the Identity service, following the same pattern as other components in this PR.
Also applies to: 9-12, 25-25
24-26: Different approach for PIN checksUnlike other files that suggest moving checks to auth policies, this TODO suggests moving the check to the DB layer. This distinction makes sense for PIN functionality which is more about user preferences than authorization.
src/components/comments/commentable.resolver.ts (1)
12-12: Successfully implemented Identity service patternThe resolver has been properly updated to use the Identity service instead of session parameters, maintaining the same functionality while improving code organization.
Also applies to: 19-22, 41-41
src/components/user/user.resolver.ts (2)
20-20: Good import addition of the Identity service.This import correctly brings in the new centralized authentication facade service.
74-74: Appropriate dependency injection of Identity service.This injection provides access to the centralized Identity facade for authentication, aligned with the PR's objective of decoupling authentication logic.
src/components/changeset/changeset-aware.resolver.ts (3)
3-5: Clean import refactoring.Properly removed the session-related imports and added the Identity service import, which aligns with the centralization of authentication logic.
13-13: Appropriate dependency injection of Identity service.This injection provides access to the centralized Identity facade for authentication.
52-66:✅ Verification successful
Good implementation of session-less authentication check.
The method signature has been simplified by removing the session parameter, and the code now correctly uses the injected Identity service to check if the user is anonymous. The call to
changesetResolver.differencehas also been updated to remove the session parameter.Consider moving this authentication check to an auth policy as mentioned in the TODO comment on line 55.
🏁 Script executed:
#!/bin/bash # Check if the changesetResolver.difference method signature has been properly updated rg -A 3 "async difference\(" --glob "src/components/changeset/*.ts"Length of output: 710
Session-less auth guard looks solid;
differencesignature matches updated resolver
File: src/components/changeset/changeset-aware.resolver.ts (lines 52–66)
- Uses the injected
Identityservice (identity.isAnonymous) to short-circuit anonymous users- Calls
changesetResolver.difference(changeset, object.id), which aligns with the removed-session signature in both the resolver and repository layersSuggestion: Consider moving the
identity.isAnonymouscheck into an auth policy, as noted by the TODO on line 55.src/components/changeset/changeset.resolver.ts (3)
2-5: Clean import refactoring.Properly removed the session-related imports and added the Identity service import, which aligns with the centralization of authentication logic.
14-14: Appropriate dependency injection of Identity service.This injection provides access to the centralized Identity facade for authentication.
26-34: Good simplification of method signature.The signature has been streamlined by removing the session parameter, which is now handled through the injected Identity service.
src/components/notifications/notification.resolver.ts (2)
2-3: Clean import refactoring.Properly removed the session-related imports and added the Identity service import, which aligns with the centralization of authentication logic.
14-17: Good dependency injection setup.The constructor now properly injects both the notification service and the Identity facade.
CarsonF
force-pushed
the
refactor/identity
branch
from
e079182 to
64fb40a
Compare
This creates an
Identityservice to be the facade for all authentication functionality that is sanctioned across the codebase.With the hope that the auth internals can be decoupled and split apart for more sanity.
This also begins the move into
~/coreI was going for a sibling to
Privilegeswith the nameIdentity, but I know the name doesn't feel very service-y.This consolidates functionality as much as possible into this service usage,
including replacing the
@AnonymousSession()&@LoggedInSession()decorators.