-
Notifications
You must be signed in to change notification settings - Fork 205
plugins: homebrew: Add "brew audit" to allowlist #494
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
base: main
Are you sure you want to change the base?
Conversation
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
Signed-off-by: Matt Coster <[email protected]>
aaf5a6f
to
1590938
Compare
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.
Thank you for your contribution! 😄
I've left only one question that is related to backwards compatibility when it comes to adding authentication for the Shell plugin to new subcommands that didn't require it before.
@@ -15,14 +15,15 @@ func HomebrewCLI() schema.Executable { | |||
NeedsAuth: needsauth.IfAll( | |||
needsauth.NotForHelpOrVersion(), | |||
needsauth.IfAny( | |||
needsauth.ForCommand("search"), | |||
needsauth.ForCommand("audit"), |
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.
How will this affect current users that don't expect to be asked to authorize the Shell plugin for this subcommand?
Is this something that we want to accept from now on?
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.
Just quickly checked brew audit
usage and it does seem like there are use cases where you would not require authentication.
So I think we need to scope this down so that auth is only provided when it's required. For:
- Security: provide access least privilege only when needed.
- Productivity: only bother user with authorization prompt when needed.
I imagine a flag like --online
or --tap
may indicate there's a need for auth, but I'll defer to @MTCoster who has more context on the use cases for this command.
We can implement using needsauth.IfAll
combined with the current needsauth.ForCommand
and a new to be created in needsauth/helpers.go WhenContainsArgs
(which can share most of its logic with existing NotWhenContainsArgs
).
Overview
Allow the homebrew plugin to provide the token for the
audit
subcommand. Also re-sort the existing subcommands.Type of change
How To Test
Changelog