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

Ensure CLI and web console clients are public OAuth clients #10819

Merged
merged 1 commit into from
Sep 27, 2016
Merged

Ensure CLI and web console clients are public OAuth clients #10819

merged 1 commit into from
Sep 27, 2016

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Sep 6, 2016

Lays the groundwork for authorization code flows from these clients in 1.4

@liggitt
Copy link
Contributor Author

liggitt commented Sep 6, 2016

[test]

return err
}
}

{
cliClient := oauthapi.OAuthClient{
ObjectMeta: kapi.ObjectMeta{Name: OpenShiftCLIClientID},
Secret: uuid.New(),
Secret: "",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming you are using "" here to be explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right

@enj
Copy link
Contributor

enj commented Sep 7, 2016

@liggitt Minor comment, but otherwise LGTM - I had a similar change in #10225 which I can remove once this is merged.

@liggitt liggitt added this to the 1.4.0 milestone Sep 7, 2016
@liggitt liggitt added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2016
assetPublicURLs := []string{}
if !options.DisabledFeatures.Has(configapi.FeatureWebConsole) {
assetPublicURLs = []string{options.OAuthConfig.AssetPublicURL, "http://localhost:9000", "https://localhost:9000"}
assetPublicURLs = []string{options.OAuthConfig.AssetPublicURL}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jwforres @spadgett this means that the development web console wouldn't be allowed as a redirect out of the box. do you have instructions or a bring-up script for starting an openshift master where you could include instructions to allow redirects to https://localhost:9000?

Copy link
Member

Choose a reason for hiding this comment

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

almost everyone on the UI team is using oc cluster up now, what are the steps that would need to be run? cc @csrwng

@rhamilto has a README he has been working on for environment set up instructions for UI development

Copy link
Member

Choose a reason for hiding this comment

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

@rhamilto has a README he has been working on for environment set up instructions for UI development

https://github.com/rhamilto/origin-web-console/wiki/Dev-env-setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what are the steps that would need to be run?

after the master starts, add https://localhost:9000 to the redirectURIs list

oc patch oauthclient/openshift-web-console -p '{"redirectURIs":["https://localhost:9000"]}'

Copy link
Member

Choose a reason for hiding this comment

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

is that going to delete the existing redirectURI for the embedded console,
do we need to specify both?

On Mon, Sep 12, 2016 at 9:39 AM, Jordan Liggitt [email protected]
wrote:

In pkg/cmd/server/origin/auth_config.go
#10819 (comment):

assetPublicURLs := []string{}
if !options.DisabledFeatures.Has(configapi.FeatureWebConsole) {
  •   assetPublicURLs = []string{options.OAuthConfig.AssetPublicURL, "http://localhost:9000", "https://localhost:9000"}
    
  •   assetPublicURLs = []string{options.OAuthConfig.AssetPublicURL}
    

what are the steps that would need to be run?

after the master starts, add https://localhost:9000 to the redirectURIs
list

oc patch oauthclient/openshift-web-console -p '{"redirectURIs":["https://localhost:9000"]}'


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/10819/files/a0578bff9477ca300b23c0f364bcdf929e9c69d5#r78373970,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7cNJlUNd4-lBSnasn3Ub0ftXo-ZVks5qpVYqgaJpZM4J17f2
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with the patchStrategy:"merge" change, a strategic patch (the default for oc patch) will merge the given list with the existing list

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's that far-fetched for 'oc cluster up' to simply add localhost:9000 as a redirect URI. It's meant to be a tool to help us get environments up for development, and this seems like a common use case. I'll open an issue.

@liggitt can you think of bad things happening because we add that as a default for cluster up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that seems reasonable in a cluster up scenario

@liggitt liggitt mentioned this pull request Sep 12, 2016
@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 12, 2016

continuous-integration/openshift-jenkins/test Waiting: Determining build queue position

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f4fdc8c

@openshift-bot
Copy link
Contributor

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

@liggitt
Copy link
Contributor Author

liggitt commented Sep 27, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f4fdc8c

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 27, 2016

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

@openshift-bot openshift-bot merged commit 5b31ed9 into openshift:master Sep 27, 2016
@liggitt liggitt deleted the public-oauth-clients branch September 28, 2016 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants