Merge branch 'improve-errors-for-jira-oauth' into 'master'

Return 401 when OAuth token generation fails

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

ee/app/controllers/oauth/jira/authorizations_controller.rb

@@ -32,15 +32,17 @@ class Oauth::Jira::AuthorizationsController < ApplicationController
     # We have to modify request.parameters because Doorkeeper::Server reads params from there
     request.parameters[:redirect_uri] = oauth_jira_callback_url
 
-    begin
-      strategy = Doorkeeper::Server.new(self).token_request('authorization_code')
-      auth_response = strategy.authorize.body
-    rescue Doorkeeper::Errors::DoorkeeperError
-      auth_response = {}
-    end
+    strategy = Doorkeeper::Server.new(self).token_request('authorization_code')
+    response = strategy.authorize
 
-    token_type, scope, token = auth_response['token_type'], auth_response['scope'], auth_response['access_token']
+    if response.status == :ok
+      access_token, scope, token_type = response.body.values_at('access_token', 'scope', 'token_type')
 
-    render body: "access_token=#{token}&scope=#{scope}&token_type=#{token_type}"
+      render body: "access_token=#{access_token}&scope=#{scope}&token_type=#{token_type}"
+    else
+      render status: response.status, body: response.body
+    end
+  rescue Doorkeeper::Errors::DoorkeeperError => e
+    render status: :unauthorized, body: e.type
   end
 end

ee/spec/controllers/oauth/jira/authorizations_controller_spec.rb

@@ -32,7 +32,7 @@ describe Oauth::Jira::AuthorizationsController do
   describe 'POST access_token' do
     it 'returns oauth params in a format JIRA expects' do
       expect_any_instance_of(Doorkeeper::Request::AuthorizationCode).to receive(:authorize) do
-        double(body: { 'access_token' => 'fake-123', 'scope' => 'foo', 'token_type' => 'bar' })
+        double(status: :ok, body: { 'access_token' => 'fake-123', 'scope' => 'foo', 'token_type' => 'bar' })
       end
 
       post :access_token, params: { code: 'code-123', client_id: 'client-123', client_secret: 'secret-123' }

ee/spec/requests/jira_authorizations_spec.rb

@@ -10,10 +10,13 @@ describe 'JIRA authorization requests' do
   end
 
   describe 'POST access_token' do
+    let(:client_id) { application.uid }
+    let(:client_secret) { application.secret }
+
     it 'should return values similar to a POST to /oauth/token' do
       post_data = {
-        client_id: application.uid,
-        client_secret: application.secret
+        client_id: client_id,
+        client_secret: client_secret
       }
 
       post '/oauth/token', params: post_data.merge({
@@ -31,5 +34,39 @@ describe 'JIRA authorization requests' do
       access_token, scope, token_type = oauth_response.values_at('access_token', 'scope', 'token_type')
       expect(jira_response).to eq("access_token=#{access_token}&scope=#{scope}&token_type=#{token_type}")
     end
+
+    context 'when authorization fails' do
+      before do
+        post '/login/oauth/access_token', params: {
+          client_id: client_id,
+          client_secret: client_secret,
+          code: try(:code) || generate_access_grant.token
+        }
+      end
+
+      shared_examples 'an unauthorized request' do
+        it 'returns 401' do
+          expect(response.status).to eq(401)
+        end
+      end
+
+      context 'when client_id is invalid' do
+        let(:client_id) { "invalid_id" }
+
+        it_behaves_like 'an unauthorized request'
+      end
+
+      context 'when client_secret is invalid' do
+        let(:client_secret) { "invalid_secret" }
+
+        it_behaves_like 'an unauthorized request'
+      end
+
+      context 'when code is invalid' do
+        let(:code) { "invalid_code" }
+
+        it_behaves_like 'an unauthorized request'
+      end
+    end
   end
 end