🐛 pkg/server: ensure that home workspace handler gets authz with audit … #2628
Conversation
| apiHandler = genericapiserver.DefaultBuildHandlerChainFromAuthz(apiHandler, genericConfig) | ||
| genericConfig.Authorization.Authorizer = authorizerWithoutAudit |
There was a problem hiding this comment.
isn't authorizerWithoutAudit what we pass to WithHomeWorkspaces ? (line 342)
There was a problem hiding this comment.
if I don't pass it there, I don't get audits at all.
There was a problem hiding this comment.
yeah, i believe something changed and WithHomeWorkspaces now hosts regular workspaces as well after the refactoring? (not sure)
There was a problem hiding this comment.
my audit logs are empty with what is in main without this PR.
There was a problem hiding this comment.
shouldn't we be passing an authoriser decorated with authorization.EnableAuditLogging ?
There was a problem hiding this comment.
yes, that's done in line 335 👍
There was a problem hiding this comment.
genericConfig.Authorization.Authorizer = authorization.EnableAuditLogging(genericConfig.Authorization.Authorizer)
There was a problem hiding this comment.
don't we pass genericConfig.Authorization.Authorizer which is overwritten in line 337 (authorizerWithoutAudit) to WithHomeWorkspaces?
There was a problem hiding this comment.
he, yes, that's confusing 😆 You're right, but the audit enabled handler is given to the wrapped apiHandler. I'll comment to make it clearer 👍
s-urbaniak
force-pushed
the
issue-2627
branch
from
6c3cc1d to
d63ceec
Compare
s-urbaniak
changed the title
s-urbaniak
force-pushed
the
issue-2627
branch
from
d63ceec to
4f4771f
Compare
|
/lgtm thanks for the comment! |
s-urbaniak
force-pushed
the
issue-2627
branch
from
4f4771f to
4ec26db
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/lgtm |
…logging
Summary
This resolves #2627
This resolves #2627
This resolves #2627
This resolves #2627
Related issue(s)
Fixes #2627