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

[wip] added option to handle OAuth grants at a per-client granularity #7739

Closed
wants to merge 1 commit into from
Closed

[wip] added option to handle OAuth grants at a per-client granularity #7739

wants to merge 1 commit into from

Conversation

stevekuznetsov
Copy link
Contributor

@liggitt PTAL

How do we test this?

liggitt
liggitt reviewed Mar 2, 2016
View reviewed changes
pkg/auth/api/types.go
@@ -40,6 +40,7 @@ type Client interface {
GetSecret() string
GetRedirectUri() string
GetUserData() interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty sure this was made to match the osin client interface... we'll likely need to include the grant strategy in the "user data"... that's our bucket to plumb whatever data we want through all the osin apis

@stevekuznetsov stevekuznetsov force-pushed the skuznets/grants branch 4 times, most recently from d6cfdea to 820c55f Compare March 2, 2016 18:14
@stevekuznetsov
Copy link
Contributor Author

@liggitt addressed comments

@stevekuznetsov stevekuznetsov mentioned this pull request Mar 2, 2016
Closed
@stevekuznetsov
Copy link
Contributor Author

doc at openshift/openshift-docs#1682

@liggitt liggitt self-assigned this Mar 2, 2016
@stevekuznetsov stevekuznetsov force-pushed the skuznets/grants branch 2 times, most recently from 8593c59 to 046f655 Compare March 11, 2016 16:25
@stevekuznetsov
Copy link
Contributor Author

@liggitt @smarterclayton how has running hack/update-generated-conversion.sh left me in some sort of indeterminate state where re-running the script fails?

$ hack/update-generated-conversions.sh 
Generating for version v1beta3
# github.com/openshift/origin/pkg/api/v1
pkg/api/v1/conversion_generated.go:4805: undefined: serverapiv1 in serverapiv1.GrantHandlerType
pkg/api/v1/conversion_generated.go:5008: undefined: serverapi in serverapi.GrantHandlerType

@stevekuznetsov
Copy link
Contributor Author

Do we not support one api package importing another? Will we need to duplicate the GrantHandlerType code?

@stevekuznetsov stevekuznetsov force-pushed the skuznets/grants branch 2 times, most recently from c06414e to b6502c6 Compare March 11, 2016 17:16
stevekuznetsov
stevekuznetsov reviewed Mar 11, 2016
View reviewed changes
pkg/api/v1beta3/conversion_generated.go
@@ -6684,8 +6588,6 @@ func init() {
autoConvert_api_OAuthAuthorizeToken_To_v1beta3_OAuthAuthorizeToken,
autoConvert_api_OAuthClientAuthorizationList_To_v1beta3_OAuthClientAuthorizationList,
autoConvert_api_OAuthClientAuthorization_To_v1beta3_OAuthClientAuthorization,
autoConvert_api_OAuthClientList_To_v1beta3_OAuthClientList,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liggitt do we want custom conversion funcs for these?

@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 Mar 21, 2016
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2016
@stevekuznetsov
Copy link
Contributor Author

@liggitt time to revive this:

  • did we figure out the best way to test this? AFAIR:
    • start the OpenShift OAuth server
    • create clients with different grant strategies
    • ensure redirect happened when expected
  • do we want custom conversion funcs for v1beta3 stuff? do we need that compat?
  • do we want this PR to also persist the grants?

@deads2k
Copy link
Contributor

deads2k commented May 13, 2016

do we want this PR to also persist the grants?

I think the grants themselves are persisted. This is about helping us give some client "special" powers when the cluster-admin believes they are used as part of the infrastructure.

@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 11, 2016
@liggitt
Copy link
Contributor

liggitt commented Jul 1, 2016

closing in favor of #9616

@liggitt liggitt closed this Jul 1, 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
needs-api-review needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stevekuznetsov @openshift-bot @deads2k @liggitt @smarterclayton