-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add Permissions integration tests #1550
Conversation
admin_group = next(group for group in response.nodes if group.groupUri == 'DHAdmins') | ||
assert_that(admin_group).contains_key('tenantPermissions') |
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.
nit: any reason we filter for DHAdmins
and not just take the first entry like response.nodes[0]
to ensure it contains tenantPermissions
key?
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 DHAdmins is a group created during deployment; it is the only one with the tenant permissions. It is true this is testing something else. We could break it down into 2 tests - test_tenant_group_exists and test_list_tenant_groups
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 ended up adding a comment to explain the purpose of that last check
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.
okay I think makes sense - may be no need to include response.nodes[0]
since it is duplicate chck of admin_group
which is testing the same and more, but PR looks good
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.
Left one minor nit but this PR looks good and tested for checks passing
Feature or Bugfix
Detail
Implement tests for Permissions api calls (inside core/permissions) as part of #1220
!Excludes updateSSMParameter mutation - I think it is unused
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.