Merge branch 'mk/remove-alternate-url' into 'master'

Geo: Remove Alternate URL

See merge request gitlab-org/gitlab-ee!10609

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20190403161806) do
+ActiveRecord::Schema.define(version: 20190404231137) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1362,7 +1362,6 @@ ActiveRecord::Schema.define(version: 20190403161806) do
     t.text "selective_sync_shards"
     t.integer "verification_max_capacity", default: 100, null: false
     t.integer "minimum_reverification_interval", default: 7, null: false
-    t.string "alternate_url"
     t.string "internal_url"
     t.index ["access_key"], name: "index_geo_nodes_on_access_key", using: :btree
     t.index ["primary"], name: "index_geo_nodes_on_primary", using: :btree

doc/api/geo_nodes.md

@@ -20,7 +20,6 @@ Example response:
   {
     "id": 1,
     "url": "https://primary.example.com/",
-    "alternate_url": null,
     "internal_url": "https://internal.example.com/",
     "primary": true,
     "enabled": true,
@@ -33,7 +32,6 @@ Example response:
   {
     "id": 2,
     "url": "https://secondary.example.com/",
-    "alternate_url": "https://alternate.example.com/",
     "internal_url": "https://secondary.example.com/",
     "primary": false,
     "enabled": true,
@@ -62,7 +60,6 @@ Example response:
 {
   "id": 1,
   "url": "https://primary.example.com/",
-  "alternate_url": null,
   "internal_url": "https://primary.example.com/",
   "primary": true,
   "enabled": true,
@@ -89,7 +86,6 @@ PUT /geo_nodes/:id
 | `id`                 | integer | yes       | The ID of the Geo node.                                                   |
 | `enabled`            | boolean | no        | Flag indicating if the Geo node is enabled.                               |
 | `url`                | string  | no        | The URL to connect to the Geo node.                                       |
-| `alternate_url`      | string  | no        | Allows users to log in to the secondary node at an alternate URL (required for OAuth). |
 | `internal_url`       | string  | no        | The URL defined on the primary node that secondary nodes should use to contact it. Returns `url` if not set.|
 | `files_max_capacity` | integer | no        | Control the maximum concurrency of LFS/attachment backfill for this secondary node. |
 | `repos_max_capacity` | integer | no        | Control the maximum concurrency of repository backfill for this secondary node.     |
@@ -101,7 +97,6 @@ Example response:
 {
   "id": 1,
   "url": "https://secondary.example.com/",
-  "alternate_url": "https://alternate.example.com/",
   "internal_url": "https://secondary.example.com/",
   "primary": false,
   "enabled": true,
@@ -144,7 +139,6 @@ Example response:
 {
   "id": 1,
   "url": "https://primary.example.com/",
-  "alternate_url": null,
   "internal_url": "https://primary.example.com/",
   "primary": true,
   "enabled": true,

doc/user/admin_area/geo_nodes.md

@@ -27,7 +27,6 @@ Secondaries have a number of additional settings available:
 | Selective synchronization | Enable Geo [selective sync](../../administration/geo/replication/configuration.md#selective-synchronization) for this secondary. |
 | Repository sync capacity  | Number of concurrent requests this secondary will make to the primary when backfilling repositories. |
 | File sync capacity        | Number of concurrent requests this secondary will make to the primary when backfilling files. |
-| Alternate URL             | Allows users to log in to the secondary at an alternate URL (required for OAuth) |
 
 ## Geo backfill
 
@@ -47,16 +46,6 @@ you can increase the values to complete backfill in a shorter time. If it's
 under heavy load and backfill is reducing its availability for normal requests,
 you can decrease them.
 
-## Multiple secondaries behind a load balancer
-
-Secondaries are authenticated via OAuth with the primary. For security, the
-primary does not allow redirecting back to an arbitrary URL. If you want to
-allow users to log in to secondaries at a common name/load balancer URL, then
-this URL must be specified as the "Alternate URL" on every secondary behind it.
-
-Additionally, the load balancer should use sticky sessions, since users must
-authenticate each first request to each secondary.
-
 ## Using a different URL for synchronization
 
 The **primary** node's Internal URL is used by **secondary** nodes to contact it

ee/app/assets/javascripts/geo_nodes/components/node_detail_sections/node_details_section_other.vue

@@ -78,12 +78,6 @@ export default {
           itemValueType: VALUE_TYPE.PLAIN,
           cssClass: this.storageShardsCssClass,
         },
-        {
-          itemTitle: s__('GeoNodes|Alternate URL'),
-          itemValue: this.node.alternateUrl,
-          itemValueType: VALUE_TYPE.PLAIN,
-          cssClass: 'node-detail-value-bold',
-        },
       ];
     },
     storageShardsStatus() {

ee/app/assets/javascripts/geo_nodes/store/geo_nodes_store.js

@@ -48,7 +48,6 @@ export default class GeoNodesStore {
       primary,
       current,
       enabled,
-      alternateUrl: rawNode.alternate_url || '',
       internalUrl: rawNode.internal_url || '',
       nodeActionActive: false,
       basePath: rawNode._links.self,

ee/app/assets/javascripts/pages/admin/geo/nodes/shared/init_form.js

@@ -5,11 +5,9 @@ import Api from '~/api';
 
 const onPrimaryCheckboxChange = function onPrimaryCheckboxChange(e, $namespaces, $reverification) {
   const $namespacesSelect = $('.select2', $namespaces);
-  const $alternateUrl = $('.js-alternate-url');
   const $internalUrl = $('.js-internal-url');
 
   $namespacesSelect.select2('data', null);
-  $alternateUrl.toggleClass('hidden', e.currentTarget.checked);
   $internalUrl.toggleClass('hidden', !e.currentTarget.checked);
   $namespaces.toggleClass('hidden', e.currentTarget.checked);
   $reverification.toggleClass('hidden', !e.currentTarget.checked);

ee/app/controllers/admin/geo/nodes_controller.rb

@@ -50,7 +50,6 @@ class Admin::Geo::NodesController < Admin::ApplicationController
   def geo_node_params
     params.require(:geo_node).permit(
       :url,
-      :alternate_url,
       :internal_url,
       :primary,
       :selective_sync_type,

ee/app/models/geo_node.rb

@@ -19,7 +19,6 @@ class GeoNode < ApplicationRecord
                  primary: false
 
   validates :url, presence: true, uniqueness: { case_sensitive: false }, url: true
-  validates :alternate_url, url: true, allow_blank: true, allow_nil: true
   validates :internal_url, url: true, allow_blank: true, allow_nil: true
 
   validates :primary, uniqueness: { message: 'node already exists' }, if: :primary
@@ -132,16 +131,6 @@ class GeoNode < ApplicationRecord
     @uri = nil
   end
 
-  def alternate_url
-    read_with_ending_slash(:alternate_url)
-  end
-
-  def alternate_url=(value)
-    write_with_ending_slash(:alternate_url, value)
-
-    @alternate_uri = nil
-  end
-
   def internal_url
     read_with_ending_slash(:internal_url).presence || read_with_ending_slash(:url)
   end
@@ -156,10 +145,6 @@ class GeoNode < ApplicationRecord
     @uri ||= URI.parse(url) if url.present?
   end
 
-  def alternate_uri
-    @alternate_uri ||= URI.parse(alternate_url) if alternate_url.present?
-  end
-
   def internal_uri
     @internal_uri ||= URI.parse(internal_url) if internal_url.present?
   end
@@ -183,12 +168,6 @@ class GeoNode < ApplicationRecord
     Gitlab::Routing.url_helpers.oauth_geo_callback_url(url_helper_args)
   end
 
-  def alternate_oauth_callback_url
-    return unless alternate_url.present?
-
-    Gitlab::Routing.url_helpers.oauth_geo_callback_url(alternate_url_helper_args)
-  end
-
   def oauth_logout_url(state)
     Gitlab::Routing.url_helpers.oauth_geo_logout_url(url_helper_args.merge(state: state))
   end
@@ -279,10 +258,6 @@ class GeoNode < ApplicationRecord
     url_helper_options(uri)
   end
 
-  def alternate_url_helper_args
-    url_helper_options(alternate_uri)
-  end
-
   def url_helper_options(given_uri)
     { protocol: given_uri.scheme, host: given_uri.host, port: given_uri.port, script_name: given_uri.path }
   end
@@ -310,7 +285,7 @@ class GeoNode < ApplicationRecord
   def update_oauth_application!
     self.build_oauth_application if oauth_application.nil?
     self.oauth_application.name = "Geo node: #{self.url}"
-    self.oauth_application.redirect_uri = [oauth_callback_url, alternate_oauth_callback_url].compact.join("\n")
+    self.oauth_application.redirect_uri = oauth_callback_url
   end
 
   def expire_cache!

ee/app/views/admin/geo/nodes/_form.html.haml

@@ -4,11 +4,6 @@
     = form.label :url, s_('Geo|URL'), class: 'font-weight-bold'
     = form.text_field :url, class: 'form-control'
 
-  .form-group.col-sm-6.js-alternate-url{ class: ('hidden' unless geo_node.secondary?) }
-    = form.label :alternate_url, s_('Geo|Alternate URL'), class: 'font-weight-bold'
-    = form.text_field :alternate_url, class: 'form-control'
-    .form-text.text-muted= s_('Geo|To support OAuth logins to this node at a different domain than URL')
-
   .form-group.col-sm-6.js-internal-url{ class: ('hidden' unless geo_node.primary?) }
     = form.label :internal_url, s_('Geo|Internal URL'), class: 'font-weight-bold'
     = form.text_field :internal_url, class: 'form-control'

ee/changelogs/unreleased/mk-alternate-geo-node-oauth-urls.yml

----
-title: 'Geo: Allow OAuth login to a secondary node at an alternate URL'
-merge_request: 9544
-author:
-type: added

ee/changelogs/unreleased/mk-geo-node-autosave-oauth-application.yml

----
-title: 'Geo: Persist OAuth callback changes for Alternate URL'
-merge_request: 10358
-author:
-type: fixed

ee/db/migrate/20190212230641_add_alternate_url_to_geo_nodes.rb

-# frozen_string_literal: true
-
-class AddAlternateUrlToGeoNodes < ActiveRecord::Migration[5.0]
-  def change
-    add_column :geo_nodes, :alternate_url, :string
-  end
-end

ee/db/post_migrate/20190404231137_remove_alternate_url_from_geo_nodes.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+# We are reverting the feature that created this column. This is for anyone who
+# migrated while the feature still existed in master.
+class RemoveAlternateUrlFromGeoNodes < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def up
+    remove_column(:geo_nodes, :alternate_url) if column_exists?(:geo_nodes, :alternate_url)
+  end
+
+  def down
+    add_column :geo_nodes, :alternate_url, :string
+  end
+end

ee/lib/api/geo_nodes.rb

@@ -144,7 +144,6 @@ module API
         params do
           optional :enabled, type: Boolean, desc: 'Flag indicating if the Geo node is enabled'
           optional :url, type: String, desc: 'The URL to connect to the Geo node'
-          optional :alternate_url, type: String, desc: 'The alternate URL to allow OAuth logins to this secondary node'
           optional :internal_url, type: String, desc: 'The URL defined on the primary node that secondary nodes should use to contact it. Defaults to url'
           optional :files_max_capacity, type: Integer, desc: 'Control the maximum concurrency of LFS/attachment backfill for this secondary node'
           optional :repos_max_capacity, type: Integer, desc: 'Control the maximum concurrency of repository backfill for this secondary node'

ee/lib/ee/api/entities.rb

@@ -462,7 +462,6 @@ module EE
 
         expose :id
         expose :url
-        expose :alternate_url
         expose :internal_url
         expose :primary?, as: :primary
         expose :enabled

ee/spec/controllers/admin/geo/nodes_controller_spec.rb

@@ -113,7 +113,6 @@ describe Admin::Geo::NodesController, :postgresql do
     let(:geo_node_attributes) do
       {
         url: 'http://example.com',
-        alternate_url: 'http://anotherexample.com',
         internal_url: 'http://internal-url.com',
         selective_sync_shards: %w[foo bar]
       }
@@ -144,7 +143,6 @@ describe Admin::Geo::NodesController, :postgresql do
 
         geo_node.reload
         expect(geo_node.url.chomp('/')).to eq(geo_node_attributes[:url])
-        expect(geo_node.alternate_url.chomp('/')).to eq(geo_node_attributes[:alternate_url])
         expect(geo_node.internal_url.chomp('/')).to eq(geo_node_attributes[:internal_url])
         expect(geo_node.selective_sync_shards).to eq(%w[foo bar])
       end

ee/spec/features/admin/geo/admin_geo_nodes_spec.rb

@@ -143,27 +143,6 @@ describe 'admin Geo Nodes', :js do
         end
       end
     end
-
-    it "allows the admin to update a secondary node's alternate URL" do
-      visit admin_geo_nodes_path
-      wait_for_requests
-
-      page.within(find('.geo-node-actions', match: :first)) do
-        page.click_link('Edit')
-      end
-
-      fill_in 'Alternate URL', with: 'http://someloadbalancer.com'
-      click_button 'Save changes'
-
-      expect(current_path).to eq admin_geo_nodes_path
-      wait_for_requests
-
-      page.within(find('.geo-node-item', match: :first)) do
-        click_button 'Other information'
-
-        expect(page).to have_content('http://someloadbalancer.com')
-      end
-    end
   end
 
   describe 'remove an existing Geo Node' do

ee/spec/fixtures/api/schemas/public_api/v4/geo_node.json

@@ -3,7 +3,6 @@
   "required" : [
     "id",
     "url",
-    "alternate_url",
     "primary",
     "enabled",
     "current",
@@ -16,7 +15,6 @@
   "properties" : {
     "id": { "type": "integer" },
     "url": { "type": ["string", "null"] },
-    "alternate_url": { "type": ["string", "null"] },
     "primary": { "type": "boolean" },
     "internal_url": { "type": ["string", "null"] },
     "enabled": { "type": "boolean" },

ee/spec/javascripts/geo_nodes/components/node_detail_sections/node_details_section_other_spec.js

@@ -57,7 +57,7 @@ describe('NodeDetailsSectionOther', () => {
       it('returns array containing items to show under secondary node when prop `nodeTypePrimary` is false', () => {
         const items = vm.nodeDetailItems;
 
-        expect(items.length).toBe(2);
+        expect(items.length).toBe(1);
         expect(items[0].itemTitle).toBe('Storage config');
       });
     });

ee/spec/javascripts/geo_nodes/mock_data.js

@@ -25,7 +25,6 @@ export const mockNodes = [
   {
     id: 2,
     url: 'http://127.0.0.1:3002/',
-    alternate_url: 'http://example.com:3002/',
     primary: false,
     enabled: true,
     current: false,
@@ -44,7 +43,6 @@ export const mockNodes = [
 export const mockNode = {
   id: 1,
   url: 'http://127.0.0.1:3001/',
-  alternateUrl: '',
   internalUrl: 'http://127.0.0.1:3001/',
   primary: true,
   current: true,

ee/spec/models/geo_node_spec.rb

@@ -60,28 +60,6 @@ describe GeoNode, type: :model do
       end
     end
 
-    context 'when validating alternate_url' do
-      subject { build(:geo_node, alternate_url: alternate_url) }
-
-      context 'when alternate_url is http' do
-        let(:alternate_url) { 'http://foo' }
-
-        it { is_expected.to be_valid }
-      end
-
-      context 'when alternate_url is https' do
-        let(:alternate_url) { 'https://foo' }
-
-        it { is_expected.to be_valid }
-      end
-
-      context 'when alternate_url is not http or https' do
-        let(:alternate_url) { 'nothttp://foo' }
-
-        it { is_expected.not_to be_valid }
-      end
-    end
-
     context 'when validating internal_url' do
       subject { build(:geo_node, internal_url: internal_url) }
 
@@ -157,17 +135,6 @@ describe GeoNode, type: :model do
 
           expect(node.oauth_application.redirect_uri).to eq(node.oauth_callback_url)
         end
-
-        context 'when the node has an alternate_url' do
-          it 'adds an alternate callback URL' do
-            node.alternate_url = 'http://alternate-url.com:1234/gitlab/'
-
-            expect(node).to be_valid
-
-            expected = [node.oauth_callback_url, node.alternate_oauth_callback_url].join("\n")
-            expect(node.oauth_application.redirect_uri).to eq(expected)
-          end
-        end
       end
 
       context 'when it is a primary node' do
@@ -229,39 +196,6 @@ describe GeoNode, type: :model do
           expect(oauth_application.reload.redirect_uri).to eq('http://modified-url/oauth/geo/callback')
         end
       end
-
-      context 'when alternate_url is added' do
-        it "adds a callback URL to the associated OAuth application's redirect_uri" do
-          expected_redirect_uri = "#{oauth_application.redirect_uri}\nhttp://alternate-url/oauth/geo/callback"
-
-          node.update!(alternate_url: 'http://alternate-url')
-
-          expect(oauth_application.reload.redirect_uri).to eq(expected_redirect_uri)
-        end
-      end
-
-      context 'when alternate_url is modified' do
-        it "updates the alternate callback URL in the associated OAuth application's redirect_uri" do
-          node.update!(alternate_url: 'http://alternate-url')
-          oauth_application.update!(redirect_uri: "#{node.oauth_callback_url}\nhttp://alternate-url/oauth/geo/callback")
-          expected_redirect_uri = "#{node.oauth_callback_url}\nhttp://modified-alternate-url/oauth/geo/callback"
-
-          node.update!(alternate_url: 'http://modified-alternate-url')
-
-          expect(oauth_application.reload.redirect_uri).to eq(expected_redirect_uri)
-        end
-      end
-
-      context 'when alternate_url is cleared' do
-        it "removes the alternate callback URL in the associated OAuth application's redirect_uri" do
-          expected_redirect_uri = oauth_application.redirect_uri
-          oauth_application.update!(redirect_uri: "#{node.oauth_callback_url}\nhttp://alternate-url/oauth/geo/callback")
-
-          node.update!(alternate_url: nil)
-
-          expect(oauth_application.reload.redirect_uri).to eq(expected_redirect_uri)
-        end
-      end
     end
   end
 
@@ -473,82 +407,6 @@ describe GeoNode, type: :model do
     end
   end
 
-  describe '#alternate_uri' do
-    let(:alternate_url) { 'https://foo:3003/bar' }
-    let(:node) { create(:geo_node, url: 'https://localhost:3000/gitlab', alternate_url: alternate_url) }
-
-    context 'when all fields are filled' do
-      it 'returns an URI object' do
-        expect(node.alternate_uri).to be_a URI
-      end
-
-      it 'includes schema, host, port and relative_url_root with a terminating /' do
-        expected_uri = URI.parse(alternate_url)
-        expected_uri.path += '/'
-        expect(node.alternate_uri).to eq(expected_uri)
-      end
-    end
-  end
-
-  describe '#alternate_url' do
-    let(:alternate_url) { 'https://foo:3003/bar' }
-    let(:node) { create(:geo_node, url: 'https://localhost:3000/gitlab', alternate_url: alternate_url) }
-
-    it 'returns a string' do
-      expect(node.alternate_url).to be_a String
-    end
-
-    it 'includes schema home port and relative_url with a terminating /' do
-      expected_url = "#{alternate_url}/"
-      expect(node.alternate_url).to eq(expected_url)
-    end
-
-    it 'can be nil' do
-      stub_config_setting(port: 443)
-      stub_config_setting(protocol: 'https')
-      stub_config_setting(relative_url_root: '/gitlab')
-
-      expect(empty_node.alternate_url).to be_nil
-    end
-  end
-
-  describe '#alternate_url=' do
-    subject { described_class.new(alternate_url: 'https://foo:3003/bar') }
-
-    it 'sets schema field based on url' do
-      expect(subject.alternate_uri.scheme).to eq('https')
-    end
-
-    it 'sets host field based on url' do
-      expect(subject.alternate_uri.host).to eq('foo')
-    end
-
-    it 'sets port field based on specified by url' do
-      expect(subject.alternate_uri.port).to eq(3003)
-    end
-
-    context 'when using unspecified ports' do
-      let(:dummy_http) { 'http://example.com/' }
-      let(:dummy_https) { 'https://example.com/' }
-
-      context 'when schema is http' do
-        it 'sets port 80' do
-          subject.alternate_url = dummy_http
-
-          expect(subject.alternate_uri.port).to eq(80)
-        end
-      end
-
-      context 'when schema is https' do
-        it 'sets port 443' do
-          subject.alternate_url = dummy_https
-
-          expect(subject.alternate_uri.port).to eq(443)
-        end
-      end
-    end
-  end
-
   describe '#internal_url' do
     let(:internal_url) { 'https://foo:3003/bar' }
     let(:node) { create(:geo_node, url: dummy_url, internal_url: internal_url) }
@@ -667,20 +525,6 @@ describe GeoNode, type: :model do
     end
   end
 
-  describe '#alternate_oauth_callback_url' do
-    let(:node) { create(:geo_node, url: 'https://localhost:3000/gitlab', alternate_url: 'https://alternate:4444/gitlabalternate') }
-    let(:alternate_oauth_callback_url) { 'https://alternate:4444/gitlabalternate/oauth/geo/callback' }
-
-    it 'returns oauth callback url based on node uri' do
-      expect(node.alternate_oauth_callback_url).to eq(alternate_oauth_callback_url)
-    end
-
-    it 'returns url that matches rails url_helpers generated one' do
-      route = url_helpers.oauth_geo_callback_url(protocol: 'https:', host: 'alternate', port: 4444, script_name: '/gitlabalternate')
-      expect(node.alternate_oauth_callback_url).to eq(route)
-    end
-  end
-
   describe '#oauth_logout_url' do
     let(:fake_state) { CGI.escape('fakestate') }
     let(:oauth_logout_url) { "https://localhost:3000/gitlab/oauth/geo/logout?state=#{fake_state}" }

ee/spec/requests/api/geo_nodes_spec.rb

@@ -212,7 +212,6 @@ describe API::GeoNodes, :geo, :prometheus, api: true do
       params = {
         enabled: false,
         url: 'https://updated.example.com/',
-        alternate_url: 'https://alternate.example.com/',
         internal_url: 'https://internal-com.com/',
         files_max_capacity: 33,
         repos_max_capacity: 44,

locale/gitlab.pot

@@ -4839,9 +4839,6 @@ msgstr ""
 msgid "GeoNodeSyncStatus|Node is slow, overloaded, or it just recovered after an outage."
 msgstr ""
 
-msgid "GeoNodes|Alternate URL"
-msgstr ""
-
 msgid "GeoNodes|Checksummed"
 msgstr ""
 
@@ -5025,9 +5022,6 @@ msgstr ""
 msgid "Geo|All projects are being scheduled for re-sync"
 msgstr ""
 
-msgid "Geo|Alternate URL"
-msgstr ""
-
 msgid "Geo|Batch operations"
 msgstr ""
 
@@ -5157,9 +5151,6 @@ msgstr ""
 msgid "Geo|This is a primary node"
 msgstr ""
 
-msgid "Geo|To support OAuth logins to this node at a different domain than URL"
-msgstr ""
-
 msgid "Geo|Tracking entry for project (%{project_id}) was successfully removed."
 msgstr ""
 

locale/uk/gitlab.po

@@ -4645,9 +4645,6 @@ msgstr "Вузол не працює або зламаний."
 msgid "GeoNodeSyncStatus|Node is slow, overloaded, or it just recovered after an outage."
 msgstr "Вузол працює повільно, перевантажений або тільки що відновився після збою."
 
-msgid "GeoNodes|Alternate URL"
-msgstr ""
-
 msgid "GeoNodes|Checksummed"
 msgstr "Із контрольною сумою"
 
@@ -4828,9 +4825,6 @@ msgstr "Всі проекти плануються для повторної п
 msgid "Geo|All projects are being scheduled for re-sync"
 msgstr "Всі проекти плануються для повторної синхронізації"
 
-msgid "Geo|Alternate URL"
-msgstr ""
-
 msgid "Geo|Batch operations"
 msgstr "Групові операції"
 
@@ -12607,4 +12601,3 @@ msgstr[3] "протягом %d хвилин "
 
 msgid "yaml invalid"
 msgstr ""
-