Add more HOMEBREW_FORBIDDEN_* configuration
#17037
Conversation
|
I assume this is related to https://github.com/Workbrew/ somehow since it seems like a few more configuration settings that would only be useful for security-conscious ops teams to toggle in a corporate environment. Could you maybe explain what types of problems you're trying to solve here with these new forbidden environment variables? I took a quick glance at the code and I feel like the logic could probably be shared more between casks and formula which would make this more maintainable going forward. |
Essentially the same as
I'm actually not convinced. I did try this because, yes, the code looks similar, but the cask and formula logic (and formula/license/tap logic) ends up being just different enough that a shared abstraction here ends up having more conditionals and being harder to read. If you have more specific requests or suggestions: I'd love to hear them. |
Yeah cask and formula logic being separate makes sense. The general problem of formula/cask duplication something I hope to refactor at some point where time allows (not just the forbidden stuff). Within the individual installers themselves, we could potentially refactor a little bit. E.g. we have this basic flow in all three formula installer checks: unless ignore_deps?
compute_dependencies.each do |(dep, _options)|
yield dep
end
end
return if only_deps?
yield formula |
|
For taps specifically, a tap allowlist probably makes more sense than a denylist. |
MikeMcQuaid
force-pushed
the
more_forbidden
branch
from
f0068b9
to
5b0db00
Compare
@Bo98 I think we'll want to have both (and they'll be able to share some logic). I'd rather hold off adding a allowlist for now (at least in this PR) but 👍🏻 to doing so in future. |
We already had `HOMEBREW_FORBIDDEN_LICENSES` but this commit adds `HOMEBREW_FORBIDDEN_CASKS`, `HOMEBREW_FORBIDDEN_FORMULAE` and `HOMEBREW_FORBIDDEN_TAPS` for also forbidding those. Relatedly, add `HOMEBREW_FORBIDDEN_OWNER` and `HOMEBREW_FORBIDDEN_OWNER_CONTACT` to allow customising these messages. There were no existing tests for `HOMEBREW_FORBIDDEN_LICENSES` so have added more tests for all of these checks. Co-authored-by: Bo Anderson <[email protected]>
MikeMcQuaid
force-pushed
the
more_forbidden
branch
from
5b0db00
to
1474806
Compare
| forbidden_taps = Homebrew::EnvConfig.forbidden_taps | ||
| return if forbidden_taps.blank? | ||
|
|
||
| forbidden_taps_set = Set.new(forbidden_taps.split.filter_map do |tap| | ||
| Tap.fetch(tap) | ||
| rescue Tap::InvalidNameError | ||
| opoo "Invalid tap name in `HOMEBREW_FORBIDDEN_TAPS`: #{tap}" | ||
| nil | ||
| end) |
There was a problem hiding this comment.
In terms of logic that could be shared between casks and formulae right now, this bit could be moved to a Tap.forbidden method or something similar.
There was a problem hiding this comment.
It would also be possible to have method that takes a list of casks or formulae and returns the first invalid one by tap, cask, formula or license.
There was a problem hiding this comment.
In terms of logic that could be shared between casks and formulae right now, this bit could be moved to a
Tap.forbiddenmethod or something similar.
Could be but feels overkill on Rule Of 3: https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)
It would also be possible to have method that takes a list of casks or formulae and returns the first invalid one by tap, cask, formula or license.
It would be possible but I'm not convinced it would be more readable.
| forbidden_formulae = Set.new(Homebrew::EnvConfig.forbidden_formulae.to_s.split) | ||
| forbidden_casks = Set.new(Homebrew::EnvConfig.forbidden_casks.to_s.split) |
There was a problem hiding this comment.
Nit: This logic could potentially exist in Homebrew::EnvConfig too.
There was a problem hiding this comment.
I prefer to keep logic out of EnvConfig when possible.
We already had
HOMEBREW_FORBIDDEN_LICENSESbut this PR addsHOMEBREW_FORBIDDEN_CASKS,HOMEBREW_FORBIDDEN_FORMULAEandHOMEBREW_FORBIDDEN_TAPSfor also forbidding those.Relatedly, add
HOMEBREW_FORBIDDEN_OWNERandHOMEBREW_FORBIDDEN_OWNER_CONTACTto allow customising these messages.There were no existing tests for
HOMEBREW_FORBIDDEN_LICENSESso have added more tests for all of these checks.