test: Handle Filter flake with ctx errors #7119
Conversation
coderd/rbac/authz.go
Outdated
| @@ -137,6 +137,9 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, a | |||
| err := auth.Authorize(ctx, subject, action, o.RBACObject()) | |||
| if err == nil { | |||
| filtered = append(filtered, o) | |||
| } else if ctx.Err() != nil { | |||
| // Exit early if the error comes from the context | |||
| return nil, ctx.Err() | |||
There was a problem hiding this comment.
This code implicitly assumes that there are only 2 classes of errors that could be returned from auth.Authorize
- the authorization completed, but the subject is not authorized
- the context is Done
Prior to this change, this code implicitly assumed that there was only 1 class of errors.
It is difficult to be sure that the right answer is 2. Could it be 3 classes? 4?
Honestly, I think a better design would be for auth.Authorize to return two values --- one that tells you whether the subject is authorized, and another to tell you whether or not there was an error checking the authorization.
Failing that, it's generally better for error checking to be explicit and specific:
err := auth.Authorize(...)
if xerrors.Is(err, rbac.Unauthorized) {
continue
}
if err != nil {
return nil, err
}
filtered = append(filtered, o)
There was a problem hiding this comment.
Actually, thinking about this more, it's not about implicit vs explicit. It's about how the code reacts if Authorize() returns an unexpected error. By only handling the context case, we'd be the in the same boat again of swallowing the unexpected error and treating it like an unauthorized response.
If we instead handle the expected errors, and return any others, we're less likely to incorrectly ignore them.
There was a problem hiding this comment.
Here are the 2 classes of error returned by authorize:
Lines 326 to 333 in 6d41edc
Right now both are ForbiddenWithInternal. Maybe the correct solution here is to return something different on the Eval error.
There was a problem hiding this comment.
Then I can check for rbac.IsUnauthorizedError() on the Filter function loop
There was a problem hiding this comment.
Yeah, a test would be good. We generate a list of say, 3 |
| err = correctCancelError(err) | ||
| return xerrors.Errorf("evaluate rego: %w", err) |
There was a problem hiding this comment.
For authorize single items.
| err = correctCancelError(err) | ||
| return xerrors.Errorf("eval error: %w", err) |
|
Ugh a unit test is really hard to make for this. The ctx is checked for cancel in a go routine: https://github.com/open-policy-agent/opa/blob/main/rego/rego.go#L2002-L2004 If the policy executes fast enough, then the ctx is ignored. Which means this is racy. |
The standard procedure for when your code needs to call some 3rd party thing that is non-deterministic, is to mock it out and have a test for each case/class of what the non-deterministic thing can return. |
|
@spikecurtis yea, I thought mocking out the rego would be tough because they are all structs. But I just need to mock out our authz interface. I'll make this test work in another PR and open it again. |
Fixes: #7114