diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 9ad13ce..9f7f372 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -21,8 +21,7 @@ # ------------------------------------------------------------- def handle_omniauth(scheme) user = User.from_omniauth(request.env["omniauth.auth"].nil? ? request.env : request.env["omniauth.auth"]) - identifier = UserIdentifier.where(identifier: request.env["omniauth.auth"].uid).first - + # If the user isn't logged in if current_user.nil? # If the uid didn't have a match in the system send them to register @@ -38,7 +37,7 @@ set_flash_message(:notice, :success, kind: scheme.description) if is_navigational_format? sign_in_and_redirect user, event: :authentication else - flash[:notice] = t('identifier_schemes.new_login_success') + flash[:notice] = _('Successfully signed in') redirect_to new_user_registration_url end end @@ -53,12 +52,19 @@ flash[:notice] = _('Your account has been successfully linked to %{scheme}.') % { scheme: scheme.description } else - flash[:notice] = _('Unable to link your account to %{scheme}.') % { scheme: scheme.description } + flash[:alert] = _('Unable to link your account to %{scheme}.') % { scheme: scheme.description } end - end - - if identifier.user.id != current_user.id - flash[:notice] = _("The current #{scheme.description} iD has been already linked to a user with email #{identifier.user.email}") + + else + # If a user was found but does NOT match the current user then the identifier has + # already been attached to another account (likely the user has 2 accounts) + identifier = UserIdentifier.where(identifier: request.env["omniauth.auth"].uid).first + if identifier.user.id != current_user.id + flash[:alert] = _("The current #{scheme.description} iD has been already linked to a user with email #{identifier.user.email}") + end + + # Otherwise, the identifier was found and it matches the one already associated + # with the current user so nothing else needs to be done end # Redirect to the User Profile page diff --git a/app/views/devise/registrations/_external_identifier_orcid.html.erb b/app/views/devise/registrations/_external_identifier_orcid.html.erb index 2ba5cf0..ca1db01 100644 --- a/app/views/devise/registrations/_external_identifier_orcid.html.erb +++ b/app/views/devise/registrations/_external_identifier_orcid.html.erb @@ -1,6 +1,6 @@
<% if id.nil? || id.identifier == '' %> - <%= link_to 'Create or Connect your Orcid iD', Rails.application.routes.url_helpers.send("user_#{scheme.name.downcase}_omniauth_authorize_path"), id:"connect-orcid-button", target: '_blank', title: t("identifier_schemes.schemes.#{scheme.name}.connect_tooltip", default: "") %> + <%= link_to 'Create or connect your ORCID iD', Rails.application.routes.url_helpers.send("user_#{scheme.name.downcase}_omniauth_authorize_path"), id:"connect-orcid-button", target: '_blank', title: t("identifier_schemes.schemes.#{scheme.name}.connect_tooltip", default: "") %> <% else %> <% if scheme.user_landing_url.nil? %> <%= _("Your account has been linked to #{scheme.description}.") %> diff --git a/lib/assets/stylesheets/dmproadmap/tabs.scss b/lib/assets/stylesheets/dmproadmap/tabs.scss index 5570e5a..b327b21 100644 --- a/lib/assets/stylesheets/dmproadmap/tabs.scss +++ b/lib/assets/stylesheets/dmproadmap/tabs.scss @@ -17,9 +17,14 @@ left: 1px; color: $white; background-color: $primary-color; - border-bottom: 2px solid $primary-color; text-decoration: none; font-weight: bold; + box-sizing: border-box; + border-width: 2px; + border-style: solid; + border-color: inherit; + border-bottom: 0px; + border-radius: 0px; } } li a:hover, diff --git a/test/functional/users/omniauth_callbacks_controller_test.rb b/test/functional/users/omniauth_callbacks_controller_test.rb index d4f7e58..f902709 100644 --- a/test/functional/users/omniauth_callbacks_controller_test.rb +++ b/test/functional/users/omniauth_callbacks_controller_test.rb @@ -54,7 +54,7 @@ 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}" + assert_equal _('Successfully signed in'), flash[:notice], "Expected a success message when simulating a valid callback from #{scheme.name}" assert @response.redirect_url.include?(new_user_registration_url), "Expected a redirect to the registration page when the user is not logged in and we received a valid callback from #{scheme.name}" assert_not "#user_identifiers[#{scheme.name}]".nil? end @@ -78,5 +78,32 @@ assert_equal usr.user_identifiers.find_by(identifier_scheme: scheme).identifier, 'foo:bar' end end + + # ------------------------------------------------------------- + test "An omniauth identifier cannot be tied to multiple accounts" do + new_account = User.create!(firstname: 'Tester', surname: 'Duplicate', + email: 'tester.duplicate@somewhereelse.org', + org: Org.first, accept_terms: true, + password: "password123", password_confirmation: "password123") + + @schemes.each do |scheme| + @user.user_identifiers << UserIdentifier.new(identifier_scheme: scheme, + identifier: "foo:bar") + sign_in new_account + + post @callback_uris[scheme.name] + + assert_equal _("The current #{scheme.description} iD has been already linked to a user with email #{@user.email}"), flash[:alert], "Expected that we could not attach #{scheme.name} to an account if it has already been associated with another account" + + assert_redirected_to "#{edit_user_registration_path}", "Expected a redirect to the edit profile page, #{edit_user_registration_path}, when omniauth returns with a valid identifier for a user that is already signed in!" + + # reload the user record and make sure the omniauth value NOT attached to their record + # and still associated with the original account + @user.reload + assert_equal @user.user_identifiers.find_by(identifier_scheme: scheme).identifier, 'foo:bar' + new_account.reload + assert new_account.user_identifiers.find_by(identifier_scheme: scheme).nil?, "Expected the new account to NOT be associated with the #{scheme.name}" + end + end end