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 permission gates on workspace-invitations #10394

Merged
merged 4 commits into from
Feb 21, 2025
Merged

Conversation

ijreilly
Copy link
Collaborator

Adding permission gates on all workspace-invitations endpoints: sendInvitation, resendInvitation, deleteWorkspaceInvitation, findWorkspaceInvitations (the latter being from my understanding only used to list the invitations to then re-send them or detee them).

  • tests on Api & webhooks permission gates

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 permission gates to workspace invitation endpoints and their corresponding integration tests, ensuring proper access control for invitation management.

  • Added SettingsPermissionsGuard with WORKSPACE_USERS feature flag to workspace-invitation.resolver.ts to restrict invitation operations
  • Added comprehensive integration tests in workspace-invitation.integration-spec.ts verifying permission denials for send/resend/find/delete operations
  • Added makeMetadataAPIRequestWithMemberRole utility in /test/integration/metadata/suites/utils for consistent member role testing
  • Added integration tests for API key and webhook permissions in api-key-webhooks.integration-spec.ts
  • Added data model permission tests in data-model.integration-spec.ts for field/object metadata operations

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

Comment on lines +196 to +198
gqlFields: `
id
`,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Inconsistent indentation in gqlFields template literal compared to other similar queries

Comment on lines +54 to +57
afterAll(async () => {
await deleteFieldMetadata(testFieldId);
await deleteOneObjectMetadataItem(listingObjectId);
});
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 wrapping cleanup operations in try/catch to ensure cleanup runs even if one operation fails


await makeGraphqlAPIRequest(disablePermissionsQuery);
});
describe('generateApiKeyToken', () => {
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 suite only covers generateApiKeyToken - missing test cases for webhook-related permissions that are mentioned in PR title

Comment on lines +31 to +57
describe('generateApiKeyToken', () => {
it('should throw a permission error when user does not have permission (member role)', async () => {
const queryData = {
query: `
mutation generateApiKeyToken {
generateApiKeyToken(apiKeyId: "test-api-key-id", expiresAt: "2025-01-01T00:00:00Z") {
token
}
}
`,
};

await client
.post('/graphql')
.set('Authorization', `Bearer ${MEMBER_ACCESS_TOKEN}`)
.send(queryData)
.expect(200)
.expect((res) => {
expect(res.body.data).toBeNull();
expect(res.body.errors).toBeDefined();
expect(res.body.errors[0].message).toBe(
PermissionsExceptionMessage.PERMISSION_DENIED,
);
expect(res.body.errors[0].extensions.code).toBe(ErrorCode.FORBIDDEN);
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Missing positive test case to verify admin role can successfully generate API key token

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 merged commit ee28102 into main Feb 21, 2025
32 checks passed
@ijreilly ijreilly deleted the perm--tests branch February 21, 2025 16:26
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