Skip to content

add impersonate-group #9062

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

Merged
merged 1 commit into from
May 31, 2016
Merged

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented May 27, 2016

Adds the ability to specify groups on an impersonation request. This allows clean impersonation on requests through a loopback to our API. This is needed to enable the jenkins template to be created using user credentials in an admission plugin.

Impersonate-Group

@openshift/api-review

@deads2k
Copy link
Contributor Author

deads2k commented May 27, 2016

[test]

@deads2k
Copy link
Contributor Author

deads2k commented May 27, 2016

@liggitt @mfojtik review? The changes are significant, but the test coverage is pretty good.

}
// if groups are not specified, then we need to look them up differently depending on the type of user
// if they are specified, then they are the authority
groupsSpecified := len(subjects) > 1
Copy link
Contributor

@liggitt liggitt May 28, 2016

Choose a reason for hiding this comment

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

subjects might not be groups... shouldn't this be checking whether the header was specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

subjects might not be groups... shouldn't this be checking whether the header was specified?

In order to get more than one subject, you must have specified a group. The requestedUser is guaranteed to produce one subject.

I can rewrite if you find the other comparison more obvious, but it is effectively identical.

@mfojtik
Copy link
Contributor

mfojtik commented May 28, 2016

LGTM (except @liggitt comment, I think we should check it header was specified). Also tests looks superb.

@deads2k deads2k force-pushed the impersonate-group branch from 8eabff4 to e8180e5 Compare May 31, 2016 13:09
@deads2k
Copy link
Contributor Author

deads2k commented May 31, 2016

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented May 31, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4233/) (Image: devenv-rhel7_4298)

@deads2k deads2k force-pushed the impersonate-group branch from e8180e5 to a13fe3d Compare May 31, 2016 17:30
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to a13fe3d

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to a13fe3d

@openshift-bot
Copy link
Contributor

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

@openshift-bot openshift-bot merged commit ae8f74d into openshift:master May 31, 2016
@deads2k deads2k deleted the impersonate-group branch September 6, 2016 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants