-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
feat(api,cli,api-client,schema): Enhance permissions to allow filtering by environments through project roles #599
base: develop
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the last push made by me, i could see a ton of e2e tests fail on the API. You would need to update the environment access across the tests i believe
const projectSlugToIdMap = await this.getProjectSlugToIdMap( | ||
dto.projectEnvironments | ||
.map((pe) => pe.projectSlug) | ||
.filter((slug) => slug) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure what the filter
construct does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cli implementation was causing undefined slug values.
roleId: workspaceRoleId, | ||
projectId: projectId, | ||
environments: { | ||
connect: pe.environmentSlugs.map((slug) => ({ slug })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For every environment slug belonging to a project, we need to make sure that the environment does indeed belong to a project.
} | ||
}, | ||
update: { | ||
environments: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what this update does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sets the previously connected environments to empty then adds the new ones.
Could be. I try it later today |
User description
Description
Allows roles to contain access to only specified environments for projects
Fixes #260
Future Improvements
Mention any improvements to be done in future related to any file/feature
Developer's checklist
If changes are made in the code:
Documentation Update
PR Type
Enhancement
Description
Enhanced workspace role permissions to support environment-specific access control:
getCollectiveEnvironmentAuthorities
function to handle environment-level authority checksprojectEnvironments
structureChanges walkthrough 📝
authority-checker.service.ts
Update environment authority checking mechanism
apps/api/src/common/authority-checker.service.ts
getCollectiveEnvironmentAuthorities
functioncollective-authorities.ts
Add environment-specific authority collection
apps/api/src/common/collective-authorities.ts
getCollectiveEnvironmentAuthorities
function to handleenvironment-specific permissions
filtering
create-workspace-role.ts
Update workspace role DTO for environment access
apps/api/src/workspace-role/dto/create-workspace-role/create-workspace-role.ts
projectSlugs
with newprojectEnvironments
structureProjectEnvironments
class to handle environment-specific accessworkspace-role.service.ts
Implement environment-specific role management
apps/api/src/workspace-role/service/workspace-role.service.ts
access
schema.prisma
Update schema for environment role associations
apps/api/src/prisma/schema.prisma
model