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

Add OpenID-Connect authentication support #2855

Merged
merged 9 commits into from
Jul 13, 2018
Merged

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Nov 28, 2017

https://www.pivotaltracker.com/n/projects/1610127/stories/121849185

The changes introduced by this PR will provide support for enabling OpenID-Connect
authentication.

This PR depends on PR16495

Steps for Testing/QA [Optional]

  1. Install and configure Identity and Access Management server, such as Keycloak, for the OpenID-Connect provider type
  2. Install and configure the Apache module: mod_auth_openidc on the ManageIQ appliance to point to the Identity and Access Management server configured in step 1
  3. Log into the appliance as admin and configure authentication for OpenId-Connect by checking the Provider Type: button Enable OpenID-Connect
  4. Log out and back in by selecting the Log In to Corporate System button on the log in screen specifying valid credentials, as configured on the Identity and Access Management server

@jvlcek
Copy link
Member Author

jvlcek commented Nov 28, 2017

@miq-bot add_labels authentication, enhancement, gaprindashvili/no

@miq-bot
Copy link
Member

miq-bot commented Nov 28, 2017

@jvlcek Cannot apply the following label because they are not recognized: authentication

@jvlcek
Copy link
Member Author

jvlcek commented Nov 28, 2017

@miq-bot add_label wip

@miq-bot miq-bot added the wip label Nov 28, 2017
def oidc_login
request.env.each do |key, value|
$log.info(" request.env[#{key}] = #{value}") if key =~ /^HTTP_/
end
Copy link
Member

Choose a reason for hiding this comment

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

probably left over from debugging all this fun stuff

Copy link
Member Author

Choose a reason for hiding this comment

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

Good. Catch. 👍

@miq-bot
Copy link
Member

miq-bot commented May 3, 2018

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Jun 4, 2018

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Jun 8, 2018

@jvlcek Cannot apply the following label because they are not recognized: authentication

@jvlcek jvlcek force-pushed the openidc branch 2 times, most recently from 270ffe7 to bf4a216 Compare June 12, 2018 14:09
@jvlcek jvlcek changed the title [WIP] Add OpenID-Connect authentication support Add OpenID-Connect authentication support Jun 22, 2018
@jvlcek
Copy link
Member Author

jvlcek commented Jun 22, 2018

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Jun 22, 2018
@jvlcek
Copy link
Member Author

jvlcek commented Jun 22, 2018

@eclarizio and @himdel Please review.

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

I'm not super well versed in this area, but is there any way we can add specs to cover primarily the code that was moved into the #identity_provider_login method? Maybe even extracting all of that logic out into a service so that it can be unit tested without having to set up a bunch of variables for the dashboard_controller?

This is a similar approach to the PrivilegeCheckerService that I remember we were looking at while debugging this problem before, but again, not sure if you think that is a good strategy to employ here or not.

@jvlcek
Copy link
Member Author

jvlcek commented Jun 25, 2018

@eclarizio Thank you for the feedback. I'll take a look at PrivilegeCHeckerService .

JoeV

@jvlcek
Copy link
Member Author

jvlcek commented Jul 11, 2018

@eclarizio As we discussed extracting logic in the new #identity_provider_login method into a service does not seem like a good solution.

I believe I have addressed your fundamental concern of having spec tests to exercise the logic in the new #identity_provider_login method by adding a context "OIDC support" do test to the dashboard_controller_spec.rb.

The existing context "SAML support" do test was already exercising some of the logic in the new #identity_provider_login method.

Please let me know if this addresses your fundamental concern.

Thank you! JoeV

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

@jvlcek Yeah, sounds good to me! 👍

@miq-bot
Copy link
Member

miq-bot commented Jul 12, 2018

Some comments on commits jvlcek/manageiq-ui-classic@fd09be0~...ab0960a

spec/controllers/dashboard_controller_spec.rb

  • ⚠️ - 127 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Jul 12, 2018

Checked commits jvlcek/manageiq-ui-classic@fd09be0~...ab0960a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@mzazrivec mzazrivec self-assigned this Jul 13, 2018
@mzazrivec mzazrivec added this to the Sprint 90 Ending Jul 16, 2018 milestone Jul 13, 2018
@mzazrivec mzazrivec merged commit 44e1922 into ManageIQ:master Jul 13, 2018
@jvlcek jvlcek deleted the openidc branch August 7, 2018 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants