Merge branch 'skip-oauth-authorization-for-trusted-applications' into 'master'

[EE] Skip OAuth authorization for trusted applications

See merge request !2456

app/controllers/admin/applications_controller.rb

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

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

@@ -6,6 +6,7 @@
     .col-sm-10
       = f.text_field :name, class: 'form-control'
       = doorkeeper_errors_for application, :name
+
   = content_tag :div, class: 'form-group' do
     = f.label :redirect_uri, class: 'col-sm-2 control-label'
     .col-sm-10
@@ -19,6 +20,13 @@
           %code= Doorkeeper.configuration.native_redirect_uri
           for local tests
 
+  = content_tag :div, class: 'form-group' do
+    = f.label :trusted, class: 'col-sm-2 control-label'
+    .col-sm-10
+      = f.check_box :trusted
+      %span.help-block
+        Trusted applications are automatically authorized on GitLab OAuth flow.
+
   .form-group
     = f.label :scopes, class: 'col-sm-2 control-label'
     .col-sm-10

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

@@ -11,6 +11,7 @@
       %th Name
       %th Callback URL
       %th Clients
+      %th Trusted
       %th
       %th
   %tbody.oauth-applications
@@ -19,5 +20,6 @@
         %td= link_to application.name, admin_application_path(application)
         %td= application.redirect_uri
         %td= application.access_tokens.map(&:resource_owner_id).uniq.count
+        %td= application.trusted? ? 'Y': 'N'
         %td= link_to 'Edit', edit_admin_application_path(application), class: 'btn btn-link'
         %td= render 'delete_form', application: application

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

@@ -23,6 +23,12 @@
           %div
             %span.monospace= uri
 
+    %tr
+      %td
+        Trusted
+      %td
+        = @application.trusted? ? 'Y' : 'N'
+
     = render "shared/tokens/scopes_list", token: @application
 
 .form-actions

changelogs/unreleased-ee/skip-oauth-authorization-for-trusted-applications.yml

+---
+title: Skip oAuth authorization for trusted applications
+merge_request:
+author:

config/initializers/doorkeeper.rb

@@ -92,9 +92,9 @@ Doorkeeper.configure do
   # Under some circumstances you might want to have applications auto-approved,
   # so that the user skips the authorization step.
   # For example if dealing with trusted a application.
-  # skip_authorization do |resource_owner, client|
-  #   client.superapp? or resource_owner.admin?
-  # end
+  skip_authorization do |resource_owner, client|
+    client.application.trusted?
+  end
 
   # WWW-Authenticate Realm (default "Doorkeeper").
   # realm "Doorkeeper"

db/migrate/20170717200542_add_trusted_column_to_oauth_applications.rb

+class AddTrustedColumnToOauthApplications < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:oauth_applications, :trusted, :boolean, default: false)
+  end
+
+  def down
+    remove_column(:oauth_applications, :trusted)
+  end
+end

db/schema.rb

@@ -1209,6 +1209,7 @@ ActiveRecord::Schema.define(version: 20170719182937) do
     t.datetime "updated_at"
     t.integer "owner_id"
     t.string "owner_type"
+    t.boolean "trusted", default: false, null: false
   end
 
   add_index "oauth_applications", ["owner_id", "owner_type"], name: "index_oauth_applications_on_owner_id_and_owner_type", using: :btree

doc/integration/oauth_provider.md

@@ -63,6 +63,9 @@ it from the admin area.
 
 ![OAuth admin_applications](img/oauth_provider_admin_application.png)
 
+You're also able to mark an application as _trusted_ when creating it through the admin area. By doing that,
+the user authorization step is automatically skipped for this application.
+
 ---
 
 ## Authorized applications

spec/controllers/admin/applications_controller_spec.rb

@@ -28,13 +28,16 @@ describe Admin::ApplicationsController do
 
   describe 'POST #create' do
     it 'creates the application' do
+      create_params = attributes_for(:application, trusted: true)
+
       expect do
-        post :create, doorkeeper_application: attributes_for(:application)
+        post :create, 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
 
     it 'renders the application form on errors' do
@@ -49,10 +52,12 @@ describe Admin::ApplicationsController do
 
   describe 'PATCH #update' do
     it 'updates the application' do
-      patch :update, id: application.id, doorkeeper_application: { redirect_uri: 'http://example.com/' }
+      patch :update, id: application.id, doorkeeper_application: { redirect_uri: 'http://example.com/', trusted: true }
+
+      application.reload
 
       expect(response).to redirect_to(admin_application_path(application))
-      expect(application.reload.redirect_uri).to eq 'http://example.com/'
+      expect(application).to have_attributes(redirect_uri: 'http://example.com/', trusted: true)
     end
 
     it 'renders the application form on errors' do

spec/controllers/oauth/authorizations_controller_spec.rb

@@ -42,8 +42,8 @@ describe Oauth::AuthorizationsController do
       end
 
       it 'deletes session.user_return_to and redirects when skip authorization' do
+        doorkeeper.update(trusted: true)
         request.session['user_return_to'] = 'http://example.com'
-        allow(controller).to receive(:skip_authorization?).and_return(true)
 
         get :new, params
 

spec/features/admin/admin_manage_applications_spec.rb

@@ -13,19 +13,24 @@ RSpec.describe 'admin manage applications' do
 
     fill_in :doorkeeper_application_name, with: 'test'
     fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com'
+    check :doorkeeper_application_trusted
     click_on 'Submit'
     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('Trusted Y')
 
     click_on 'Edit'
     expect(page).to have_content('Edit application')
 
     fill_in :doorkeeper_application_name, with: 'test_changed'
+    uncheck :doorkeeper_application_trusted
+
     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')
 
     visit admin_applications_path
     page.within '.oauth-applications' do

spec/services/projects/destroy_service_spec.rb

@@ -127,7 +127,7 @@ describe Projects::DestroyService do
     context 'errors' do
       context 'when `remove_legacy_registry_tags` fails' do
         before do
-          expect_any_instance_of(Projects::DestroyService)
+          expect_any_instance_of(described_class)
             .to receive(:remove_legacy_registry_tags).and_return(false)
         end
 
@@ -136,7 +136,7 @@ describe Projects::DestroyService do
 
       context 'when `remove_repository` fails' do
         before do
-          expect_any_instance_of(Projects::DestroyService)
+          expect_any_instance_of(described_class)
             .to receive(:remove_repository).and_return(false)
         end