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] Place lab + billing behind settings/workspace permission gates #10354

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

ijreilly
Copy link
Collaborator

No description provided.

@ijreilly ijreilly requested a review from martmull February 20, 2025 10:20
Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

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

lgtm

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

Added permission gates to lab and billing functionality, requiring workspace settings permissions for access and modifications.

  • Added PermissionsModule to packages/twenty-server/src/engine/core-modules/billing/billing.module.ts and lab.module.ts for permission control
  • Added SettingsPermissionsGuard with WORKSPACE feature requirement to billing.resolver.ts and lab.resolver.ts
  • Added permission check for Lab section in useSettingsNavigationItems.tsx, requiring both lab feature flags and workspace permissions
  • Added comprehensive integration tests in workspace.integration-spec.ts and security.integration-spec.ts to verify permission gates
  • Protected billing portal and lab feature flag updates with proper role-based access controls

8 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +347 to 348
describe('updateBillingSubscription', () => {
it('should throw a permission error when user does not have permission (member role)', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Test name 'updateBillingSubscription' doesn't match the actual test (BillingPortalSession). Either the test name or the test content needs to be updated.

Suggested change
describe('updateBillingSubscription', () => {
it('should throw a permission error when user does not have permission (member role)', async () => {
describe('billingPortalSession', () => {
it('should throw a permission error when user does not have permission (member role)', async () => {

Comment on lines +380 to 378
describe('billingPortalSession', () => {
it('should throw a permission error when user does not have permission (member role)', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Duplicate test case - billingPortalSession test is identical to updateBillingSubscription test above

Comment on lines +320 to +326
mutation DeleteCurrentWorkspace {
deleteCurrentWorkspace {
id
__typename
}
}
`,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: GraphQL mutation indentation is inconsistent with rest of file

@ijreilly ijreilly force-pushed the perm--billing-lab-gates branch from 2e13317 to 95d3f59 Compare February 20, 2025 15:23
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!

@ijreilly ijreilly enabled auto-merge (squash) February 20, 2025 15:28
@ijreilly ijreilly merged commit b2bbf88 into main Feb 20, 2025
47 checks passed
@ijreilly ijreilly deleted the perm--billing-lab-gates branch February 20, 2025 15:31
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.

3 participants