Skip to content

chore(cli): add linter to detect potential spurious usage of owner user in cli tests #10133

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 24 commits into from
Oct 10, 2023
Merged

chore(cli): add linter to detect potential spurious usage of owner user in cli tests #10133

merged 24 commits into from
Oct 10, 2023

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Oct 9, 2023 •
edited
Loading

Detects the following pattern where the CLI is initialized with a client authenticated as the "first user":

client := coderdtest.New(t, ...)
[...]
user := coderdtest.CreateFirstUser(t, client)
[...]
clitest.SetupConfig(t, client, root)

When updating the unit tests, I tried to stick to the principle of least privilege as much as possible. Note that this doesn't stop you from using the owner if needed, you just need to add a //nolint:gocritic with reasoning.

Note: In the course of going through this, I also noted an inaccuracy in our admin RBAC docs regarding updating other workspaces. Updated.

@johnstcn johnstcn self-assigned this Oct 9, 2023
@johnstcn johnstcn marked this pull request as ready for review October 9, 2023 15:27
Emyrk
Emyrk approved these changes Oct 9, 2023
View reviewed changes
Copy link
Member

@Emyrk Emyrk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Diff doesn't seem as bad as I would have guessed. Sweet!

Curious how robust the rules.go is. I guess we never nest those 2 things in different scopes, so it should work pretty well.

mafredri
mafredri approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

This solution turned out nice, awesome job!

@johnstcn
Copy link
Member Author

Curious how robust the rules.go is. I guess we never nest those 2 things in different scopes, so it should work pretty well.

There does seem to be room for improvement, it didn't actually catch the port-forwarding thing because the calls to coderdtest.CreateFirstUser and clitest.SetupConfig were in different functions. I'll see about addressing that in a follow-up.

@johnstcn johnstcn merged commit c83af5e into main Oct 10, 2023
@johnstcn johnstcn deleted the cj/owner-user-linter branch October 10, 2023 10:14
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

@Emyrk Emyrk Emyrk approved these changes

@mtojek mtojek Awaiting requested review from mtojek

@kylecarbs kylecarbs Awaiting requested review from kylecarbs

Assignees

@johnstcn johnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johnstcn @mafredri @Emyrk