Revert "Merge branch 'security-add-presence-validation-to-oauth-applications' into 'master'"

This reverts merge request !797

app/services/applications/create_service.rb

@@ -11,16 +11,7 @@ module Applications
 
     # EE would override and use `request` arg
     def execute(request)
-      @application = Doorkeeper::Application.new(params)
-
-      unless params[:scopes].present?
-        @application.errors.add(:base, _("Scopes can't be blank"))
-
-        return @application
-      end
-
-      @application.save
-      @application
+      Doorkeeper::Application.create(params)
     end
   end
 end

ee/changelogs/unreleased/security-add-presence-validation-to-oauth-applications.yml

----
-title: Update default OAuth Applications scope to 'read_user' & add scopes presence validation on OAuth Application creation
-merge_request: 797
-author:
-type: security

ee/spec/controllers/admin/applications_controller_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe Admin::ApplicationsController do
     it 'creates the application' do
       stub_licensed_features(extended_audit_events: true)
 
-      create_params = attributes_for(:application, trusted: true, scopes: ['api'])
+      create_params = attributes_for(:application, trusted: true)
 
       expect do
         post :create, params: { doorkeeper_application: create_params }

ee/spec/controllers/oauth/applications_controller_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe Oauth::ApplicationsController do
         sign_in(user)
 
         application = build(:oauth_application)
-        application_attributes = application.attributes.merge("scopes" => ['read_user'])
+        application_attributes = application.attributes.merge("scopes" => [])
 
         expect { post :create, params: { doorkeeper_application: application_attributes } }.to change { AuditEvent.count }.by(1)
       end

ee/spec/services/applications/create_service_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe ::Applications::CreateService do
   let(:user) { create(:user) }
-  let(:params) { attributes_for(:application, scopes: ['read_user']) }
+  let(:params) { attributes_for(:application) }
   let(:request) { ActionController::TestRequest.new({ remote_ip: "127.0.0.1" }, ActionController::TestSession.new, nil) }
 
   subject { described_class.new(user, params) }

lib/gitlab/auth.rb

@@ -24,7 +24,7 @@ module Gitlab
     PROFILE_SCOPES = [:profile, :email].freeze
 
     # Default scopes for OAuth applications that don't define their own
-    DEFAULT_SCOPES = [:read_user].freeze
+    DEFAULT_SCOPES = [:api].freeze
 
     CI_JOB_USER = 'gitlab-ci-token'
 

spec/controllers/admin/applications_controller_spec.rb

@@ -40,7 +40,7 @@ RSpec.describe Admin::ApplicationsController do
 
   describe 'POST #create' do
     it 'creates the application' do
-      create_params = attributes_for(:application, trusted: true, confidential: false, scopes: ['api'])
+      create_params = attributes_for(:application, trusted: true, confidential: false)
 
       expect do
         post :create, params: { doorkeeper_application: create_params }
@@ -63,7 +63,7 @@ RSpec.describe Admin::ApplicationsController do
 
     context 'when the params are for a confidential application' do
       it 'creates a confidential application' do
-        create_params = attributes_for(:application, confidential: true, scopes: ['read_user'])
+        create_params = attributes_for(:application, confidential: true)
 
         expect do
           post :create, params: { doorkeeper_application: create_params }
@@ -75,18 +75,6 @@ RSpec.describe Admin::ApplicationsController do
         expect(application).to have_attributes(create_params.except(:uid, :owner_type))
       end
     end
-
-    context 'when scopes are not present' do
-      it 'renders the application form on errors' do
-        create_params = attributes_for(:application, trusted: true, confidential: false)
-
-        expect do
-          post :create, params: { doorkeeper_application: create_params }
-        end.not_to change { Doorkeeper::Application.count }
-
-        expect(response).to render_template :new
-      end
-    end
   end
 
   describe 'PATCH #update' do

spec/controllers/oauth/applications_controller_spec.rb

@@ -123,8 +123,7 @@ RSpec.describe Oauth::ApplicationsController do
           invalid_uri_params = {
             doorkeeper_application: {
               name: 'foo',
-              redirect_uri: 'javascript://alert()',
-              scopes: ['read_user']
+              redirect_uri: 'javascript://alert()'
             }
           }
 
@@ -134,23 +133,6 @@ RSpec.describe Oauth::ApplicationsController do
         end
       end
 
-      context 'when scopes are not present' do
-        render_views
-
-        it 'shows an error for blank scopes' do
-          invalid_uri_params = {
-            doorkeeper_application: {
-              name: 'foo',
-              redirect_uri: 'http://example.org'
-            }
-          }
-
-          post :create, params: invalid_uri_params
-
-          expect(response.body).to include 'Scopes can't be blank'
-        end
-      end
-
       it_behaves_like 'redirects to login page when the user is not signed in'
       it_behaves_like 'redirects to 2fa setup page when the user requires it'
     end
@@ -190,8 +172,7 @@ RSpec.describe Oauth::ApplicationsController do
     {
       doorkeeper_application: {
         name: 'foo',
-        redirect_uri: 'http://example.org',
-        scopes: ['read_user']
+        redirect_uri: 'http://example.org'
       }
     }
   end

spec/controllers/oauth/authorizations_controller_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe Oauth::AuthorizationsController do
     context 'when the user is confirmed' do
       context 'when there is already an access token for the application with a matching scope' do
         before do
-          scopes = Doorkeeper::OAuth::Scopes.from_string('read_user')
+          scopes = Doorkeeper::OAuth::Scopes.from_string('api')
 
           allow(Doorkeeper.configuration).to receive(:scopes).and_return(scopes)
 

spec/features/admin/admin_manage_applications_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe 'admin manage applications' do
     sign_in(create(:admin))
   end
 
-  it 'creates new oauth application' do
+  it do
     visit admin_applications_path
 
     click_on 'New application'
@@ -16,7 +16,6 @@ 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
-    check :doorkeeper_application_scopes_read_user
     click_on 'Submit'
     expect(page).to have_content('Application: test')
     expect(page).to have_content('Application ID')
@@ -44,19 +43,4 @@ RSpec.describe 'admin manage applications' do
     end
     expect(page.find('.oauth-applications')).not_to have_content('test_changed')
   end
-
-  context 'when scopes are blank' do
-    it 'returns an error' do
-      visit admin_applications_path
-
-      click_on 'New application'
-      expect(page).to have_content('New application')
-
-      fill_in :doorkeeper_application_name, with: 'test'
-      fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com'
-      click_on 'Submit'
-
-      expect(page).to have_content("Scopes can't be blank")
-    end
-  end
 end

spec/features/profiles/user_manages_applications_spec.rb

@@ -15,7 +15,6 @@ RSpec.describe 'User manages applications' do
 
     fill_in :doorkeeper_application_name,         with: 'test'
     fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com'
-    check :doorkeeper_application_scopes_read_user
     click_on 'Save application'
 
     expect(page).to have_content 'Application: test'
@@ -42,16 +41,4 @@ RSpec.describe 'User manages applications' do
     end
     expect(page.find('.oauth-applications')).not_to have_content 'test_changed'
   end
-
-  context 'when scopes are blank' do
-    it 'returns an error' do
-      expect(page).to have_content 'Add new application'
-
-      fill_in :doorkeeper_application_name,         with: 'test'
-      fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com'
-      click_on 'Save application'
-
-      expect(page).to have_content("Scopes can't be blank")
-    end
-  end
 end

spec/lib/gitlab/auth_spec.rb

@@ -24,13 +24,13 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
     end
 
     it 'DEFAULT_SCOPES contains all default scopes' do
-      expect(subject::DEFAULT_SCOPES).to eq [:read_user]
+      expect(subject::DEFAULT_SCOPES).to eq [:api]
     end
 
     it 'optional_scopes contains all non-default scopes' do
       stub_container_registry_config(enabled: true)
 
-      expect(subject.optional_scopes).to eq %i[api read_api read_repository write_repository read_registry write_registry sudo openid profile email]
+      expect(subject.optional_scopes).to eq %i[read_user read_api read_repository write_repository read_registry write_registry sudo openid profile email]
     end
   end
 

spec/requests/openid_connect_spec.rb

@@ -189,7 +189,7 @@ RSpec.describe 'OpenID Connect requests' do
       expect(response).to have_gitlab_http_status(:ok)
       expect(json_response['issuer']).to eq('http://localhost')
       expect(json_response['jwks_uri']).to eq('http://www.example.com/oauth/discovery/keys')
-      expect(json_response['scopes_supported']).to eq(%w[read_user api read_api read_repository write_repository sudo openid profile email])
+      expect(json_response['scopes_supported']).to eq(%w[api read_user read_api read_repository write_repository sudo openid profile email])
     end
   end
 

spec/services/applications/create_service_spec.rb

@@ -6,24 +6,9 @@ RSpec.describe ::Applications::CreateService do
   include TestRequestHelpers
 
   let(:user) { create(:user) }
+  let(:params) { attributes_for(:application) }
 
   subject { described_class.new(user, params) }
 
-  context 'when scopes are present' do
-    let(:params) { attributes_for(:application, scopes: ['read_user']) }
-
-    it { expect { subject.execute(test_request) }.to change { Doorkeeper::Application.count }.by(1) }
-  end
-
-  context 'when scopes are missing' do
-    let(:params) { attributes_for(:application) }
-
-    it { expect { subject.execute(test_request) }.not_to change { Doorkeeper::Application.count } }
-
-    it 'includes blank scopes error message' do
-      application = subject.execute(test_request)
-
-      expect(application.errors.full_messages).to include "Scopes can't be blank"
-    end
-  end
+  it { expect { subject.execute(test_request) }.to change { Doorkeeper::Application.count }.by(1) }
 end