Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rules review endpoint for other user #10611

Closed
wants to merge 2 commits into from

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Aug 24, 2016

Adds a subjectrulesreviews resource for looking at the rules on another user. project-admins and above have the power and can use it to see what permission a given user and/or groups have in their project.

Also adds options to oc policy can-i to wire the functionality together.

@openshift/api-review

@deads2k deads2k added this to the 1.4.0 milestone Aug 24, 2016
@deads2k
Copy link
Contributor Author

deads2k commented Aug 24, 2016

[test]

@@ -80,8 +80,22 @@ os::cmd::expect_success_and_text 'oc policy can-i --list' 'get update.*imagestre
os::cmd::expect_success_and_text 'oc policy can-i create pods --all-namespaces' 'yes'
os::cmd::expect_success_and_text 'oc policy can-i create pods' 'yes'
os::cmd::expect_success_and_text 'oc policy can-i create pods --as harold' 'no'
os::cmd::expect_failure 'oc policy can-i create pods --as harold --user harold'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially with the influx of these tests, can-i should have it's own suite in it's own file, not here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could possibly use os::cmd::expect_failure_and_text in some of these tests as well.

@mfojtik mfojtik self-assigned this Sep 6, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016
@mfojtik
Copy link
Member

mfojtik commented Sep 20, 2016

@smarterclayton ping (api review)

@mfojtik
Copy link
Member

mfojtik commented Sep 20, 2016

@deads2k i'm ok with this, @enj any objections?

also need a rebase

}
}

func (c *subjectRulesReviews) Create(subjectRulesReview *authorizationapi.SubjectRulesReview) (result *authorizationapi.SubjectRulesReview, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the named return values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the named return values?

Consistency with the rest. These all die when we switch to generated.

@enj
Copy link
Contributor

enj commented Sep 22, 2016

@mfojtik this seems reasonable to me.
cc @openshift/cli-review since this changes the CLI as well.

@fabianofranz
Copy link
Member

LGTM from a CLI perspective.

@stevekuznetsov
Copy link
Contributor

Still requesting the test bits

return ret, nil
}

func GetEffectivePolicyRules(ctx kapi.Context, ruleResolver rulevalidation.AuthorizationRuleResolver, clusterPolicyGetter client.ClusterPolicyLister) ([]authorizationapi.PolicyRule, []error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

godoc

@smarterclayton
Copy link
Contributor

Are these APIs now fractal in that we are creating all possible intersections of "target" / "data"?


// SubjectRulesReviewSpec adds information about how to conduct the check
type SubjectRulesReviewSpec struct {
// User is optional. At least one of User and Groups must be specified.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not an array of Subjects? Since this is SubjectRulesReview

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e. I'd probably want to use this on ServiceAccounts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not an array of Subjects? Since this is SubjectRulesReview

I was matching SubjectAccessReviewSpec. I would expect these look the same in v1 and we'd change them all in v2.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2016
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 00c8f7e

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9308/)

@deads2k
Copy link
Contributor Author

deads2k commented Sep 29, 2016

@mfojtik There's some sort of serialization problem, but the guts work correctly. Can I leave this with you?

@ncdc

@mfojtik
Copy link
Member

mfojtik commented Sep 30, 2016

@deads2k i will have a look today

@openshift-bot
Copy link
Contributor

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2016
@deads2k deads2k closed this Oct 18, 2016
@deads2k deads2k deleted the rules-review branch February 3, 2017 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-api-review needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants