Merge branch 'jhyson/doorkeeper_upgrade' into 'master'

Upgrade Doorkeeper to 4.4.3

See merge request gitlab-org/gitlab!20953

Gemfile

@@ -26,7 +26,7 @@ gem 'marginalia', '~> 1.8.0'
 
 # Authentication libraries
 gem 'devise', '~> 4.6'
-gem 'doorkeeper', '~> 4.3'
+gem 'doorkeeper', '~> 4.4.3'
 gem 'doorkeeper-openid_connect', '~> 1.5'
 gem 'omniauth', '~> 1.8'
 gem 'omniauth-auth0', '~> 2.0.0'

Gemfile.lock

@@ -231,7 +231,7 @@ GEM
     docile (1.3.1)
     domain_name (0.5.20180417)
       unf (>= 0.0.5, < 1.0.0)
-    doorkeeper (4.3.2)
+    doorkeeper (4.4.3)
       railties (>= 4.2)
     doorkeeper-openid_connect (1.5.0)
       doorkeeper (~> 4.3)
@@ -1180,7 +1180,7 @@ DEPENDENCIES
   diff_match_patch (~> 0.1.0)
   diffy (~> 3.1.0)
   discordrb-webhooks-blackst0ne (~> 3.3)
-  doorkeeper (~> 4.3)
+  doorkeeper (~> 4.4.3)
   doorkeeper-openid_connect (~> 1.5)
   ed25519 (~> 1.2)
   elasticsearch-api (~> 6.8)

app/controllers/admin/applications_controller.rb

@@ -55,6 +55,8 @@ class Admin::ApplicationsController < Admin::ApplicationController
 
   # Only allow a trusted parameter "white list" through.
   def application_params
-    params.require(:doorkeeper_application).permit(:name, :redirect_uri, :trusted, :scopes)
+    params
+      .require(:doorkeeper_application)
+      .permit(:name, :redirect_uri, :trusted, :scopes, :confidential)
   end
 end

app/views/admin/applications/_form.html.haml

@@ -30,6 +30,14 @@
       %span.form-text.text-muted
         Trusted applications are automatically authorized on GitLab OAuth flow.
 
+  = content_tag :div, class: 'form-group row' do
+    .col-sm-2.col-form-label.pt-0
+      = f.label :confidential
+    .col-sm-10
+      = f.check_box :confidential
+      %span.form-text.text-muted
+        = _('The application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential.')
+
   .form-group.row
     .col-sm-2.col-form-label.pt-0
       = f.label :scopes

app/views/admin/applications/index.html.haml

@@ -12,6 +12,7 @@
       %th Callback URL
       %th Clients
       %th Trusted
+      %th Confidential
       %th
       %th
   %tbody.oauth-applications
@@ -21,6 +22,7 @@
         %td= application.redirect_uri
         %td= @application_counts[application.id].to_i
         %td= application.trusted? ? 'Y': 'N'
+        %td= application.confidential? ? 'Y': 'N'
         %td= link_to 'Edit', edit_admin_application_path(application), class: 'btn btn-link'
         %td= render 'delete_form', application: application
 = paginate @applications, theme: 'gitlab'

app/views/admin/applications/show.html.haml

@@ -36,6 +36,12 @@
       %td
         = @application.trusted? ? 'Y' : 'N'
 
+    %tr
+      %td
+        Confidential
+      %td
+        = @application.confidential? ? 'Y' : 'N'
+
     = render "shared/tokens/scopes_list", token: @application
 
 .form-actions

app/views/doorkeeper/applications/_form.html.haml

@@ -15,6 +15,12 @@
       %span.form-text.text-muted
         = _('Use <code>%{native_redirect_uri}</code> for local tests').html_safe % { native_redirect_uri: Doorkeeper.configuration.native_redirect_uri }
 
+  .form-group.form-check
+    = f.check_box :confidential, class: 'form-check-input'
+    = f.label :confidential, class: 'label-bold form-check-label'
+    %span.form-text.text-muted
+      = _('The application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential.')
+
   .form-group
     = f.label :scopes, class: 'label-bold'
     = render 'shared/tokens/scopes_form', prefix: 'doorkeeper_application', token: application, scopes: @scopes

app/views/doorkeeper/applications/show.html.haml

@@ -34,6 +34,12 @@
           %div
             %span.monospace= uri
 
+    %tr
+      %td
+        = _('Confidential')
+      %td
+        = @application.confidential? ? _('Yes') : _('No')
+
     = render "shared/tokens/scopes_list", token: @application
 
 .form-actions

changelogs/unreleased/jhyson-doorkeeper_upgrade.yml

+---
+title: Upgrade Doorkeeper to 4.4.3 to address CVE-2018-1000211
+merge_request: 20953
+author:
+type: security

config/initializers/doorkeeper.rb

@@ -118,8 +118,8 @@ end
 # app created does not match the complete list of scopes of the configured app.
 # It also prevents the OAuth authorize application window to appear every time.
 
-# Remove after we upgrade the doorkeeper gem from version 4.3.2
-if Doorkeeper.gem_version > Gem::Version.new('4.3.2')
+# Remove after we upgrade the doorkeeper gem from version 4.x
+if Doorkeeper.gem_version > Gem::Version.new('5.0.0')
   raise "Doorkeeper was upgraded, please remove the monkey patch in #{__FILE__}"
 end
 

db/migrate/20191127163053_add_confidential_to_doorkeeper_application.rb

+# frozen_string_literal: true
+
+class AddConfidentialToDoorkeeperApplication < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default( # rubocop:disable Migration/AddColumnWithDefault
+      :oauth_applications,
+      :confidential,
+      :boolean,
+      default: false, # assume all existing applications are non-confidential
+      allow_null: false
+    )
+
+    # set the default to true so that all future applications are confidential by default
+    change_column_default(:oauth_applications, :confidential, true)
+  end
+
+  def down
+    remove_column :oauth_applications, :confidential
+  end
+end

db/schema.rb

@@ -2853,6 +2853,7 @@ ActiveRecord::Schema.define(version: 2020_01_21_132641) do
     t.integer "owner_id"
     t.string "owner_type"
     t.boolean "trusted", default: false, null: false
+    t.boolean "confidential", default: true, null: false
     t.index ["owner_id", "owner_type"], name: "index_oauth_applications_on_owner_id_and_owner_type"
     t.index ["uid"], name: "index_oauth_applications_on_uid", unique: true
   end

doc/api/applications.md

@@ -22,11 +22,12 @@ POST /applications
 
 Parameters:
 
-| Attribute      | Type   | Required | Description                      |
-|:---------------|:-------|:---------|:---------------------------------|
-| `name`         | string | yes      | Name of the application.         |
-| `redirect_uri` | string | yes      | Redirect URI of the application. |
-| `scopes`       | string | yes      | Scopes of the application.       |
+| Attribute      | Type    | Required | Description                      |
+|:---------------|:--------|:---------|:---------------------------------|
+| `name`         | string  | yes      | Name of the application.         |
+| `redirect_uri` | string  | yes      | Redirect URI of the application. |
+| `scopes`       | string  | yes      | Scopes of the application.       |
+| `confidential` | boolean | no       | The application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential. Defaults to `true` if not supplied |
 
 Example request:
 
@@ -42,7 +43,8 @@ Example response:
     "application_id": "5832fc6e14300a0d962240a8144466eef4ee93ef0d218477e55f11cf12fc3737",
     "application_name": "MyApplication",
     "secret": "ee1dd64b6adc89cf7e2c23099301ccc2c61b441064e9324d963c46902a85ec34",
-    "callback_url": "http://redirect.uri"
+    "callback_url": "http://redirect.uri",
+    "confidential": true
 }
 ```
 
@@ -68,7 +70,8 @@ Example response:
         "id":1,
         "application_id": "5832fc6e14300a0d962240a8144466eef4ee93ef0d218477e55f11cf12fc3737",
         "application_name": "MyApplication",
-        "callback_url": "http://redirect.uri"
+        "callback_url": "http://redirect.uri",
+        "confidential": true
     }
 ]
 ```

lib/api/applications.rb

@@ -14,6 +14,9 @@ module API
         requires :name, type: String, desc: 'Application name'
         requires :redirect_uri, type: String, desc: 'Application redirect URI'
         requires :scopes, type: String, desc: 'Application scopes'
+
+        optional :confidential, type: Boolean, default: true,
+          desc: 'Application will be used where the client secret is confidential'
       end
       post do
         application = Doorkeeper::Application.new(declared_params)

lib/api/entities.rb

@@ -1828,6 +1828,7 @@ module API
       expose :uid, as: :application_id
       expose :name, as: :application_name
       expose :redirect_uri, as: :callback_url
+      expose :confidential
     end
 
     # Use with care, this exposes the secret

locale/gitlab.pot

@@ -18420,6 +18420,9 @@ msgstr ""
 msgid "The amount of seconds after which a request to get a secondary node status will time out."
 msgstr ""
 
+msgid "The application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential."
+msgstr ""
+
 msgid "The branch for this project has no active pipeline configuration."
 msgstr ""
 

spec/controllers/admin/applications_controller_spec.rb

@@ -40,7 +40,7 @@ describe Admin::ApplicationsController do
 
   describe 'POST #create' do
     it 'creates the application' do
-      create_params = attributes_for(:application, trusted: true)
+      create_params = attributes_for(:application, trusted: true, confidential: false)
 
       expect do
         post :create, params: { doorkeeper_application: create_params }
@@ -60,16 +60,34 @@ describe Admin::ApplicationsController do
       expect(response).to render_template :new
       expect(assigns[:scopes]).to be_kind_of(Doorkeeper::OAuth::Scopes)
     end
+
+    context 'when the params are for a confidential application' do
+      it 'creates a confidential application' do
+        create_params = attributes_for(:application, confidential: true)
+
+        expect do
+          post :create, params: { doorkeeper_application: create_params }
+        end.to change { Doorkeeper::Application.count }.by(1)
+
+        application = Doorkeeper::Application.last
+
+        expect(response).to redirect_to(admin_application_path(application))
+        expect(application).to have_attributes(create_params.except(:uid, :owner_type))
+      end
+    end
   end
 
   describe 'PATCH #update' do
     it 'updates the application' do
-      patch :update, params: { id: application.id, doorkeeper_application: { redirect_uri: 'http://example.com/', trusted: true } }
+      doorkeeper_params = { redirect_uri: 'http://example.com/', trusted: true, confidential: false }
+
+      patch :update, params: { id: application.id, doorkeeper_application: doorkeeper_params }
 
       application.reload
 
       expect(response).to redirect_to(admin_application_path(application))
-      expect(application).to have_attributes(redirect_uri: 'http://example.com/', trusted: true)
+      expect(application)
+        .to have_attributes(redirect_uri: 'http://example.com/', trusted: true, confidential: false)
     end
 
     it 'renders the application form on errors' do
@@ -78,5 +96,16 @@ describe Admin::ApplicationsController do
       expect(response).to render_template :edit
       expect(assigns[:scopes]).to be_kind_of(Doorkeeper::OAuth::Scopes)
     end
+
+    context 'when updating the application to be confidential' do
+      it 'successfully sets the application to confidential' do
+        doorkeeper_params = { confidential: true }
+
+        patch :update, params: { id: application.id, doorkeeper_application: doorkeeper_params }
+
+        expect(response).to redirect_to(admin_application_path(application))
+        expect(application).to be_confidential
+      end
+    end
   end
 end

spec/features/admin/admin_manage_applications_spec.rb

@@ -21,18 +21,21 @@ RSpec.describe 'admin manage applications' do
     expect(page).to have_content('Application ID')
     expect(page).to have_content('Secret')
     expect(page).to have_content('Trusted Y')
+    expect(page).to have_content('Confidential Y')
 
     click_on 'Edit'
     expect(page).to have_content('Edit application')
 
     fill_in :doorkeeper_application_name, with: 'test_changed'
     uncheck :doorkeeper_application_trusted
+    uncheck :doorkeeper_application_confidential
 
     click_on 'Submit'
     expect(page).to have_content('test_changed')
     expect(page).to have_content('Application ID')
     expect(page).to have_content('Secret')
     expect(page).to have_content('Trusted N')
+    expect(page).to have_content('Confidential N')
 
     visit admin_applications_path
     page.within '.oauth-applications' do

spec/features/profiles/user_manages_applications_spec.rb

@@ -20,16 +20,19 @@ describe 'User manages applications' do
     expect(page).to have_content 'Application: test'
     expect(page).to have_content 'Application ID'
     expect(page).to have_content 'Secret'
+    expect(page).to have_content 'Confidential Yes'
 
     click_on 'Edit'
 
     expect(page).to have_content 'Edit application'
     fill_in :doorkeeper_application_name, with: 'test_changed'
+    uncheck :doorkeeper_application_confidential
     click_on 'Save application'
 
     expect(page).to have_content 'test_changed'
     expect(page).to have_content 'Application ID'
     expect(page).to have_content 'Secret'
+    expect(page).to have_content 'Confidential No'
 
     visit applications_profile_path
 

spec/requests/api/applications_spec.rb

@@ -21,6 +21,7 @@ describe API::Applications, :api do
         expect(json_response['application_id']).to eq application.uid
         expect(json_response['secret']).to eq application.secret
         expect(json_response['callback_url']).to eq application.redirect_uri
+        expect(json_response['confidential']).to eq application.confidential
       end
 
       it 'does not allow creating an application with the wrong redirect_uri format' do
@@ -72,6 +73,16 @@ describe API::Applications, :api do
         expect(json_response).to be_a Hash
         expect(json_response['error']).to eq('scopes is missing')
       end
+
+      it 'does not allow creating an application with confidential set to nil' do
+        expect do
+          post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '', confidential: nil }
+        end.not_to change { Doorkeeper::Application.count }
+
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response).to be_a Hash
+        expect(json_response['message']['confidential'].first).to eq('is not included in the list')
+      end
     end
 
     context 'authorized user without authorization' do