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

Support a separate auth URL for external authentication #12697

Merged
merged 1 commit into from
Nov 21, 2016

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Nov 16, 2016

This will allow external auth to only do a single auth at
login, which is requried by OTP configurations.

https://bugzilla.redhat.com/show_bug.cgi?id=1390349

This will allow external auth to only do a single auth at
login, which is requried by OTP configurations.

https://bugzilla.redhat.com/show_bug.cgi?id=1390349
@jvlcek
Copy link
Member Author

jvlcek commented Nov 16, 2016

This PR depends on: ManageIQ/manageiq-appliance#101

@miq-bot
Copy link
Member

miq-bot commented Nov 16, 2016

Checked commit jvlcek@639a56e with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 0 offenses detected
Everything looks good. 🏆

@jvlcek
Copy link
Member Author

jvlcek commented Nov 16, 2016

@himdel Can you please review the UI changes?

@abellotti
Copy link
Member

👍 on authentication changes.

will need some more testing for ext-auth against AD. Also, separately might need an rpm .spec change (post section) to update the /etc/httpd/conf.d/manageiq-external-auth.conf file in place if present. so customers that have ext-auth configured and upgrade to CFME 5.7 will not have to reconfigure.

@jvlcek
Copy link
Member Author

jvlcek commented Nov 16, 2016

This change represents a team effort with much input from @abellotti

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

Looks good. thanks @jvlcek. Will wait for ok from UI team before merging.

@martinpovolny martinpovolny changed the title Support a seperate auth URL for external authentication Support a separate auth URL for external authentication Nov 18, 2016
@himdel
Copy link
Contributor

himdel commented Nov 18, 2016

The code changes look good, tested that the websocket notification authentication works too .. 👍 :)

@jvlcek
Copy link
Member Author

jvlcek commented Nov 18, 2016

I have successfully tested this fix with Ext Auth SSSD/LDAP

@gtanzillo gtanzillo added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 21, 2016
@gtanzillo gtanzillo merged commit ad93833 into ManageIQ:master Nov 21, 2016
@chessbyte
Copy link
Member

chessbyte pushed a commit that referenced this pull request Nov 21, 2016
Support a separate auth URL for external authentication
(cherry picked from commit ad93833)

https://bugzilla.redhat.com/show_bug.cgi?id=1397091
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log -1
commit 686bf6f7173062e4a76031a9a00def67cfea7ca1
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Mon Nov 21 10:11:24 2016 -0500

    Merge pull request #12697 from jvlcek/master_bz1390349_2fa
    
    Support a separate auth URL for external authentication
    (cherry picked from commit ad9383313d627499906d7ee44ee00c9f22341265)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1397091

@chessbyte
Copy link
Member

Darga Backport conflict:

$ git cherry-pick -e -x -m 1   ad93833
error: could not apply ad93833... Merge pull request #12697 from jvlcek/master_bz1390349_2fa
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

$ git status
On branch darga
Your branch is up-to-date with 'upstream/darga'.
You are currently cherry-picking commit ad93833.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   app/assets/javascripts/miq_application.js
	modified:   app/views/dashboard/login.html.haml
	modified:   config/routes.rb

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   app/controllers/dashboard_controller.rb

$ git diff
diff --cc app/controllers/dashboard_controller.rb
index 5e2f35f,6968355..0000000
--- a/app/controllers/dashboard_controller.rb
+++ b/app/controllers/dashboard_controller.rb
@@@ -1,13 -1,19 +1,22 @@@
  class DashboardController < ApplicationController
 -  include DashboardHelper
 -  include StartUrl
 -
 -  menu_section :vi
 -
    @@items_per_page = 8
  
++<<<<<<< HEAD
 +  before_action :check_privileges, :except => [:csp_report, :window_sizes, :authenticate, :kerberos_authenticate,
 +                                               :logout, :login, :login_retry, :wait_for_task,
 +                                               :saml_login, :initiate_saml_login]
 +  before_action :get_session_data, :except => [:csp_report, :window_sizes,
 +                                               :authenticate, :kerberos_authenticate, :saml_login]
++=======
+   before_action :check_privileges, :except => [:csp_report, :authenticate,
+                                                :external_authenticate, :kerberos_authenticate,
+                                                :logout, :login, :login_retry, :wait_for_task,
+                                                :saml_login, :initiate_saml_login]
+   before_action :get_session_data, :except => [:csp_report, :authenticate,
+                                                :external_authenticate, :kerberos_authenticate, :saml_login]
++>>>>>>> ad93833... Merge pull request #12697 from jvlcek/master_bz1390349_2fa
    after_action :cleanup_action,    :except => [:csp_report]
 -  after_action :set_session_data,  :except => [:csp_report]
 +  after_action :set_session_data,  :except => [:csp_report, :window_sizes]
  
    def index
      redirect_to :action => 'show'

@chessbyte
Copy link
Member

Backported to Darga via #12779

@jvlcek jvlcek deleted the master_bz1390349_2fa branch July 6, 2017 21:14
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.

6 participants