Merge branch 'security_dblessing_invite_link_any_email' into 'master'

Only allow invite to be accepted by user with matching email

See merge request gitlab-org/security/gitlab!1557

app/controllers/invites_controller.rb

@@ -20,7 +20,7 @@ class InvitesController < ApplicationController
   end
 
   def accept
-    if member.accept_invite!(current_user)
+    if current_user_matches_invite? && member.accept_invite!(current_user)
       redirect_to invite_details[:path], notice: helpers.invite_accepted_notice(member)
     else
       redirect_back_or_default(options: { alert: _("The invitation could not be accepted.") })
@@ -52,7 +52,7 @@ class InvitesController < ApplicationController
   end
 
   def current_user_matches_invite?
-    @member.invite_email == current_user.email
+    current_user.verified_emails.include?(@member.invite_email)
   end
 
   def member?

app/views/invites/show.html.haml

 - page_title _("Invitation")
 %h3.page-title= _("Invitation")
 
-%p
-  = _("You have been invited")
-  - inviter = @member.created_by
-  - if inviter
-    = _("by")
-    = link_to inviter.name, user_url(inviter)
-  = _("to join %{source_name}") % { source_name: @invite_details[:title] }
-  %strong
-    = link_to @invite_details[:name], @invite_details[:url]
-  = _("as %{role}.") % { role: @member.human_access }
-
-- if member?
+- if current_user_matches_invite?
+  - if member?
     %p
-    = _("However, you are already a member of this %{member_source}. Sign in using a different account to accept the invitation.") % { member_source: @invite_details[:title] }
+      = _("You are already a member of this %{member_source}.") % { member_source: @invite_details[:title] }
+    .actions
+      = link_to _("Go to %{source_name}") % { source_name: @invite_details[:title] }, @invite_details[:url], class: "btn gl-button btn-confirm"
 
-- if !current_user_matches_invite?
+  - else
     %p
-    - mail_to_invite_email = mail_to(@member.invite_email)
-    - mail_to_current_user = mail_to(current_user.email)
-    - link_to_current_user = link_to(current_user.to_reference, user_url(current_user))
-    = _("Note that this invitation was sent to %{mail_to_invite_email}, but you are signed in as %{link_to_current_user} with email %{mail_to_current_user}.").html_safe % { mail_to_invite_email: mail_to_invite_email, mail_to_current_user: mail_to_current_user, link_to_current_user: link_to_current_user }
+      - inviter = @member.created_by
+      - link_to_inviter = link_to(inviter.name, user_url(inviter))
+      - link_to_source = link_to(@invite_details[:name], @invite_details[:url])
+
+      = html_escape(_("You have been invited by %{link_to_inviter} to join %{source_name} %{strong_open}%{link_to_source}%{strong_close} as %{role}")) % { link_to_inviter: link_to_inviter, source_name: @invite_details[:title], strong_open: '<strong>'.html_safe, link_to_source: link_to_source, strong_close: '</strong>'.html_safe, role: @member.human_access }
 
-- if !member?
     .actions
       = link_to _("Accept invitation"), accept_invite_url(@token), method: :post, class: "btn gl-button btn-confirm"
       = link_to _("Decline"), decline_invite_url(@token), method: :post, class: "btn gl-button btn-danger gl-ml-3"
+
+- else
+  %p
+    - mail_to_invite_email = mail_to(@member.invite_email)
+    - mail_to_current_user = mail_to(current_user.email)
+    - link_to_current_user = link_to(current_user.to_reference, user_url(current_user))
+    = _("This invitation was sent to %{mail_to_invite_email}, but you are signed in as %{link_to_current_user} with email %{mail_to_current_user}.").html_safe % { mail_to_invite_email: mail_to_invite_email, mail_to_current_user: mail_to_current_user, link_to_current_user: link_to_current_user }
+  %p
+    = _("Sign in as a user with the matching email address, add the email to this account, or sign-up for a new account using the matching email.")

locale/gitlab.pot

@@ -15311,6 +15311,9 @@ msgstr ""
 msgid "Go full screen"
 msgstr ""
 
+msgid "Go to %{source_name}"
+msgstr ""
+
 msgid "Go to commits"
 msgstr ""
 
@@ -16443,9 +16446,6 @@ msgstr ""
 msgid "How many users will be evaluating the trial?"
 msgstr ""
 
-msgid "However, you are already a member of this %{member_source}. Sign in using a different account to accept the invitation."
-msgstr ""
-
 msgid "I accept the %{terms_link}"
 msgstr ""
 
@@ -22616,9 +22616,6 @@ msgstr ""
 msgid "Note that pushing to GitLab requires write access to this repository."
 msgstr ""
 
-msgid "Note that this invitation was sent to %{mail_to_invite_email}, but you are signed in as %{link_to_current_user} with email %{mail_to_current_user}."
-msgstr ""
-
 msgid "Note: As an administrator you may like to configure %{github_integration_link}, which will allow login via GitHub and allow connecting repositories without generating a Personal Access Token."
 msgstr ""
 
@@ -30541,6 +30538,9 @@ msgstr ""
 msgid "Sign in / Register"
 msgstr ""
 
+msgid "Sign in as a user with the matching email address, add the email to this account, or sign-up for a new account using the matching email."
+msgstr ""
+
 msgid "Sign in preview"
 msgstr ""
 
@@ -33808,6 +33808,9 @@ msgstr ""
 msgid "This group, its subgroups and projects will be removed on %{date} since its parent group '%{parent_group_name}'' has been scheduled for removal."
 msgstr ""
 
+msgid "This invitation was sent to %{mail_to_invite_email}, but you are signed in as %{link_to_current_user} with email %{mail_to_current_user}."
+msgstr ""
+
 msgid "This is a \"Ghost User\", created to hold all issues authored by users that have since been deleted. This user cannot be removed."
 msgstr ""
 
@@ -37570,6 +37573,9 @@ msgstr ""
 msgid "You are about to transfer the control of your account to %{group_name} group. This action is NOT reversible, you won't be able to access any of your groups and projects outside of %{group_name} once this transfer is complete."
 msgstr ""
 
+msgid "You are already a member of this %{member_source}."
+msgstr ""
+
 msgid "You are an admin, which means granting access to %{client_name} will allow them to interact with GitLab as an admin as well. Proceed with caution."
 msgstr ""
 
@@ -37894,7 +37900,7 @@ msgstr ""
 msgid "You have been granted %{member_human_access} access to project %{name}."
 msgstr ""
 
-msgid "You have been invited"
+msgid "You have been invited by %{link_to_inviter} to join %{source_name} %{strong_open}%{link_to_source}%{strong_close} as %{role}"
 msgstr ""
 
 msgid "You have been redirected to the only result; see the %{a_start}search results%{a_end} instead."
@@ -38490,9 +38496,6 @@ msgstr ""
 msgid "archived:"
 msgstr ""
 
-msgid "as %{role}."
-msgstr ""
-
 msgid "assign yourself"
 msgstr ""
 
@@ -39974,9 +39977,6 @@ msgstr ""
 msgid "time summary"
 msgstr ""
 
-msgid "to join %{source_name}"
-msgstr ""
-
 msgid "toggle collapse"
 msgstr ""
 

spec/controllers/invites_controller_spec.rb

@@ -25,9 +25,64 @@ RSpec.describe InvitesController do
     end
   end
 
+  shared_examples 'invite email match enforcement' do |error_status:, flash_alert: nil|
+    it 'accepts user if invite email matches signed in user' do
+      expect do
+        request
+      end.to change { project_members.include?(user) }.from(false).to(true)
+
+      expect(response).to have_gitlab_http_status(:found)
+      expect(flash[:notice]).to include 'You have been granted'
+    end
+
+    it 'accepts invite if invite email matches confirmed secondary email' do
+      secondary_email = create(:email, :confirmed, user: user)
+      member.update!(invite_email: secondary_email.email)
+
+      expect do
+        request
+      end.to change { project_members.include?(user) }.from(false).to(true)
+
+      expect(response).to have_gitlab_http_status(:found)
+      expect(flash[:notice]).to include 'You have been granted'
+    end
+
+    it 'does not accept if invite email matches unconfirmed secondary email' do
+      secondary_email = create(:email, user: user)
+      member.update!(invite_email: secondary_email.email)
+
+      expect do
+        request
+      end.not_to change { project_members.include?(user) }
+
+      expect(response).to have_gitlab_http_status(error_status)
+      expect(flash[:alert]).to eq(flash_alert)
+    end
+
+    it 'does not accept if invite email does not match signed in user' do
+      member.update!(invite_email: 'bogus@email.com')
+
+      expect do
+        request
+      end.not_to change { project_members.include?(user) }
+
+      expect(response).to have_gitlab_http_status(error_status)
+      expect(flash[:alert]).to eq(flash_alert)
+    end
+  end
+
   describe 'GET #show', :snowplow do
     subject(:request) { get :show, params: params }
 
+    context 'when logged in' do
+      before do
+        sign_in(user)
+      end
+
+      it_behaves_like 'invite email match enforcement', error_status: :ok
+      it_behaves_like 'invalid token'
+    end
+
     context 'when it is an initial invite email' do
       let(:extra_params) { { invite_type: 'initial_email' } }
 
@@ -69,34 +124,6 @@ RSpec.describe InvitesController do
       end
     end
 
-    context 'when logged in' do
-      before do
-        sign_in(user)
-      end
-
-      it 'accepts user if invite email matches signed in user' do
-        expect do
-          request
-        end.to change { project_members.include?(user) }.from(false).to(true)
-
-        expect(response).to have_gitlab_http_status(:found)
-        expect(flash[:notice]).to include 'You have been granted'
-      end
-
-      it 'forces re-confirmation if email does not match signed in user' do
-        member.update!(invite_email: 'bogus@email.com')
-
-        expect do
-          request
-        end.not_to change { project_members.include?(user) }
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(flash[:notice]).to be_nil
-      end
-
-      it_behaves_like 'invalid token'
-    end
-
     context 'when not logged in' do
       context 'when invite token belongs to a valid member' do
         context 'when instance allows sign up' do
@@ -223,6 +250,7 @@ RSpec.describe InvitesController do
 
     subject(:request) { post :accept, params: params }
 
+    it_behaves_like 'invite email match enforcement', error_status: :redirect, flash_alert: 'The invitation could not be accepted.'
     it_behaves_like 'invalid token'
   end
 

spec/features/invites_spec.rb

@@ -90,48 +90,17 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
         end
 
         context 'when signed in and an invite link is clicked' do
-          context 'when an invite email is a secondary email for the user' do
-            let(:invite_email) { 'user_secondary@example.com' }
-
-            before do
-              sign_in(user)
-              visit invite_path(group_invite.raw_invite_token)
-            end
-
-            it 'sends user to the invite url and allows them to decline' do
-              expect(current_path).to eq(invite_path(group_invite.raw_invite_token))
-              expect(page).to have_content("Note that this invitation was sent to #{invite_email}")
-              expect(page).to have_content("but you are signed in as #{user.to_reference} with email #{user.email}")
-
-              click_link('Decline')
-
-              expect(page).to have_content('You have declined the invitation')
-              expect(current_path).to eq(dashboard_projects_path)
-              expect { group_invite.reload }.to raise_error ActiveRecord::RecordNotFound
-            end
-
-            it 'sends uer to the invite url and allows them to accept' do
-              expect(current_path).to eq(invite_path(group_invite.raw_invite_token))
-              expect(page).to have_content("Note that this invitation was sent to #{invite_email}")
-              expect(page).to have_content("but you are signed in as #{user.to_reference} with email #{user.email}")
-
-              click_link('Accept invitation')
-
-              expect(page).to have_content('You have been granted')
-              expect(current_path).to eq(activity_group_path(group))
-            end
-          end
-
           context 'when user is an existing member' do
             before do
-              sign_in(owner)
+              group.add_developer(user)
+              sign_in(user)
               visit invite_path(group_invite.raw_invite_token)
             end
 
             it 'shows message user already a member' do
               expect(current_path).to eq(invite_path(group_invite.raw_invite_token))
-              expect(page).to have_link(owner.name, href: user_url(owner))
-              expect(page).to have_content('However, you are already a member of this group.')
+              expect(page).to have_link(user.name, href: user_path(user))
+              expect(page).to have_content('You are already a member of this group.')
             end
           end
         end