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

Jump to bottom

Fixes #30535 - Use HTTP headers in puma #7909

Merged
merged 1 commit into from
Oct 19, 2020
+31 −22
Merged

Fixes #30535 - Use HTTP headers in puma #7909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion app/controllers/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ def ajax_request
def remote_user_provided?
return false unless Setting["authorize_login_delegation"]
return false if api_request? && !(Setting["authorize_login_delegation_api"])
(@remote_user = request.env["REMOTE_USER"]).present?
(@remote_user = request.env["HTTP_REMOTE_USER"]).present?
Copy link
Member

Choose a reason for hiding this comment

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

Where is @remote_user even used? I don't see it anywhere. It looks like this was used until 4e7ea9b but is now delegated to the abstract SSO provider. Feels like this part should also be delegated to the SSO provider.

end

def resource_base_with_search
Expand Down
4 changes: 2 additions & 2 deletions app/models/setting/auth.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ def self.default_settings
set('websockets_encrypt', N_("VNC/SPICE websocket proxy console access encryption (websockets_ssl_key/cert setting required)"), !!SETTINGS[:require_ssl], N_('Websockets encryption')),
set('login_delegation_logout_url', N_('Redirect your users to this url on logout (authorize_login_delegation should also be enabled)'), nil, N_('Login delegation logout URL')),
set('authorize_login_delegation_auth_source_user_autocreate', N_('Name of the external auth source where unknown externally authentication users (see authorize_login_delegation) should be created (If you want to prevent the autocreation, keep unset)'), 'External', N_('Authorize login delegation auth source user autocreate')),
set('authorize_login_delegation', N_("Authorize login delegation with REMOTE_USER environment variable"), false, N_('Authorize login delegation')),
set('authorize_login_delegation_api', N_("Authorize login delegation with REMOTE_USER environment variable for API calls too"), false, N_('Authorize login delegation API')),
set('authorize_login_delegation', N_("Authorize login delegation with REMOTE_USER HTTP header"), false, N_('Authorize login delegation')),
set('authorize_login_delegation_api', N_("Authorize login delegation with REMOTE_USER HTTP header for API calls too"), false, N_('Authorize login delegation API')),
hsahmed marked this conversation as resolved.
Show resolved Hide resolved
set('idle_timeout', N_("Log out idle users after a certain number of minutes"), 60, N_('Idle timeout')),
set('bcrypt_cost', N_("Cost value of bcrypt password hash function for internal auth-sources (4-30). Higher value is safer but verification is slower particularly for stateless API calls and UI logins. Password change needed to take effect."), 4, N_('BCrypt password cost')),
set('bmc_credentials_accessible', N_("Permits access to BMC interface passwords through ENC YAML output and in templates"), true, N_('BMC credentials access')),
Expand Down
14 changes: 7 additions & 7 deletions app/services/sso/apache.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ module SSO
class Apache < Base
delegate :session, :to => :controller

CAS_USERNAME = 'REMOTE_USER'
CAS_USERNAME = 'HTTP_REMOTE_USER'
Copy link
Member

Choose a reason for hiding this comment

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

I like this constant a lot more than duplicating it everywhere. Not going to block on it, but I'd like to see similar constants for HTTP_REMOTE_USER_GROUPS + email + firstname + lastname.

However, it is weird that the constant is only used here and not in ApplicationController.

ENV_TO_ATTR_MAPPING = {
'REMOTE_USER_EMAIL' => :mail,
'REMOTE_USER_FIRSTNAME' => :firstname,
'REMOTE_USER_LASTNAME' => :lastname,
'HTTP_REMOTE_USER_EMAIL' => :mail,
'HTTP_REMOTE_USER_FIRSTNAME' => :firstname,
'HTTP_REMOTE_USER_LASTNAME' => :lastname,
}

def available?
Expand All @@ -30,10 +30,10 @@ def support_fallback?
def authenticated?
return false unless (self.user = request.env[CAS_USERNAME])
attrs = { :login => user }.merge(additional_attributes)
group_count = request.env['REMOTE_USER_GROUP_N'].to_i
if group_count > 0
if request.env.has_key?('HTTP_REMOTE_USER_GROUPS')
attrs[:groups] = []
group_count.times { |i| attrs[:groups] << request.env["REMOTE_USER_GROUP_#{i + 1}"] }
groups = request.env['HTTP_REMOTE_USER_GROUPS'].split(':')
groups.each { |group| attrs[:groups] << group }
end

return false unless User.find_or_create_external_user(attrs, Setting['authorize_login_delegation_auth_source_user_autocreate'])
Expand Down
11 changes: 11 additions & 0 deletions db/migrate/20200918223816_update_login_delegation_description.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class UpdateLoginDelegationDescription < ActiveRecord::Migration[6.0]
def up
Setting.where(name: 'authorize_login_delegation').update_all(description: "Authorize login delegation with REMOTE_USER HTTP header")
Setting.where(name: 'authorize_login_delegation_api').update_all(description: "Authorize login delegation with REMOTE_USER HTTP header for API calls too")
end

def down
hsahmed marked this conversation as resolved.
Show resolved Hide resolved
Setting.where(name: 'authorize_login_delegation').update_all(description: "Authorize login delegation with REMOTE_USER environment variable")
Setting.where(name: 'authorize_login_delegation_api').update_all(description: "Authorize login delegation with REMOTE_USER environment variable for API calls too")
end
end
2 changes: 1 addition & 1 deletion test/controllers/api/v2/hosts_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ def last_record
end

def set_remote_user_to(user)
@request.env['REMOTE_USER'] = user.login
@request.env['HTTP_REMOTE_USER'] = user.login
end

test "when REMOTE_USER is provided and both authorize_login_delegation{,_api}
Expand Down
2 changes: 1 addition & 1 deletion test/controllers/hosts_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,7 +853,7 @@ def setup_multiple_compute_resource
end

def set_remote_user_to(user)
@request.env['REMOTE_USER'] = user.login
@request.env['HTTP_REMOTE_USER'] = user.login
end

context 'submit actions with multiple hosts' do
Expand Down
8 changes: 4 additions & 4 deletions test/controllers/users_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ class UsersControllerTest < ActionController::TestCase
Setting['authorize_login_delegation'] = true
Setting['authorize_login_delegation_auth_source_user_autocreate'] = 'apache'
time = Time.zone.now
@request.env['REMOTE_USER'] = users(:admin).login
@request.env['HTTP_REMOTE_USER'] = users(:admin).login
get :extlogin, session: {:user => users(:admin).id }
assert_redirected_to hosts_path
users(:admin).reload
Expand All @@ -244,15 +244,15 @@ class UsersControllerTest < ActionController::TestCase
Setting['authorize_login_delegation'] = true
Setting['authorize_login_delegation_auth_source_user_autocreate'] = 'apache'
users(:external).update(disabled: true)
@request.env['REMOTE_USER'] = users(:external).login
@request.env['HTTP_REMOTE_USER'] = users(:external).login
get :extlogin, session: {:user => users(:external).id }
assert_redirected_to '/users/login'
end

test "should login external user preserving uri" do
Setting['authorize_login_delegation'] = true
Setting['authorize_login_delegation_auth_source_user_autocreate'] = 'apache'
@request.env['REMOTE_USER'] = users(:admin).login
@request.env['HTTP_REMOTE_USER'] = users(:admin).login
get :extlogin, session: { :original_uri => '/test' }
assert_redirected_to '/test'
end
Expand All @@ -261,7 +261,7 @@ class UsersControllerTest < ActionController::TestCase
Setting['authorize_login_delegation'] = true
Setting['authorize_login_delegation_auth_source_user_autocreate'] = 'apache_mod'
@request.session.clear
@request.env['REMOTE_USER'] = 'ares'
@request.env['HTTP_REMOTE_USER'] = 'ares'
get :extlogin
assert_redirected_to edit_user_path(User.unscoped.find_by_login('ares'))
end
Expand Down
10 changes: 4 additions & 6 deletions test/unit/sso/apache_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ def test_authenticated_passes_attributes
apache = get_apache_method

apache.controller.request.env[SSO::Apache::CAS_USERNAME] = 'ares'
apache.controller.request.env['REMOTE_USER_EMAIL'] = 'foobar@example.com'
apache.controller.request.env['REMOTE_USER_FIRSTNAME'] = 'Foo'
apache.controller.request.env['REMOTE_USER_LASTNAME'] = 'Bar'
apache.controller.request.env['HTTP_REMOTE_USER_EMAIL'] = 'foobar@example.com'
apache.controller.request.env['HTTP_REMOTE_USER_FIRSTNAME'] = 'Foo'
apache.controller.request.env['HTTP_REMOTE_USER_LASTNAME'] = 'Bar'
User.expects(:find_or_create_external_user).
with({:login => 'ares', :mail => 'foobar@example.com', :firstname => 'Foo', :lastname => 'Bar'}, 'apache').
returns(User.new)
Expand All @@ -67,10 +67,8 @@ def test_authenticated_parses_user_groups
apache = get_apache_method

apache.controller.request.env[SSO::Apache::CAS_USERNAME] = 'ares'
apache.controller.request.env['REMOTE_USER_GROUP_N'] = 2
existing = FactoryBot.build :usergroup
apache.controller.request.env['REMOTE_USER_GROUP_1'] = existing.name
apache.controller.request.env['REMOTE_USER_GROUP_2'] = 'does-not-exist-for-sure'
apache.controller.request.env['HTTP_REMOTE_USER_GROUPS'] = "#{existing.name}:does-not-exist-for-sure"
User.expects(:find_or_create_external_user).
with({:login => 'ares', :groups => [existing.name, 'does-not-exist-for-sure']}, 'apache').
returns(User.new)
Expand Down