Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Permissions] Add userWorkspaceId to JWT token #9954

Merged
merged 5 commits into from
Jan 31, 2025
Merged

Conversation

ijreilly
Copy link
Collaborator

This information will be used to fetch a user's role and check their permissions

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR adds userWorkspaceId to JWT tokens and request context to support role-based permission checking across the application.

  • Added new USER_WORKSPACE_NOT_FOUND exception code and validation in JwtAuthStrategy to handle cases where user workspace associations are missing
  • Added AuthUserWorkspaceId decorator in /decorators/auth/auth-user-workspace-id.decorator.ts to extract workspace IDs from requests, though it lacks error handling
  • Extended AuthContext and JwtPayload types in /types/auth-context.type.ts to include optional userWorkspaceId field
  • Modified AccessTokenService to include userWorkspaceId in token generation and validation flow
  • Added UserWorkspace entity to TypeORM configurations in both AuthModule and TokenModule for database operations

10 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +123 to +127
const userWorkspace = await this.userWorkspaceRepository.findOne({
where: {
id: payload.userWorkspaceId,
},
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider also validating that the userWorkspace belongs to both the user and workspace being validated to prevent unauthorized access.

Suggested change
const userWorkspace = await this.userWorkspaceRepository.findOne({
where: {
id: payload.userWorkspaceId,
},
});
const userWorkspace = await this.userWorkspaceRepository.findOne({
where: {
id: payload.userWorkspaceId,
userId: user.id,
workspaceId: workspace.id,
},
});

Comment on lines +93 to +98
const userWorkspace = await this.userWorkspaceRepository.findOne({
where: {
userId: user.id,
workspaceId,
},
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: No error handling if userWorkspace lookup fails. Consider throwing AuthException with USER_WORKSPACE_NOT_FOUND code since userWorkspaceId is required for permissions.

Comment on lines +6 to +10
(data: unknown, ctx: ExecutionContext) => {
const request = getRequest(ctx);

return request.userWorkspaceId;
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: No error handling if userWorkspaceId is undefined. Could silently fail permission checks. Consider throwing AuthException with USER_WORKSPACE_NOT_FOUND code.

Suggested change
(data: unknown, ctx: ExecutionContext) => {
const request = getRequest(ctx);
return request.userWorkspaceId;
},
(data: unknown, ctx: ExecutionContext) => {
const request = getRequest(ctx);
if (!request.userWorkspaceId) {
throw new UnauthorizedException('User workspace not found');
}
return request.userWorkspaceId;
},

@ijreilly ijreilly changed the title Add userWorkspaceId to JWT token [Permissions] Add userWorkspaceId to JWT token Jan 31, 2025
Weiko

This comment was marked as resolved.

Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @ijreilly

@Weiko Weiko merged commit 58aa86c into main Jan 31, 2025
32 checks passed
@Weiko Weiko deleted the perm-permission-service branch January 31, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants