use the upstream authorization filters#16110
use the upstream authorization filters#16110openshift-merge-robot merged 4 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-merge-robot
added
the
approved
deads2k
force-pushed
the
server-40-normal-authz
branch
2 times, most recently
from
1e3e781 to
3ea481e
Compare
pkg/cmd/server/origin/master.go
Outdated
| // these are all equivalent to the kube handler chain | ||
| /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| handler = serverhandlers.AuthorizationFilter(handler, c.Authorizer, c.AuthorizationAttributeBuilder, genericConfig.RequestContextMapper) | ||
| handler = apifilters.WithAuthorization(apiHandler, c.RequestContextMapper, c.Authorizer) |
There was a problem hiding this comment.
I think you want handler, not apiHandler.
There was a problem hiding this comment.
Yikes. Hopefully that's what failed test-cmd.
test/cmd/authentication.sh
Outdated
| os::cmd::expect_success_and_text "oc get user/~ --token='${whoamitoken}'" "${username}" | ||
| os::cmd::expect_success_and_text "oc whoami --token='${whoamitoken}'" "${username}" | ||
| os::cmd::expect_failure_and_text "oc get pods --token='${whoamitoken}' -n '${project}'" "prevent this action; User \"scoped-user\" cannot list pods in project \"${project}\"" | ||
| os::cmd::expect_failure_and_text "oc get pods --token='${whoamitoken}' -n '${project}'" "prevent this action; User \"scoped-user\" cannot list pods in the namespace \"${project}\"" |
There was a problem hiding this comment.
I feel like this going to break people. And I am a bit confused why our forbidden message maker is not keeping this from changing.
There was a problem hiding this comment.
The filter response writer upstream (what this is using) is shaped this way. For all but get projects, this is a fair equivalent to have to deal with.
| os::cmd::expect_success_and_text 'oc status --all-namespaces' "Showing all projects on server" | ||
| # make sure `oc status` does not re-use the "no projects" message from `oc login` if -n is specified | ||
| os::cmd::expect_failure_and_text 'oc status -n forbidden' 'Error from server \(Forbidden\): User "test-user" cannot get project "forbidden"' | ||
| os::cmd::expect_failure_and_text 'oc status -n forbidden' 'Error from server \(Forbidden\): User "test-user" cannot get projects in the namespace "forbidden"' |
There was a problem hiding this comment.
This is confusing, to say the least.
There was a problem hiding this comment.
This particular one is ugly. I'd rather take the ugly and try to make it prettier upstream. Having separate handling chains is causing us to keep and track bugs that are already fixed upstream.
openshift-merge-robot
added
the
needs-rebase
deads2k
force-pushed
the
server-40-normal-authz
branch
from
3ea481e to
c5518d1
Compare
openshift-merge-robot
removed
the
needs-rebase
|
updated |
deads2k
force-pushed
the
server-40-normal-authz
branch
from
c5518d1 to
96c02b3
Compare
deads2k
force-pushed
the
server-40-normal-authz
branch
from
96c02b3 to
5f83f0c
Compare
|
/retest |
|
got an ok on irc from @enj |
|
Automatic merge from submit-queue |
5 participants
This brings in a config option from upstream and uses it to remove custom handler code we had.
@openshift/sig-security