Add OpenIDP Connect Form to Cluster Settings OAuth Page #1328

jcaianirh · 2019-03-23T03:40:44Z

https://jira.coreos.com/browse/CONSOLE-1285

Create form for OpenIDP Connect, and add route to the form via the Oath Cluster Settings Page. The form follows the design located here:
https://github.com/openshift/openshift-origin-design/blob/b1bb5ed27f9da2067f91fc24d3888c1e3efa0912/web-console/futures/oauth/oauth.md

jcaianirh · 2019-03-24T13:57:25Z

@bmignano The text specified for the Client Secret states:

This value may also be provided in an environment variable, external file, or encrypted file.

But the field is shown as required. Should this field not be marked required?

bmignano · 2019-03-25T13:44:46Z

@jcaianirh It's looking great! I think we can make that Client Secret field optional. I think we also probably want to include the default text for the claims fields (shown in the design doc).

spadgett · 2019-03-25T16:30:08Z

It looks like client secret is required in the API.

This value may also be provided in an environment variable, external file, or encrypted file.

@jcaianirh where is that text from?

jcaianirh · 2019-03-25T16:59:03Z

@spadgett The text came from @bmignano. I think she got the text here: https://docs.openshift.com/container-platform/3.7/install_config/configuring_authentication.html
Is that text outdated?

spadgett · 2019-03-25T17:03:52Z

OK, this has all changed in 4.0. It's no longer optional and must be provided in a secret.

https://github.com/openshift/api/blob/master/config/v1/types_oauth.go#L496-L500

spadgett · 2019-03-25T17:17:12Z

Claims is an array in the API. Are we going to provide a way to add additional claims? If not, we should at least adjust the heading and text.

There needs to be some additional help text on many of these fields.

It's not obvious to me how to enter extra scopes and extra authorize parameters since these aren't simple strings.

If we need to trim things back to the essentials, we can do that since feature freeze is rapidly approaching. We can always add to this form.

cc @enj

spadgett · 2019-03-25T20:27:29Z

Related API changes pending: openshift/api#265

jcaianirh · 2019-03-26T02:11:37Z

@spadgett added api changes for openid connect except for More Options. Do we want to move forward with that part of the form for openid? Added single array for claims. Removed the outdated client secret help text. Successfully created openid connect providers. @bmignano Added default values for claims text fields. Updated the description screenshot above.

@spadgett looks like urls and the two booleans are going away in favor of an issuer based on todays new api changes.

spadgett

Thanks @jcaianirh. The API changes are in place, so we should update this PR.

I realize this is WIP, but I don't see where you're handling the CA file. We'll need that.

We might punt on extra scopes and extra authorize parameters on the initial PR if they're not common. @enj could say better

frontend/public/components/cluster-settings/openid-idp-form.tsx

spadgett · 2019-03-26T11:50:44Z

frontend/public/components/cluster-settings/openid-idp-form.tsx

We should eventually create a common component for mapping method so we don't repeat it 9 times, but I'd leave it for now

frontend/public/components/cluster-settings/openid-idp-form.tsx

jcaianirh · 2019-03-26T18:22:52Z

@spadgett updated code to address comments. need to add ca.

frontend/public/components/cluster-settings/openid-idp-form.tsx

jcaianirh · 2019-03-26T19:31:36Z

@spadgett Added optional ca and replied to review comments except claims. What do you think of making them mandatory and removing the help text?

spadgett · 2019-03-26T19:56:22Z

@spadgett Added optional ca and replied to review comments except claims. What do you think of making them mandatory and removing the help text?

It doesn't look like you need to supply all of them though. We'll need confirmation from the auth team. Each of these is an array in the API.

@enj We need help understanding this :)

jcaianirh · 2019-03-26T20:31:37Z

@spadgett @bmignano updated screenshot.

spadgett · 2019-03-26T23:38:30Z

@enj I pushed a commit that allows you to set more than one claim value and extra scopes. We probably need better help text on these fields, but it's a start.

@jcaianirh PTAL

* Allow users to set more than one claim value and extra scopes * Add generic `ListInput` component * Clean up promises

spadgett · 2019-03-26T23:48:31Z

/lgtm
/hold

@jcaianirh I squashed the commits as well. Feel free to hold cancel if this looks good

openshift-ci-robot · 2019-03-26T23:48:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcaianirh, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jcaianirh · 2019-03-27T01:51:48Z

/hold cancel

openshift-bot · 2019-03-27T02:25:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-27T04:22:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-27T09:21:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2019-03-27T10:52:51Z

/retest

spadgett · 2019-03-27T12:31:39Z

It looks like the storage tests might be fixed now.

/retest

spadgett · 2019-03-27T12:50:49Z

/retest

spadgett · 2019-03-27T14:18:13Z

/retest

spadgett · 2019-03-27T14:44:10Z

/retest

openshift-bot · 2019-03-27T15:05:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2019-03-27T17:25:34Z

/retest

spadgett · 2019-03-27T17:26:26Z

/retest

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2019

openshift-ci-robot requested review from benjaminapetersen and dtaylor113 March 23, 2019 03:40

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 23, 2019

spadgett reviewed Mar 26, 2019

View reviewed changes

spadgett force-pushed the oath_idp branch from 3343f9f to 882f59b Compare March 26, 2019 23:37

spadgett force-pushed the oath_idp branch from 882f59b to 2ffb9fb Compare March 26, 2019 23:40

jcaianirh and others added 2 commits March 26, 2019 19:43

Add OpenID IDP Form to Cluster Settings OAuth Page

2f30fd1

Improve OpenID IDP Form

6d205f3

* Allow users to set more than one claim value and extra scopes * Add generic `ListInput` component * Clean up promises

spadgett force-pushed the oath_idp branch from 2ffb9fb to 6d205f3 Compare March 26, 2019 23:44

openshift-ci-robot assigned spadgett Mar 26, 2019

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 26, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2019

jcaianirh changed the title ~~[WIP]: Add OpenIDP Connect Form to Cluster Settings Oath Page~~ Add OpenIDP Connect Form to Cluster Settings Oath Page Mar 27, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2019

spadgett changed the title ~~Add OpenIDP Connect Form to Cluster Settings Oath Page~~ Add OpenIDP Connect Form to Cluster Settings OAuth Page Mar 27, 2019

openshift-merge-robot merged commit 625390c into openshift:master Mar 27, 2019

Add OpenIDP Connect Form to Cluster Settings OAuth Page #1328

Add OpenIDP Connect Form to Cluster Settings OAuth Page #1328

Uh oh!

Conversation

jcaianirh commented Mar 23, 2019 • edited by spadgett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcaianirh commented Mar 24, 2019

Uh oh!

bmignano commented Mar 25, 2019

Uh oh!

spadgett commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcaianirh commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented Mar 25, 2019

Uh oh!

spadgett commented Mar 25, 2019

Uh oh!

spadgett commented Mar 25, 2019

Uh oh!

jcaianirh commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spadgett Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcaianirh commented Mar 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcaianirh commented Mar 26, 2019

Uh oh!

spadgett commented Mar 26, 2019

Uh oh!

jcaianirh commented Mar 26, 2019

Uh oh!

spadgett commented Mar 26, 2019

Uh oh!

spadgett commented Mar 26, 2019

Uh oh!

openshift-ci-robot commented Mar 26, 2019

Uh oh!

jcaianirh commented Mar 27, 2019

Uh oh!

openshift-bot commented Mar 27, 2019

Uh oh!

openshift-bot commented Mar 27, 2019

Uh oh!

openshift-bot commented Mar 27, 2019

Uh oh!

spadgett commented Mar 27, 2019

Uh oh!

spadgett commented Mar 27, 2019

Uh oh!

spadgett commented Mar 27, 2019

Uh oh!

spadgett commented Mar 27, 2019

Uh oh!

spadgett commented Mar 27, 2019

Uh oh!

openshift-bot commented Mar 27, 2019

Uh oh!

spadgett commented Mar 27, 2019

Uh oh!

spadgett commented Mar 27, 2019

Uh oh!

jcaianirh commented Mar 23, 2019 •

edited by spadgett

Loading

spadgett commented Mar 25, 2019 •

edited

Loading

jcaianirh commented Mar 25, 2019 •

edited

Loading

jcaianirh commented Mar 26, 2019 •

edited

Loading