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.

Sign up for GitHub

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

Jump to bottom

Add option to handle OAuth grants at a per-client granularity #9616

Merged
merged 1 commit into from Jul 13, 2016
Merged

Add option to handle OAuth grants at a per-client granularity #9616

merged 1 commit into from Jul 13, 2016

Conversation

sgallagher
Copy link
Contributor

@liggitt Pushing my WIP so you can take a look.

@liggitt liggitt self-assigned this Jun 28, 2016
@liggitt
Copy link
Contributor

liggitt commented Jun 28, 2016 •
edited

looks good at a quick glance. I think we'll want to update CreateOrUpdateDefaultOAuthClients to set the grant strategy to auto for the three clients we create (unless the clients already exist and have a non-empty grant strategy set)

@sgallagher sgallagher force-pushed the grants-wip branch 3 times, most recently from 8278a5d to 328fafc Compare June 29, 2016 19:44
@sgallagher
Copy link
Contributor Author

OK, I set the grant strategy to auto for those clients when they are created. The code will currently only write the grant strategy once and keep it that way, even if we were to change the default in the code. As @liggitt explained on IRC, we'd want an upgraded cluster to maintain its old behavior, even if we eventually change the default grant strategy.

liggitt
liggitt reviewed Jun 29, 2016
View reviewed changes
pkg/auth/oauth/handlers/grant.go
func NewServiceAccountAwareGrant(standardGrantHandler, saClientGrantHandler GrantHandler) GrantHandler {
return &serviceAccountAwareGrant{standardGrantHandler: standardGrantHandler, saClientGrantHandler: saClientGrantHandler}
// NewPerClientGrant returns a grant handler that determines what to do based on the grant strategy in the client
func NewPerClientGrant(prompt GrantHandler, defaultStrategy oauthapi.GrantHandlerType, defaultServiceAccountStrategy oauthapi.GrantHandlerType) GrantHandler {
Copy link
Contributor

@liggitt liggitt Jun 29, 2016 •
edited

Choose a reason for hiding this comment

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

I think we'd want to plumb defaultServiceAccountStrategy into NewServiceAccountOAuthClientGetter instead, so that the OAuthClient returned by that method was accurate. That would mean by the time this was handed the OAuthClient for a serviceaccount, it would already have a grantStrategy

otherwise, anything using NewServiceAccountOAuthClientGetter is getting incomplete OAuthClient objects and has to do the service account detection/defaulting logic themselves

@liggitt
Copy link
Contributor

liggitt commented Jun 30, 2016

[test]

@sgallagher sgallagher force-pushed the grants-wip branch 2 times, most recently from 2e68b6b to c97f2ed Compare June 30, 2016 18:46
@sgallagher
Copy link
Contributor Author

@liggitt PTAL
I think I've addressed your concerns with the latest patch.

@liggitt liggitt mentioned this pull request Jul 1, 2016
Closed
liggitt
liggitt reviewed Jul 1, 2016
View reviewed changes
pkg/cmd/server/origin/master.go
@@ -540,7 +541,7 @@ func (c *MasterConfig) GetRestStorage() map[string]rest.Storage {
clientStorage, err := clientetcd.NewREST(c.RESTOptionsGetter)
checkStorageErr(err)
clientRegistry := clientregistry.NewRegistry(clientStorage)
combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(c.KubeClient(), c.KubeClient(), clientRegistry)
combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(c.KubeClient(), c.KubeClient(), clientRegistry, oauthapi.GrantHandlerType(c.Options.OAuthConfig.GrantConfig.Method))
Copy link
Contributor

Choose a reason for hiding this comment

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

OAuthConfig can be nil if OAuth is not enabled... I'd probably default to deny in that case here... not sure

Copy link
Contributor

Choose a reason for hiding this comment

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

should be ServiceAccountMethod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't realize OAuth could be disabled. (How does login work with out it?)
I'll check for that.

And yeah, typo. Thanks for catching it.

liggitt
liggitt reviewed Jul 1, 2016
View reviewed changes
pkg/oauth/api/v1/types.go
// - auto: always approves grant requests, useful for trusted clients
// - prompt: prompts the end user for approval of grant requests, useful for third-party clients
// - deny: always denies grant requests, useful for black-listed clients
GrantStrategy GrantHandlerType `json:"grantStrategy,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

this has been controlled by a field named "method" (or "serviceAccountMethod") in the config up until now... let's keep that terminology and use GrantMethod (and grantMethod in the JSON)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same thought - similar naming in config and API.

@sgallagher sgallagher force-pushed the grants-wip branch 2 times, most recently from 28a7041 to 2663bd5 Compare July 1, 2016 17:34
@sgallagher
Copy link
Contributor Author

@liggitt PTAL

I renamed the "method" pieces to strategy and fixed that typo with ServiceAccountMethod

@sgallagher sgallagher changed the title [WIP] Add option to handle OAuth grants at a per-client granularity Add option to handle OAuth grants at a per-client granularity Jul 1, 2016
@liggitt
Copy link
Contributor

liggitt commented Jul 6, 2016

test failure:

# github.com/openshift/origin/pkg/serviceaccounts/oauthclient
_output/local/go/src/github.com/openshift/origin/pkg/serviceaccounts/oauthclient/oauthclientregistry_test.go:110: not enough arguments in call to NewServiceAccountOAuthClientGetter

@sgallagher sgallagher mentioned this pull request Jul 11, 2016
Closed
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2016
@sgallagher sgallagher mentioned this pull request Jul 11, 2016
Merged
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2016
@liggitt
Copy link
Contributor

liggitt commented Jul 12, 2016

LGTM, squash and merge

@sgallagher
Add option to handle OAuth grants at a per-client granularity
95eb9bc 
Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 95eb9bc

@sgallagher
Copy link
Contributor Author

@liggitt Squashed, please merge.

@liggitt
Copy link
Contributor

liggitt commented Jul 12, 2016

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 12, 2016 •
edited

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

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 95eb9bc

@openshift-bot
Copy link
Contributor

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

@openshift-bot openshift-bot merged commit b58281f into openshift:master Jul 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
api-approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sgallagher @liggitt @openshift-bot @smarterclayton