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

Add support to Enable OpenID-Connect in the appliance console. #48

Merged
merged 7 commits into from
Jul 20, 2018
Merged

Add support to Enable OpenID-Connect in the appliance console. #48

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b8440ee
Add support to Enable OpenID-Connect in the appliance console.
jvlcek Jun 27, 2018
2f6a4fa
Address rubocop issues
jvlcek Jun 28, 2018
446050b
Remove useless assignment
jvlcek Jun 28, 2018
42c3d3f
Refactor to clean provider_type logic
jvlcek Jun 28, 2018
3920ee4
Fix typo
jvlcek Jun 28, 2018
edde501
rename enable_provider_type to enable_provider_type!
jvlcek Jun 28, 2018
1e9d276
Name methods making permanent changes using a verb.
jvlcek Jul 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions lib/manageiq/appliance_console/external_auth_options.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ class ExternalAuthOptions
AUTH_PATH = "/authentication".freeze

EXT_AUTH_OPTIONS = {
"#{AUTH_PATH}/sso_enabled" => {:label => "Single Sign-On", :logic => true},
"#{AUTH_PATH}/saml_enabled" => {:label => "SAML", :logic => true},
"#{AUTH_PATH}/local_login_disabled" => {:label => "Local Login", :logic => false}
"#{AUTH_PATH}/sso_enabled" => {:label => "Single Sign-On", :logic => true},
"#{AUTH_PATH}/saml_enabled" => {:label => "SAML", :logic => true},
"#{AUTH_PATH}/oidc_enabled" => {:label => "OIDC", :logic => true},
"#{AUTH_PATH}/local_login_disabled" => {:label => "Local Login", :logic => false}
}.freeze

include ManageIQ::ApplianceConsole::Logging
Expand Down Expand Up @@ -42,6 +43,7 @@ def ask_questions
end
end
@updates = {} if selection == skip
@updates = {} unless validate_provider_type
true
end

Expand Down Expand Up @@ -79,11 +81,44 @@ def update_configuration(update_hash = nil)
if update_hash.present?
say("\nUpdating external authentication options on appliance ...")
params = update_hash.collect { |key, value| "#{key}=#{value}" }
params = enable_provider_type(params)
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 ...")
Copy link
Member

Choose a reason for hiding this comment

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

Typo .. should be "enabled"

false
end

def enable_provider_type(params)
Copy link
Member

Choose a reason for hiding this comment

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

Since you're altering the passed-in hash here, I think this method should be named enable_provider_type!

Copy link
Member

@carbonin carbonin Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This should be named with a ! because you're altering the parameter passed into the method. (same with all the provider_type_* methods)

if params.include?("/authentication/saml_enabled=true")
provider_type_saml(params)
elsif params.include?("/authentication/oidc_enabled=true")
provider_type_oidc(params)
elsif params.include?("/authentication/oidc_enabled=false") || params.include?("/authentication/saml_enabled=false")
provider_type_none(params)
end
end

def provider_type_saml(params)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@carbonin Thank you! Will do.

params << "/authentication/oidc_enabled=false"
params << "/authentication/provider_type=saml"
end

def provider_type_oidc(params)
params << "/authentication/saml_enabled=false"
params << "/authentication/provider_type=oidc"
end

def provider_type_none(params)
params << "/authentication/oidc_enabled=false"
params << "/authentication/saml_enabled=false"
params << "/authentication/provider_type=none"
end

# extauth_opts option parser: syntax is key=value,key=value
# key is one of the EXT_AUTH_OPTIONS keys.
# value is one of 1, true, 0 or false.
Expand Down