Stage #9305
Merged
Stage #9305
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fix: agent build release path
[Feat] Shared entity module feature
* feat(employee): change User-Employee relation from OneToOne to ManyToOne This allows one User to have multiple Employee records across different organizations within the same tenant. Fixes #8465 * feat(migration): add PostgreSQL migration for User-Employee relation change Removes UNIQUE constraint on employee.userId to allow multiple employees per user across organizations. * feat(database): add MySQL migration to remove UNIQUE constraint on employee.userId Adds MySQL Up/Down migration queries to support multi-employee per user across organizations. - MySQL Up: Drops FK, drops UNIQUE index, re-adds FK - MySQL Down: Drops FK, creates UNIQUE index, re-adds FK Related to #8465 * fix(employee): add organizationId context to findOneByUserId and findEmployeeIdByUserId - Add RequestContext.currentOrganizationId() to retrieve organization from headers - Update findEmployeeIdByUserId to accept optional organizationId parameter - Update findOneByUserId to include tenantId and organizationId in where clause - Ensures correct employee is found when user has multiple employees across organizations Related to #8465 * fix(employee): prevent duplicate user creation and fix organization switcher for employees - EmployeeCreateHandler: Check if user with email exists in tenant before creating new user - header.component: Show organization selector if user has more than 1 organization - organization.component: Only request featureOrganizations relations if user has ALL_ORG_VIEW permission Related to #8465 * fix(auth): support multi-organization employee context switching - Modified jwt.strategy.ts to verify JWT employeeId belongs to user, then dynamically get the employee for the current organization context - Created OrganizationContextService to automatically refresh user data (including correct employee) when selectedOrganization changes - Integrated OrganizationContextService initialization in AppInitService - This ensures store.user.employee always reflects the current organization Fixes #8465 * feat(auth): add switch-organization API endpoint - Add SwitchOrganizationDTO for the switch organization request - Add AuthService.switchOrganization() method to generate new JWT with correct employeeId for target organization - Add POST /auth/switch-organization endpoint in AuthController - Modify EmployeeService.findOneByUserId() to accept optional organizationId - Modify AuthService.getJwtAccessToken() to accept optional organizationId This allows users with employees in multiple organizations to switch context and receive a new JWT with the correct employeeId. Relates to #8465 * refactor(auth): simplify jwt.strategy.ts validation - Verify employeeId from JWT belongs to the user (security check) - Assign employeeId directly from JWT (no dynamic lookup needed) - JWT now always contains correct employeeId for current organization (regenerated via /auth/switch-organization when user switches) Relates to #8465 * feat(ui): integrate organization switch with new JWT regeneration - Add AuthService.switchOrganization() to call backend API - Rewrite OrganizationContextService to use explicit switch API instead of automatic listener-based refresh - Modify OrganizationSelectorComponent.selectOrganization() to call OrganizationContextService.switchOrganization() and update tokens When user switches organization, the frontend now: 1. Calls POST /auth/switch-organization 2. Receives new JWT with correct employeeId 3. Updates store with new tokens and user data 4. Updates selected organization in URL Fixes #8465 * fix(employee): wrap findOneByWhereOptions in try-catch to handle NotFoundException findOneByWhereOptions throws NotFoundException when no record is found, but the code assumed it returns null. This caused an exception instead of proceeding to create a new user when the user doesn't exist. * refactor(ui-core): remove no-op OrganizationContextService.initialize() call The initialize() method is a no-op for backward compatibility, but the comment claimed it would refresh user.employee when organization changes. Removed both the misleading comment and the unnecessary method call. * fix(auth): add block scope to switch case clauses to prevent variable leaking Switch statements in JS/TS share scope across case clauses, which can cause variable declarations to leak or conflict. Wrapped MikroORM and TypeORM case bodies in their own blocks ({}) so declarations are lexically scoped to each case. * Update packages/core/src/lib/auth/auth.service.ts Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * fix migration * feat(auth): add organization context to JWT tokens and secure organization switching - Add CHANGE_SELECTED_ORGANIZATION permission to EMPLOYEE role - Add PermissionGuard to /switch-organization endpoint - Add organizationId to JWT access token payload - Add organizationId to JWT refresh token payload - Update getAccessTokenFromRefreshToken to maintain organization context - Secure RequestContext.currentOrganizationId() to read from JWT instead of headers BREAKING CHANGE: JWT tokens now include organizationId field. Clients should handle the new token structure. * fix(ui): restore CHANGE_SELECTED_ORGANIZATION permission check for organization selector Re-add permission verification that was removed - users without CHANGE_SELECTED_ORGANIZATION permission should not see the organization selector in the header. * fix(migration): remove UNIQUE constraint on userId in SQLite UP migration Remove CONSTRAINT REL_f4b0d329c4a3cf79ffe9d56504 UNIQUE (userId) from all CREATE TABLE temporary_employee statements in sqliteUpQueryRunner to allow many-to-one relationship (multiple employees can reference the same user). The DOWN migration retains the UNIQUE constraint to restore the original one-to-one relationship when reverting. * fix(context): merge duplicate currentOrganizationId methods with proper fallback Consolidate two currentOrganizationId() methods into one with priority: 1. JWT token organizationId (most secure) 2. User's employee organizationId (fallback for old tokens) 3. Request header organization-id (legacy backward compatibility) This ensures existing functionality continues to work while preferring the secure JWT-based organization context when available. * fix(auth): inject organizationId from JWT into user with fallback Make organizationId follow the same pattern as employeeId: - jwt.strategy.ts: inject organizationId from JWT into user.lastOrganizationId - request-context.ts: currentOrganizationId() reads from user.lastOrganizationId with fallback to user.employee.organizationId and header for backward compatibility This ensures consistency across all context methods while maintaining backward compatibility with old tokens. * fix(auth): validate organization access in JWT strategy - Add UserOrganizationService to validate user has access to organization - Remove unvalidated header fallback from currentOrganizationId() - organizationId is now only accepted from validated JWT tokens * fix(employee): catch specific NotFoundException and validate input - Catch only NotFoundException instead of all errors - Add validation for input.user.email before accessing it * fix(ui): add await for async selectOrganization calls - Make updateOrganization, deleteOrganization, selectOrganizationById async - Properly await selectOrganization to prevent race conditions * fix(ui): add @deprecated to initialize() method - Mark initialize() as deprecated with JSDoc - Clean up comments in applyOrganizationData() * docs(auth): clarify refresh token organization behavior - Add note explaining refresh token is organization-specific - Document that /auth/switch-organization should be used to change org * refactor(ui): use inject() function instead of constructor injection - Replace constructor parameter injection with inject() function - Follow Angular modern DI pattern * fix(auth): include organizationId in refresh token - Pass organizationId to getJwtRefreshToken in login, signinWorkspaceByToken, and switchWorkspace - Ensures refresh token contains same organization context as access token * fix(auth): add cross-validation between employeeId and organizationId in JWT - Validate that employee.organizationId matches the claimed organizationId - Prevents JWT token manipulation attacks * fix(employee): use BadRequestException and check for existing employee - Use BadRequestException instead of generic Error for proper HTTP 400 - Check if employee already exists for user+organization to prevent duplicates * fix(ui): validate response fields before applying to store - Check token and user exist before updating store - Return false and show error if validation fails * fix(auth): update user.lastOrganizationId in memory after DB update - Ensures returned user object has fresh lastOrganizationId value * fix(employee): load role relation when finding existing user - Use findOneByOptions with relations: { role: true } - Fixes 'Cannot read properties of undefined (reading name)' error - addUserToOrganization requires user.role.name for SUPER_ADMIN check --------- Co-authored-by: Ruslan Konviser <[email protected]> Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
|
Contributor
Sequence DiagramsequenceDiagram
participant User
participant OrganizationSelectorComponent
participant OrganizationContextService
participant AuthService
participant AuthController
participant AuthService as BackendAuthService
participant JwtStrategy
participant EmployeeService
participant Store
User->>OrganizationSelectorComponent: "Select different organization"
OrganizationSelectorComponent->>OrganizationContextService: "switchOrganization(organization)"
OrganizationContextService->>AuthService: "switchOrganization(organizationId)"
AuthService->>AuthController: "POST /auth/switch-organization"
AuthController->>BackendAuthService: "switchOrganization(organizationId)"
BackendAuthService->>EmployeeService: "findOneByUserId(userId, organizationId)"
EmployeeService-->>BackendAuthService: "employee data"
BackendAuthService->>BackendAuthService: "getJwtAccessToken(user, organizationId)"
BackendAuthService->>BackendAuthService: "getJwtRefreshToken(user, organizationId)"
BackendAuthService-->>AuthController: "IAuthResponse with new tokens"
AuthController-->>AuthService: "IAuthResponse"
AuthService-->>OrganizationContextService: "response with new tokens"
OrganizationContextService->>OrganizationContextService: "applyOrganizationData(response, organization)"
OrganizationContextService->>Store: "Update tokens and user data"
OrganizationContextService->>Store: "Set selectedOrganization"
OrganizationContextService-->>OrganizationSelectorComponent: "success = true"
OrganizationSelectorComponent->>User: "Organization switched successfully"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Additional Comments (2)
-
packages/core/src/lib/employee/commands/handlers/employee.create.handler.ts, line 143-145 (link)logic: Error handling should throw the error or return a meaningful result instead of just logging
-
packages/ui-core/shared/src/lib/selectors/organization/organization.component.ts, line 395-402 (link)syntax: The getRandomOrganization method returns null when the array is empty, but the return type suggests IOrganization. This should be IOrganization | null to match the actual behavior.
38 files reviewed, 18 comments
| } | ||
|
|
||
| // Return the result | ||
| return result; |
Contributor
There was a problem hiding this comment.
style: Inconsistent indentation - this line has extra spaces before 'return'
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/shared-entity/shared-entity.helper.ts
Line: 83:83
Comment:
**style:** Inconsistent indentation - this line has extra spaces before 'return'
How can I resolve this? If you propose a fix, please make it concise.| /** | ||
| * Create Shared Entity DTO | ||
| */ | ||
| export class CreateSharedEntityDTO extends IntersectionType(TenantOrganizationBaseDTO, OmitType(SharedEntity, ['token'])) implements ISharedEntityCreateInput {} |
Contributor
There was a problem hiding this comment.
style: Omitting token prevents token manipulation in user-supplied data but client-side token generation may conflict with server handling. Is the token auto-generated server-side or user-provided? Should the token field be auto-generated on the server side or is it expected to be provided by the client?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/shared-entity/dto/create-shared-entity.dto.ts
Line: 9:9
Comment:
**style:** Omitting `token` prevents token manipulation in user-supplied data but client-side token generation may conflict with server handling. Is the token auto-generated server-side or user-provided? Should the `token` field be auto-generated on the server side or is it expected to be provided by the client?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| @HttpCode(HttpStatus.OK) | ||
| @Get('/token/:token') | ||
| @UseValidationPipe() | ||
| async getSharedEntityByToken(@Param('token') token: string): Promise<any> { |
Contributor
There was a problem hiding this comment.
style: Return type 'any' reduces type safety - consider defining a specific interface for the shared entity response
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/shared-entity/shared-entity.controller.ts
Line: 60:60
Comment:
**style:** Return type 'any' reduces type safety - consider defining a specific interface for the shared entity response
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+27
to
+28
| if (typeof entity.shareRules === 'object') { | ||
| entity.shareRules = JSON.stringify(entity.shareRules); |
Contributor
There was a problem hiding this comment.
style: Should check for null/undefined before typeof check to prevent potential issues
Suggested change
| if (typeof entity.shareRules === 'object') { | |
| entity.shareRules = JSON.stringify(entity.shareRules); | |
| if (entity.shareRules && typeof entity.shareRules === 'object') { | |
| entity.shareRules = JSON.stringify(entity.shareRules); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/shared-entity/shared-entity.subscriber.ts
Line: 27:28
Comment:
**style:** Should check for null/undefined before typeof check to prevent potential issues
```suggestion
if (entity.shareRules && typeof entity.shareRules === 'object') {
entity.shareRules = JSON.stringify(entity.shareRules);
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+32
to
+33
| if (typeof entity.sharedOptions === 'object') { | ||
| entity.sharedOptions = JSON.stringify(entity.sharedOptions); |
Contributor
There was a problem hiding this comment.
style: Should check for null/undefined before typeof check to prevent potential issues
Suggested change
| if (typeof entity.sharedOptions === 'object') { | |
| entity.sharedOptions = JSON.stringify(entity.sharedOptions); | |
| if (entity.sharedOptions && typeof entity.sharedOptions === 'object') { | |
| entity.sharedOptions = JSON.stringify(entity.sharedOptions); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/shared-entity/shared-entity.subscriber.ts
Line: 32:33
Comment:
**style:** Should check for null/undefined before typeof check to prevent potential issues
```suggestion
if (entity.sharedOptions && typeof entity.sharedOptions === 'object') {
entity.sharedOptions = JSON.stringify(entity.sharedOptions);
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+61
to
+66
| if (validatedEmployee && validatedEmployee.organizationId !== organizationId) { | ||
| return done( | ||
| new UnauthorizedException('Employee does not belong to the claimed organization'), | ||
| false | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
logic: Cross-validation logic prevents legitimate multi-organization access. Should users be able to have employee records in multiple organizations simultaneously? This validation would prevent that use case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/auth/strategies/jwt.strategy.ts
Line: 61:66
Comment:
**logic:** Cross-validation logic prevents legitimate multi-organization access. Should users be able to have employee records in multiple organizations simultaneously? This validation would prevent that use case.
How can I resolve this? If you propose a fix, please make it concise.| where: { | ||
| userId: user.id, | ||
| organizationId, | ||
| tenantId: tenantId || user.tenantId, |
Contributor
There was a problem hiding this comment.
logic: Inconsistent tenantId fallback logic may cause authorization bypass
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/auth/strategies/jwt.strategy.ts
Line: 72:72
Comment:
**logic:** Inconsistent tenantId fallback logic may cause authorization bypass
How can I resolve this? If you propose a fix, please make it concise.| done(null, user); | ||
| } catch (error) { | ||
| console.error('Error occurred during JWT validation:', error); | ||
| return done(new UnauthorizedException('unauthorized', error.message), false); |
Contributor
There was a problem hiding this comment.
logic: Error message in UnauthorizedException exposes internal error details
Suggested change
| return done(new UnauthorizedException('unauthorized', error.message), false); | |
| return done(new UnauthorizedException('unauthorized'), false); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/auth/strategies/jwt.strategy.ts
Line: 88:88
Comment:
**logic:** Error message in UnauthorizedException exposes internal error details
```suggestion
return done(new UnauthorizedException('unauthorized'), false);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+85
to
107
| public async selectOrganization(organization: IOrganization | null | undefined): Promise<void> { | ||
| if (!organization) { | ||
| this._toastrService.warning('No organization provided to select.'); | ||
| console.warn('No organization provided to select.'); | ||
| return; | ||
| } | ||
|
|
||
| // Update the store with the selected organization details | ||
| this._store.selectedOrganization = organization; | ||
| this._store.organizationId = organization.id; | ||
|
|
||
| // Reset the selected employee store if required | ||
| if (isResetEmployeeStore) { | ||
| this._store.selectedEmployee = null; | ||
| console.info('Selected employee store has been reset.'); | ||
| // Check if we're already on this organization | ||
| if (this._store.selectedOrganization?.id === organization.id) { | ||
| console.info('Already on this organization, skipping switch.'); | ||
| return; | ||
| } | ||
|
|
||
| console.log(`Selected Organization: ${organization.name}`); | ||
| console.log(`Switching to Organization: ${organization.name}`); | ||
|
|
||
| // Call the backend to switch organization and get new JWT with correct employeeId | ||
| const success = await this._organizationContextService.switchOrganization(organization); | ||
|
|
||
| // Update the query parameters in the URL | ||
| this._navigationService.updateQueryParams({ organizationId: organization.id }); | ||
| if (success) { | ||
| // Update the query parameters in the URL | ||
| this._navigationService.updateQueryParams({ organizationId: organization.id }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
logic: The selectOrganization method has been made async but lacks error handling. If the OrganizationContextService.switchOrganization fails, it returns false but the error is not communicated to the user.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui-core/shared/src/lib/selectors/organization/organization.component.ts
Line: 85:107
Comment:
**logic:** The selectOrganization method has been made async but lacks error handling. If the OrganizationContextService.switchOrganization fails, it returns false but the error is not communicated to the user.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+370
to
377
| async selectOrganizationById(organizationId: ID): Promise<void> { | ||
| const organization = this.organizations.find( | ||
| (organization: IOrganization) => organization.id === organizationId | ||
| ); | ||
| if (organization) { | ||
| this.selectOrganization(organization, false); | ||
| await this.selectOrganization(organization); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
logic: The selectOrganizationById method is now async but doesn't handle the case where selectOrganization fails. If the organization switch fails, the user won't receive feedback about the failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui-core/shared/src/lib/selectors/organization/organization.component.ts
Line: 370:377
Comment:
**logic:** The selectOrganizationById method is now async but doesn't handle the case where selectOrganization fails. If the organization switch fails, the user won't receive feedback about the failure.
How can I resolve this? If you propose a fix, please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by cubic
Enable secure organization switching and multi-organization employees with org-aware JWTs and a new /auth/switch-organization flow, plus UI updates to select organizations. Adds a Shared Entity module to share controlled slices of data.
New Features
Migration
Written for commit bf51ff9. Summary will update automatically on new commits.