Merge pull request #351 from DigitalCurationCentre/vyruss/bugfixes

Vyruss/bugfixes

app/controllers/application_controller.rb

@@ -106,4 +106,15 @@ def errors_to_s(obj)
  msg
  end
  end
+
+ ##
+ # Sign out of Shibboleth SP local session too.
+ # -------------------------------------------------------------
+ def after_sign_out_path_for(resource_or_scope)
+ if Rails.application.config.shibboleth_enabled
+ return Rails.application.config.shibboleth_logout_url + root_url
+ super
+ end
+ end
+ # -------------------------------------------------------------
 end

app/controllers/phases_controller.rb

@@ -56,7 +56,7 @@ def edit
  end
  theme_guidance[title] << {
  text: guidance.text,
- org: guidance_group.name
+ org: guidance_group.name + ':'
  }
  end
  end

app/controllers/users/omniauth_callbacks_controller.rb

@@ -61,6 +61,7 @@ def handle_omniauth(scheme)
  end
  # -------------------------------------------------------------
 
+
  def failure
  redirect_to root_path
  end

app/views/devise/registrations/_external_identifier.html.erb

@@ -1,6 +1,4 @@
-<div class="user-identifier" 
- style="background-image: url(<%= "#{scheme.logo_url}" %>); background-size: 16px;">
-
+<div>
  <% if id.nil? || id.identifier == '' %>
  <%= link_to "#{_("Create or Link your #{scheme.description} ID")}",
  Rails.application.routes.url_helpers.send(
@@ -15,9 +13,9 @@
  <%= link_to "#{_("Your account has been linked to #{scheme.description}.")}", "#{scheme.user_landing_url}/#{id.identifier}", target: '_blank', 
  title: t("identifier_schemes.schemes.#{scheme.name}.connect_tooltip", default: "") %>
  <% end %>
- <%= link_to image_tag('remove.png', height: '16px', width: '16px'), 
+ <%= link_to '<i class="fa fa-fw fa-times-circle" aria-hidden="false"></i>'.html_safe, 
  destroy_user_identifier_path(id), method: :delete,
  title: _("Unlink your account from #{scheme.description}. You can link again at any time."),
- data: {confirm: _("Are you sure you want to unlink your #{scheme.description} ID?")} %>
+ data: {confirm: _("Are you sure you want to unlink #{scheme.description} ID?")} %>
  <% end %>
 </div>

app/views/devise/registrations/edit.html.erb

@@ -63,8 +63,14 @@
 
  <% @identifier_schemes.each do |scheme| %>
  <div>
- <label><%= scheme.name %></label>
- <div class="identifier-scheme identifier-scheme-indent">
+ <label>
+ <% if scheme.logo_url.nil? %>
+ <i class="fa fa-user" title="<%= scheme.name %>" aria-hidden="true" style="font-size:16px; width:16px; text-align:center;"></i>
+ <% else %>
+ <img title="<%= scheme.name %>" src=<%= "#{scheme.logo_url}" %> size="16px" />
+ <% end %>
+ </label>
+ <div class="identifier-scheme">
  <%= render partial: 'external_identifier', 
  locals: {scheme: scheme, 
  id: current_user.identifier_for(scheme)} %>

config/application.rb

@@ -91,12 +91,12 @@ class Application < Rails::Application
 
  # Enable shibboleth as an alternative authentication method
  # Requires server configuration and omniauth shibboleth provider configuration
- # See config/initializers/omniauth.rb
+ # See config/initializers/devise.rb
  config.shibboleth_enabled = true
 
- # Absolute path to Shibboleth SSO Login
- config.shibboleth_login = '/Shibboleth.sso/Login'
- 
+ # Relative path to Shibboleth SSO Logout
+ config.shibboleth_logout_url = '/Shibboleth.sso/Logout?return='
+
  # Active Record will no longer suppress errors raised in after_rollback or after_commit
  # in the next version. Devise appears to be using those callbacks.
  # To accept the new behaviour use 'true' otherwise use 'false'

config/routes.rb

@@ -46,7 +46,7 @@
 
  # fix for activeadmin signout bug
  devise_scope :user do
- get '/users/sign_out' => 'devise/sessions#destroy'
+ delete '/users/sign_out' => 'devise/sessions#destroy'
  end
 
  delete '/users/identifiers/:id', to: 'user_identifiers#destroy', as: 'destroy_user_identifier'

db/seeds.rb

@@ -8,7 +8,8 @@
  {name: 'orcid', description: 'ORCID', active: true, 
  logo_url:'http://orcid.org/sites/default/files/images/orcid_16x16.png', 
  user_landing_url:'https://orcid.org' },
- {name: 'shibboleth', description: 'Shibboleth', active: true}
+ {name: 'shibboleth', description: 'your institutional credentials', active: true,
+ },
 ]
 identifier_schemes.map{ |is| IdentifierScheme.create!(is) if IdentifierScheme.find_by(name: is[:name]).nil? }
 

test/functional/sessions_controller_test.rb

@@ -53,7 +53,11 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
  delete destroy_user_session_path
  assert_equal nil, session[:locale], "expected the locale to have been deleted from the session"
  assert_response :redirect
- assert_redirected_to root_path
+ if Rails.application.config.shibboleth_enabled
+ assert_redirected_to Rails.application.config.shibboleth_logout_url + root_url
+ else 
+ assert_redirected_to root_path
+ end
  end
 
 end

test/functional/users/omniauth_callbacks_controller_test.rb

@@ -50,8 +50,8 @@ class OmniauthCallbacksControllerTest < ActionDispatch::IntegrationTest
 
  ### Until ORCID login becomes supported.
  if scheme.name == 'shibboleth'
- assert [I18n.t('devise.omniauth_callbacks.user.success').gsub('%{kind}', scheme.name).downcase,
- I18n.t('devise.omniauth_callbacks.success').gsub('%{kind}', scheme.name).downcase].include?(flash[:notice].downcase), "Expected a success message when simulating a valid callback from #{scheme.name}"
+ assert [I18n.t('devise.omniauth_callbacks.user.success').gsub('%{kind}', scheme.description).downcase,
+ I18n.t('devise.omniauth_callbacks.success').gsub('%{kind}', scheme.description).downcase].include?(flash[:notice].downcase), "Expected a success message when simulating a valid callback from #{scheme.name}"
  assert @response.redirect_url.include?(root_url), "Expected a redirect to the root page, #{root_url}, when omniauth returns with a valid identifier!"
  else
  assert_equal I18n.t('identifier_schemes.new_login_success'), flash[:notice], "Expected a success message when simulating a valid callback from #{scheme.name}"

test/integration/authentication_test.rb

@@ -31,10 +31,14 @@ class AuthenticationFlowTest < ActionDispatch::IntegrationTest
  delete destroy_user_session_path
 
  assert_response :redirect
- follow_redirect!
+ if Rails.application.config.shibboleth_enabled
+ assert_redirected_to Rails.application.config.shibboleth_logout_url + root_url
+ else
+ assert_redirected_to root_path
+ end
+ get root_path
 
  # Make sure that the user is sent to the page that lists their plans
- assert_response :success
  assert_select '.welcome-message h2', _('Welcome.')
  end