Merge branch 'iss-32584-preserve-line-number-fragment-after-redirect' into 'master'

Preserve URL fragment across sign-in and sign-up redirects

Closes #32584

See merge request gitlab-org/gitlab-ce!19165

13 files changed, 300 insertions, 28 deletions

diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 9850f7ce782..4ba84589705 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -42,22 +42,35 @@ export function mergeUrlParams(params, url) {
   return `${urlparts[1]}?${query}${urlparts[3]}`;
 }
 
-export function removeParamQueryString(url, param) {
-  const decodedUrl = decodeURIComponent(url);
-  const urlVariables = decodedUrl.split('&');
-
-  return urlVariables.filter(variable => variable.indexOf(param) === -1).join('&');
-}
-
-export function removeParams(params, source = window.location.href) {
-  const url = document.createElement('a');
-  url.href = source;
+/**
+ * Removes specified query params from the url by returning a new url string that no longer
+ * includes the param/value pair. If no url is provided, `window.location.href` is used as
+ * the default value.
+ *
+ * @param {string[]} params - the query param names to remove
+ * @param {string} [url=windowLocation().href] - url from which the query param will be removed
+ * @returns {string} A copy of the original url but without the query param
+ */
+export function removeParams(params, url = window.location.href) {
+  const [rootAndQuery, fragment] = url.split('#');
+  const [root, query] = rootAndQuery.split('?');
+
+  if (!query) {
+    return url;
+  }
 
-  params.forEach(param => {
-    url.search = removeParamQueryString(url.search, param);
-  });
+  const encodedParams = params.map(param => encodeURIComponent(param));
+  const updatedQuery = query
+    .split('&')
+    .filter(paramPair => {
+      const [foundParam] = paramPair.split('=');
+      return encodedParams.indexOf(foundParam) < 0;
+    })
+    .join('&');
 
-  return url.href;
+  const writableQuery = updatedQuery.length > 0 ? `?${updatedQuery}` : '';
+  const writableFragment = fragment ? `#${fragment}` : '';
+  return `${root}${writableQuery}${writableFragment}`;
 }
 
 export function getLocationHash(url = window.location.href) {
@@ -66,6 +79,20 @@ export function getLocationHash(url = window.location.href) {
   return hashIndex === -1 ? null : url.substring(hashIndex + 1);
 }
 
+/**
+ * Apply the fragment to the given url by returning a new url string that includes
+ * the fragment. If the given url already contains a fragment, the original fragment
+ * will be removed.
+ *
+ * @param {string} url - url to which the fragment will be applied
+ * @param {string} fragment - fragment to append
+ */
+export const setUrlFragment = (url, fragment) => {
+  const [rootUrl] = url.split('#');
+  const encodedFragment = encodeURIComponent(fragment.replace(/^#/, ''));
+  return `${rootUrl}#${encodedFragment}`;
+};
+
 export function visitUrl(url, external = false) {
   if (external) {
     // Simulate `target="blank" rel="noopener noreferrer"`
diff --git a/app/assets/javascripts/pages/sessions/new/index.js b/app/assets/javascripts/pages/sessions/new/index.js
index 07f32210d93..d54bff88f70 100644
--- a/app/assets/javascripts/pages/sessions/new/index.js
+++ b/app/assets/javascripts/pages/sessions/new/index.js
@@ -2,6 +2,7 @@ import $ from 'jquery';
 import UsernameValidator from './username_validator';
 import SigninTabsMemoizer from './signin_tabs_memoizer';
 import OAuthRememberMe from './oauth_remember_me';
+import preserveUrlFragment from './preserve_url_fragment';
 
 document.addEventListener('DOMContentLoaded', () => {
   new UsernameValidator(); // eslint-disable-line no-new
@@ -10,4 +11,8 @@ document.addEventListener('DOMContentLoaded', () => {
   new OAuthRememberMe({
     container: $('.omniauth-container'),
   }).bindEvents();
+
+  // Save the URL fragment from the current window location. This will be present if the user was
+  // redirected to sign-in after attempting to access a protected URL that included a fragment.
+  preserveUrlFragment(window.location.hash);
 });
diff --git a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
index 761618109a4..191221a48cd 100644
--- a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
+++ b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
@@ -1,4 +1,5 @@
 import $ from 'jquery';
+import { mergeUrlParams, removeParams } from '~/lib/utils/url_utility';
 
 /**
  * OAuth-based login buttons have a separate "remember me" checkbox.
@@ -24,9 +25,9 @@ export default class OAuthRememberMe {
       const href = $(element).attr('href');
 
       if (rememberMe) {
-        $(element).attr('href', `${href}?remember_me=1`);
+        $(element).attr('href', mergeUrlParams({ remember_me: 1 }, href));
       } else {
-        $(element).attr('href', href.replace('?remember_me=1', ''));
+        $(element).attr('href', removeParams(['remember_me'], href));
       }
     });
   }
diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
new file mode 100644
index 00000000000..e617fecaa0f
--- /dev/null
+++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
@@ -0,0 +1,32 @@
+import { mergeUrlParams, setUrlFragment } from '~/lib/utils/url_utility';
+
+/**
+ * Ensure the given URL fragment is preserved by appending it to sign-in/sign-up form actions and
+ * OAuth/SAML login links.
+ *
+ * @param fragment {string} - url fragment to be preserved
+ */
+export default function preserveUrlFragment(fragment = '') {
+  if (fragment) {
+    const normalFragment = fragment.replace(/^#/, '');
+
+    // Append the fragment to all sign-in/sign-up form actions so it is preserved when the user is
+    // eventually redirected back to the originally requested URL.
+    const forms = document.querySelectorAll('#signin-container form');
+    Array.prototype.forEach.call(forms, form => {
+      const actionWithFragment = setUrlFragment(form.getAttribute('action'), `#${normalFragment}`);
+      form.setAttribute('action', actionWithFragment);
+    });
+
+    // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment
+    // query param will be available in the omniauth callback upon successful authentication
+    const anchors = document.querySelectorAll('#signin-container a.oauth-login');
+    Array.prototype.forEach.call(anchors, anchor => {
+      const newHref = mergeUrlParams(
+        { redirect_fragment: normalFragment },
+        anchor.getAttribute('href'),
+      );
+      anchor.setAttribute('href', newHref);
+    });
+  }
+}
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index 30be50d4595..f8e482937d5 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -75,6 +75,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   private
 
   def omniauth_flow(auth_module, identity_linker: nil)
+    if fragment = request.env.dig('omniauth.params', 'redirect_fragment').presence
+      store_redirect_fragment(fragment)
+    end
+
     if current_user
       log_audit_event(current_user, with: oauth['provider'])
 
@@ -189,4 +193,13 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     request_params = request.env['omniauth.params']
     (request_params['remember_me'] == '1') if request_params.present?
   end
+
+  def store_redirect_fragment(redirect_fragment)
+    key = stored_location_key_for(:user)
+    location = session[key]
+    if uri = parse_uri(location)
+      uri.fragment = redirect_fragment
+      store_location_for(:user, uri.to_s)
+    end
+  end
 end
diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml
index 34d4293bd45..30ed7ed6b29 100644
--- a/app/views/devise/sessions/new.html.haml
+++ b/app/views/devise/sessions/new.html.haml
@@ -1,6 +1,6 @@
 - page_title "Sign in"
 
-%div
+#signin-container
   - if form_based_providers.any?
     = render 'devise/shared/tabs_ldap'
   - else
diff --git a/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml b/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml
new file mode 100644
index 00000000000..8025cd472bd
--- /dev/null
+++ b/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml
@@ -0,0 +1,6 @@
+---
+title: Fix lost line number when navigating to a specific line in a protected file
+  before authenticating.
+merge_request: 19165
+author: Scott Escue
+type: fixed
diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb
index d377d69457f..21936491ffc 100644
--- a/spec/controllers/omniauth_callbacks_controller_spec.rb
+++ b/spec/controllers/omniauth_callbacks_controller_spec.rb
@@ -45,6 +45,40 @@ describe OmniauthCallbacksController, type: :controller do
       end
     end
 
+    context 'when a redirect fragment is provided' do
+      let(:provider) { :jwt }
+      let(:extern_uid) { 'my-uid' }
+
+      before do
+        request.env['omniauth.params'] = { 'redirect_fragment' => 'L101' }
+      end
+
+      context 'when a redirect url is stored' do
+        it 'redirects with fragment' do
+          post provider, nil, { user_return_to: '/fake/url' }
+
+          expect(response).to redirect_to('/fake/url#L101')
+        end
+      end
+
+      context 'when a redirect url with a fragment is stored' do
+        it 'redirects with the new fragment' do
+          post provider, nil, { user_return_to: '/fake/url#replaceme' }
+
+          expect(response).to redirect_to('/fake/url#L101')
+        end
+      end
+
+      context 'when no redirect url is stored' do
+        it 'does not redirect with the fragment' do
+          post provider
+
+          expect(response.redirect?).to be true
+          expect(response.location).not_to include('#L101')
+        end
+      end
+    end
+
     context 'strategies' do
       context 'github' do
         let(:extern_uid) { 'my-uid' }
diff --git a/spec/javascripts/fixtures/oauth_remember_me.html.haml b/spec/javascripts/fixtures/oauth_remember_me.html.haml
index 7886e995e57..a5d7c4e816a 100644
--- a/spec/javascripts/fixtures/oauth_remember_me.html.haml
+++ b/spec/javascripts/fixtures/oauth_remember_me.html.haml
@@ -3,3 +3,4 @@
 
   %a.oauth-login.twitter{ href: "http://example.com/" }
   %a.oauth-login.github{ href: "http://example.com/" }
+  %a.oauth-login.facebook{ href: "http://example.com/?redirect_fragment=L1" }
diff --git a/spec/javascripts/fixtures/sessions.rb b/spec/javascripts/fixtures/sessions.rb
new file mode 100644
index 00000000000..e90a58e8c54
--- /dev/null
+++ b/spec/javascripts/fixtures/sessions.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe 'Sessions (JavaScript fixtures)' do
+  include JavaScriptFixturesHelpers
+
+  before(:all) do
+    clean_frontend_fixtures('sessions/')
+  end
+
+  describe SessionsController, '(JavaScript fixtures)', type: :controller do
+    include DeviseHelpers
+
+    render_views
+
+    before do
+      set_devise_mapping(context: @request)
+    end
+
+    it 'sessions/new.html.raw' do |example|
+      get :new
+
+      expect(response).to be_success
+      store_frontend_fixture(response, example.description)
+    end
+  end
+end
diff --git a/spec/javascripts/lib/utils/url_utility_spec.js b/spec/javascripts/lib/utils/url_utility_spec.js
index e4df8441793..381c7b2d0a6 100644
--- a/spec/javascripts/lib/utils/url_utility_spec.js
+++ b/spec/javascripts/lib/utils/url_utility_spec.js
@@ -1,4 +1,4 @@
-import { webIDEUrl, mergeUrlParams } from '~/lib/utils/url_utility';
+import * as urlUtils from '~/lib/utils/url_utility';
 
 describe('URL utility', () => {
   describe('webIDEUrl', () => {
@@ -8,7 +8,7 @@ describe('URL utility', () => {
 
     describe('without relative_url_root', () => {
       it('returns IDE path with route', () => {
-        expect(webIDEUrl('/gitlab-org/gitlab-ce/merge_requests/1')).toBe(
+        expect(urlUtils.webIDEUrl('/gitlab-org/gitlab-ce/merge_requests/1')).toBe(
           '/-/ide/project/gitlab-org/gitlab-ce/merge_requests/1',
         );
       });
@@ -20,7 +20,7 @@ describe('URL utility', () => {
       });
 
       it('returns IDE path with route', () => {
-        expect(webIDEUrl('/gitlab/gitlab-org/gitlab-ce/merge_requests/1')).toBe(
+        expect(urlUtils.webIDEUrl('/gitlab/gitlab-org/gitlab-ce/merge_requests/1')).toBe(
           '/gitlab/-/ide/project/gitlab-org/gitlab-ce/merge_requests/1',
         );
       });
@@ -29,23 +29,82 @@ describe('URL utility', () => {
 
   describe('mergeUrlParams', () => {
     it('adds w', () => {
-      expect(mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag');
-      expect(mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag');
-      expect(mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1');
-      expect(mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe('https://host/path?w=1#frag');
-      expect(mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe('https://h/p?k1=v1&w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe(
+        'https://host/path?w=1#frag',
+      );
+
+      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe(
+        'https://h/p?k1=v1&w=1#frag',
+      );
     });
 
     it('updates w', () => {
-      expect(mergeUrlParams({ w: 1 }, '?k1=v1&w=0#frag')).toBe('?k1=v1&w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, '?k1=v1&w=0#frag')).toBe('?k1=v1&w=1#frag');
     });
 
     it('adds multiple params', () => {
-      expect(mergeUrlParams({ a: 1, b: 2, c: 3 }, '#frag')).toBe('?a=1&b=2&c=3#frag');
+      expect(urlUtils.mergeUrlParams({ a: 1, b: 2, c: 3 }, '#frag')).toBe('?a=1&b=2&c=3#frag');
     });
 
     it('adds and updates encoded params', () => {
-      expect(mergeUrlParams({ a: '&', q: '?' }, '?a=%23#frag')).toBe('?a=%26&q=%3F#frag');
+      expect(urlUtils.mergeUrlParams({ a: '&', q: '?' }, '?a=%23#frag')).toBe('?a=%26&q=%3F#frag');
+    });
+  });
+
+  describe('removeParams', () => {
+    describe('when url is passed', () => {
+      it('removes query param with encoded ampersand', () => {
+        const url = urlUtils.removeParams(['filter'], '/mail?filter=n%3Djoe%26l%3Dhome');
+
+        expect(url).toBe('/mail');
+      });
+
+      it('should remove param when url has no other params', () => {
+        const url = urlUtils.removeParams(['size'], '/feature/home?size=5');
+
+        expect(url).toBe('/feature/home');
+      });
+
+      it('should remove param when url has other params', () => {
+        const url = urlUtils.removeParams(['size'], '/feature/home?q=1&size=5&f=html');
+
+        expect(url).toBe('/feature/home?q=1&f=html');
+      });
+
+      it('should remove param and preserve fragment', () => {
+        const url = urlUtils.removeParams(['size'], '/feature/home?size=5#H2');
+
+        expect(url).toBe('/feature/home#H2');
+      });
+
+      it('should remove multiple params', () => {
+        const url = urlUtils.removeParams(['z', 'a'], '/home?z=11111&l=en_US&a=true#H2');
+
+        expect(url).toBe('/home?l=en_US#H2');
+      });
+    });
+  });
+
+  describe('setUrlFragment', () => {
+    it('should set fragment when url has no fragment', () => {
+      const url = urlUtils.setUrlFragment('/home/feature', 'usage');
+
+      expect(url).toBe('/home/feature#usage');
+    });
+
+    it('should set fragment when url has existing fragment', () => {
+      const url = urlUtils.setUrlFragment('/home/feature#overview', 'usage');
+
+      expect(url).toBe('/home/feature#usage');
+    });
+
+    it('should set fragment when given fragment includes #', () => {
+      const url = urlUtils.setUrlFragment('/home/feature#overview', '#install');
+
+      expect(url).toBe('/home/feature#install');
     });
   });
 });
diff --git a/spec/javascripts/oauth_remember_me_spec.js b/spec/javascripts/oauth_remember_me_spec.js
index 2caa266b85f..4125706a407 100644
--- a/spec/javascripts/oauth_remember_me_spec.js
+++ b/spec/javascripts/oauth_remember_me_spec.js
@@ -20,6 +20,10 @@ describe('OAuthRememberMe', () => {
     expect($('#oauth-container .oauth-login.github').attr('href')).toBe(
       'http://example.com/?remember_me=1',
     );
+
+    expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe(
+      'http://example.com/?redirect_fragment=L1&remember_me=1',
+    );
   });
 
   it('removes the "remember_me" query parameter from all OAuth login buttons', () => {
@@ -28,5 +32,8 @@ describe('OAuthRememberMe', () => {
 
     expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe('http://example.com/');
     expect($('#oauth-container .oauth-login.github').attr('href')).toBe('http://example.com/');
+    expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe(
+      'http://example.com/?redirect_fragment=L1',
+    );
   });
 });
diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
new file mode 100644
index 00000000000..7a8227479d4
--- /dev/null
+++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
@@ -0,0 +1,61 @@
+import $ from 'jquery';
+import preserveUrlFragment from '~/pages/sessions/new/preserve_url_fragment';
+
+describe('preserve_url_fragment', () => {
+  preloadFixtures('sessions/new.html.raw');
+
+  beforeEach(() => {
+    loadFixtures('sessions/new.html.raw');
+  });
+
+  it('adds the url fragment to all login and sign up form actions', () => {
+    preserveUrlFragment('#L65');
+
+    expect($('#new_user').attr('action')).toBe('http://test.host/users/sign_in#L65');
+    expect($('#new_new_user').attr('action')).toBe('http://test.host/users#L65');
+  });
+
+  it('does not add an empty url fragment to login and sign up form actions', () => {
+    preserveUrlFragment();
+
+    expect($('#new_user').attr('action')).toBe('http://test.host/users/sign_in');
+    expect($('#new_new_user').attr('action')).toBe('http://test.host/users');
+  });
+
+  it('does not add an empty query parameter to OmniAuth login buttons', () => {
+    preserveUrlFragment();
+
+    expect($('#oauth-login-cas3').attr('href')).toBe('http://test.host/users/auth/cas3');
+
+    expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
+      'http://test.host/users/auth/auth0',
+    );
+  });
+
+  describe('adds "redirect_fragment" query parameter to OmniAuth login buttons', () => {
+    it('when "remember_me" is not present', () => {
+      preserveUrlFragment('#L65');
+
+      expect($('#oauth-login-cas3').attr('href')).toBe(
+        'http://test.host/users/auth/cas3?redirect_fragment=L65',
+      );
+
+      expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
+        'http://test.host/users/auth/auth0?redirect_fragment=L65',
+      );
+    });
+
+    it('when "remember-me" is present', () => {
+      $('a.omniauth-btn').attr('href', (i, href) => `${href}?remember_me=1`);
+      preserveUrlFragment('#L65');
+
+      expect($('#oauth-login-cas3').attr('href')).toBe(
+        'http://test.host/users/auth/cas3?remember_me=1&redirect_fragment=L65',
+      );
+
+      expect($('#oauth-login-auth0').attr('href')).toBe(
+        'http://test.host/users/auth/auth0?remember_me=1&redirect_fragment=L65',
+      );
+    });
+  });
+});