Consider supporting groups instead of single users only and create their tests #351
Conversation
|
This is an open source project, remove any references to trackers that are not in this repo. If they contain additional context, move it to the GitHub issue you are fixing. |
ShazaAldawamneh
changed the title
…heir tests] added comment Refactor group storage structure in status.go to use sets Optimize group lookup by precomputing GroupSet in NewStaticAuthorizer for O(1) checks. Update pkg/authorization/static/static.go Co-authored-by: Krzysztof Ostrowski <[email protected]> Update pkg/authorization/static/static.go Co-authored-by: Krzysztof Ostrowski <[email protected]> imported set
ShazaAldawamneh
force-pushed
the
issue40
branch
from
26e78f3 to
2b27a4d
Compare
| Groups []string `json:"groups,omitempty"` | ||
| Name string `json:"name,omitempty"` | ||
| Groups []string `json:"groups,omitempty"` | ||
| GroupSet set.Set[string] |
There was a problem hiding this comment.
Can we split the external, serialized config, and its internal representation?
Make it so that only one of username/group can be specified.
| func NewStaticAuthorizer(config []StaticAuthorizationConfig) (*staticAuthorizer, error) { | ||
| for _, c := range config { | ||
| if c.ResourceRequest != (c.Path == "") { | ||
| for c := range config { |
There was a problem hiding this comment.
Since we need some internal representation of the config anyway, it might be easier to implement this part as a unionauthorizer.New([]staticAuthorizer{...}) (union authorizer constructor) where each staticAuthorizer in the above mentioned slice represents each element from []StaticAuthorizationConfig here.
That way func (saConfig StaticAuthorizationConfig) Matches(a authorizer.Attributes) bool
changes into
func (sa staticAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error).
Each return true changes into authorizer.DecisionAllow and each return false into authorizer.DecisionNoOpinion.
The constructor might then look something like this:
func NewStaticAuthorizer(config []StaticAuthorizationConfig) (authorizer.Authorizer, error) {
var authorizers []staticAuthorizer
for _, c := range config {
authz, err := newStaticAuthorizers(&c)
// handle error
authorizers = append(authorizers, authz)
}
return unionauthorizer.New(authorizers...)
}WDYT?
cc @ibihim
There was a problem hiding this comment.
lol. Yes, I agree. This is what I tried to convey. We need an internal and an external representation. The internal one has a Set, the other has a slice of groups potentially.
WRT the I have no strong opinions. We could leave it as is, but why invent our own logic, if we try to satisfy the authorizer interface, right?
| return true | ||
| } | ||
| for _, group := range requestGroups { | ||
| if _, exists := saConfig.User.GroupSet[group]; exists { |
stlaz
added
the
sig-auth-acceptance
|
As this PR is pretty stale, I will take it over, if you don't mind @ShazaAldawamneh |
3 participants
this PR to cover issue: 333