Add support to Enable OpenID-Connect in the appliance console. #48
Conversation
|
@miq-bot add_label authentication |
|
@jvlcek Cannot apply the following label because they are not recognized: authentication |
|
@miq-bot add_label gaprindashvili/no |
|
@miq-bot add_label wip |
jvlcek
changed the title
|
@jvlcek Cannot apply the following label because they are not recognized: gaprindashvili/no |
| result = ManageIQ::ApplianceConsole::Utilities.rake_run("evm:settings:set", params) | ||
| raise parse_errors(result).join(', ') if result.failure? | ||
| end | ||
| end | ||
|
|
||
| def never_enable_saml_and_oidc(params) | ||
| if params.include?("/authentication/oidc_enabled=true") && params.include?("/authentication/saml_enabled=true") | ||
| say("\nWARNING: Both SAML and OIDC can not be enable. SAML will be enabled ...") |
| result = ManageIQ::ApplianceConsole::Utilities.rake_run("evm:settings:set", params) | ||
| raise parse_errors(result).join(', ') if result.failure? | ||
| end | ||
| end | ||
|
|
||
| def never_enable_saml_and_oidc(params) |
There was a problem hiding this comment.
Methods that alter the parameter passed in should end in a !
| end | ||
| end | ||
|
|
||
| def remove_oidc(params) |
There was a problem hiding this comment.
These should also all end in ! to show that the parameter is being altered.
| end | ||
|
|
||
| if params.include?("/authentication/saml_enabled=true") | ||
| params = remove_oidc(params) |
There was a problem hiding this comment.
If you are editing the hash passed in, you don't need to re-assign.
| result = ManageIQ::ApplianceConsole::Utilities.rake_run("evm:settings:set", params) | ||
| raise parse_errors(result).join(', ') if result.failure? | ||
| end | ||
| end | ||
|
|
||
| def never_enable_saml_and_oidc(params) | ||
| if params.include?("/authentication/oidc_enabled=true") && params.include?("/authentication/saml_enabled=true") |
There was a problem hiding this comment.
I'm not sure what the user experience is here, but why can this even happen? Shouldn't we ask for the user to specify only one external auth type? Or check for this as they are selecting the options rather than assuming one way or the other after the fact?
Either way I think it would be better to fail here rather than just picking one of the options they said they want to configure.
There was a problem hiding this comment.
The logic in external_auth_options.rb expects all the options to be a toggle of a true or false value. The new authentication setting provider_type, with possible values of oidc, saml or none, is used by the UI to toggle oidc_enabled and saml_enabled.
After speaking with @abellotti about this, the agreed approach is to get the needed changes in that will make it work even if not elegant and avoid a larger refactoring, since the fate of OIDC on non-podified appliances is not finalized.
Because the appliance_console loops allowing the user to reselect options the params array could have many combinations. Without reworking external_auth_options.rb the solution I presented ensures correct results.
That said, I could error out if both oidc_enabled and saml_enabled are true.
There was a problem hiding this comment.
Oh and thanks for taking a look! @carbonin
There was a problem hiding this comment.
Okay, then I think a validate! which will error if there is an incompatible combination selected is a better experience than possibly configuring something the user may not have intended.
There was a problem hiding this comment.
The only incompatible combination it so enable both oidc and saml. I will add a validate! for that and error out.
Thanks!
| result = ManageIQ::ApplianceConsole::Utilities.rake_run("evm:settings:set", params) | ||
| raise parse_errors(result).join(', ') if result.failure? | ||
| end | ||
| end | ||
|
|
||
| def validate_provider_type | ||
| return true unless @updates["/authentication/oidc_enabled"] == true && @updates["/authentication/saml_enabled"] == true | ||
| say("\Error: Both SAML and OIDC can not be enable ...") |
| false | ||
| end | ||
|
|
||
| def enable_provider_type(params) |
There was a problem hiding this comment.
Since you're altering the passed-in hash here, I think this method should be named enable_provider_type!
| false | ||
| end | ||
|
|
||
| def enable_provider_type(params) |
There was a problem hiding this comment.
This should be named with a ! because you're altering the parameter passed into the method. (same with all the provider_type_* methods)
| end | ||
| end | ||
|
|
||
| def provider_type_saml(params) |
There was a problem hiding this comment.
I think I would prefer these have a method name that uses a verb as it's altering the hash. A noun method feels more like it's returning a value retrieved from somewhere else.
Also all of these should be named with a ! for the same reason as above.
There was a problem hiding this comment.
@carbonin Thank you! Will do.
|
Checked commits jvlcek/manageiq-appliance_console@b8440ee~...1e9d276 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
jvlcek
changed the title
|
@carbonin and @abellotti Can the failing codeclimate Complexity issue be waved? This PR and ManageIQ/manageiq#16567 need to be merged together. |
https://www.pivotaltracker.com/n/projects/1610127/stories/121849185
https://www.pivotaltracker.com/n/projects/1610127/stories/121849185
The change introduced by this PR will allow the appliance console support of the OpenID Connect protocol.
This is a WIP PR for now as we only support OpenID in the pods now with the configmap generator. Standalone appliance apache config does not support OpenID (i.e. we don't have the separate Apache templates & the needed manual steps in http://manageiq.org/docs/reference/ under Authentication).
The change introduced by this PR is reliant on the following work for full OpenID Connect support:
#16495
ManageIQ/manageiq-ui-classic#2855