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

test: Unit test to assert role capabilities #1781

Merged
merged 4 commits into from
May 27, 2022
Merged

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented May 26, 2022

This unit test allows for asserting which roles can perform
actions on various objects. This is much easier than making
unit tests to hit the api.

Emyrk added 3 commits May 26, 2022 10:18
This unit test allows for asserting which roles can perform
actions on various objects. This is much easier than making
unit tests to hit the api.
@Emyrk Emyrk changed the title test: Unit test to assert role permissions test: Unit test to assert role capabilities May 26, 2022
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Love me some more tests!

coderd/rbac/builtin_test.go Outdated Show resolved Hide resolved
Comment on lines 64 to 70
Name: "AUser",
Actions: []rbac.Action{rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete},
Resource: rbac.ResourceUser,
Assertions: map[bool][]authSubject{
true: {admin},
false: {member, orgMember, orgAdmin, otherOrgMember, otherOrgAdmin},
},
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Could we rename these fields for clarity? When I initially read the test I got a bit confused, as it appears to me that this test was asserting that any random user resource should have the admin role. Now after reading it, it appears that the Resource in question is the object, and the subjects are all of those identified in assertions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. I can rename. I changed this struct up a few times, probably why it ended as it did.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added comments too.

Resource rbac.Object
Actions []rbac.Action
// Assertions must cover all subjects in 'requiredSubjects'
Assertions map[bool][]authSubject
Copy link
Member

Choose a reason for hiding this comment

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

nit: map[authSubject]bool? Kind of easier to reason about. Optional though, this is really a nit pick.

Copy link
Member Author

Choose a reason for hiding this comment

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

I felt it was easier to just list out subjects in a 1 line list.

},
},
{
Name: "APIKey",
Copy link
Member

Choose a reason for hiding this comment

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

We should probably clarify that the APIKey is owned by the user referred to by orgMember

Copy link
Member Author

Choose a reason for hiding this comment

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

It is. I changed member and orgMember to memberMe and orgMemberMe respectively.

- Add comments, rename fields
@Emyrk Emyrk merged commit ebaae75 into main May 27, 2022
@Emyrk Emyrk deleted the stevenmasley/rbac_more_testing branch May 27, 2022 13:48
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
* test: Unit test to assert role permissions

This unit test allows for asserting which roles can perform
actions on various objects. This is much easier than making
unit tests to hit the api.
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.

None yet

2 participants