Fix TokenExtractor and write specs

src/api/app/services/trigger_controller_service/token_extractor.rb

@@ -26,35 +26,34 @@ def extract_auth_token_from_headers
     end
 
     def extract_token_from_request_signature
-      token = Token::Service.find_by(id: @token_id)
-      return token if token && token.valid_signature?(signature, @body)
+      token = Token.find_by(id: @token_id)
+      return token if token && valid_signature?(token.string)
     end
 
-    def valid_signature?(signature)
+    def valid_signature?(token_string)
       return false unless signature
 
-      ActiveSupport::SecurityUtils.secure_compare(signature_of(@body), signature)
+      ActiveSupport::SecurityUtils.secure_compare(signature_of(token_string), signature)
     end
 
-    def signature_of
-      # FIXME: use sha256 (from X-Hub-Signature-256)
-      'sha1=' + OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), string, @body)
+    def signature_of(token_string)
+      'sha256=' + OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha256'), token_string, @body)
     end
 
     # To trigger the webhook, the sender needs to
     # generate a signature with a secret token.
     # The signature needs to be generated over the
     # payload of the HTTP request and stored
     # in a HTTP header.
-    # GitHub: HTTP_X_HUB_SIGNATURE
+    # GitHub: HTTP_X_HUB_SIGNATURE_256
     # https://developer.github.com/webhooks/securing/
     # Pagure: HTTP_X-Pagure-Signature-256
     # https://docs.pagure.org/pagure/usage/using_webhooks.html
     # Custom signature: HTTP_X_OBS_SIGNATURE
     def signature
-      @http_request.env['HTTP_X_OBS_SIGNATURE'] ||
-        @http_request.env['HTTP_X_HUB_SIGNATURE'] ||
-        @http_request.env['HTTP_X-Pagure-Signature-256']
+      @signature ||= @http_request.env['HTTP_X_OBS_SIGNATURE'] ||
+                     @http_request.env['HTTP_X_HUB_SIGNATURE_256'] ||
+                     @http_request.env['HTTP_X-Pagure-Signature-256']
     end
   end
 end

src/api/spec/controllers/trigger_controller_spec.rb

@@ -208,8 +208,8 @@
       it_behaves_like 'it verifies the signature'
     end
 
-    context 'with HTTP_X_HUB_SIGNATURE http header' do
-      let(:signature_header_name) { 'HTTP_X_HUB_SIGNATURE' }
+    context 'with HTTP_X_HUB_SIGNATURE_256 http header' do
+      let(:signature_header_name) { 'HTTP_X_HUB_SIGNATURE_256' }
 
       it_behaves_like 'it verifies the signature'
     end

src/api/spec/services/trigger_controller_service/token_extractor_spec.rb

@@ -1,29 +1,94 @@
 require 'rails_helper'
-require 'ostruct'
+require 'ostruct' # for OpenStruct
+require 'stringio' # for StringIO
 
 RSpec.describe ::TriggerControllerService::TokenExtractor do
-  let(:request) { OpenStruct.new(env: { 'HTTP_X_GITLAB_EVENT' => 'Push Hook', 'HTTP_X_GITLAB_TOKEN' => 'XY123456' }) }
-  let(:token_extractor) { described_class.new(request) }
+  describe '#call' do
+    subject { described_class.new(request).call }
 
-  describe '.new' do
-    it { expect { token_extractor }.not_to raise_error }
-  end
+    let(:token) { create(:service_token) }
+    let(:request_body) { 'Lorem Ipsum' }
+
+    context 'without a token ID in the params and a token in HTTP headers' do
+      let(:request) { OpenStruct.new(params: {}, body: StringIO.new(request_body), env: {}) }
 
-  describe '#extract_auth_token' do
-    it { expect(token_extractor.extract_auth_token).to eq('Token XY123456') }
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
 
-    context 'with HTTP_AUTHORIZATION' do
-      let(:request) { OpenStruct.new(env: { 'HTTP_AUTHORIZATION' => 'FOO1234' }) }
+    context 'with the ID of a nonexistent token in the params' do
+      let(:request) { OpenStruct.new(params: { id: -1 }, body: StringIO.new(request_body)) }
 
-      it { expect(token_extractor.extract_auth_token).to eq('FOO1234') }
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
     end
-  end
 
-  describe '#valid?' do
-    before do
-      token_extractor.extract_auth_token
+    context 'with the ID of a token in the params and a valid signature in the HTTP headers' do
+      let(:request) { OpenStruct.new(params: { id: token.id }, body: StringIO.new(request_body), env: {}) }
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
     end
 
-    it { expect(token_extractor).to be_valid }
+    ['HTTP_X_OBS_SIGNATURE', 'HTTP_X_HUB_SIGNATURE_256', 'HTTP_X-Pagure-Signature-256'].each do |http_header|
+      context "with the ID of a token in the params and the HTTP header #{http_header} containing a signature of the request body" do
+        let(:signature) { OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha256'), token.string, request_body) }
+        let(:request) do
+          OpenStruct.new(params: { id: token.id }, body: StringIO.new(request_body),
+                         env: { http_header => "sha256=#{signature}" })
+        end
+
+        it 'returns the token' do
+          expect(subject).to eq(token)
+        end
+      end
+    end
+
+    context 'with a wrong token in the HTTP header HTTP_X_GITLAB_TOKEN' do
+      let(:request) do
+        OpenStruct.new(params: {}, body: StringIO.new(request_body),
+                       env: { 'HTTP_X_GITLAB_TOKEN' => 'Québec' })
+      end
+
+      it 'returns nil' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'with a token in the HTTP header HTTP_X_GITLAB_TOKEN' do
+      let(:request) do
+        OpenStruct.new(params: {}, body: StringIO.new(request_body),
+                       env: { 'HTTP_X_GITLAB_TOKEN' => token.string })
+      end
+
+      it 'returns the token' do
+        expect(subject).to eq(token)
+      end
+    end
+
+    context 'with an incorrectly formatted HTTP header HTTP_AUTHORIZATION' do
+      let(:request) do
+        OpenStruct.new(params: {}, body: StringIO.new(request_body),
+                       env: { 'HTTP_AUTHORIZATION' => token.string })
+      end
+
+      it 'raises ActiveRecord::RecordNotFound' do
+        expect { subject }.to raise_error(ActiveRecord::RecordNotFound, "Couldn't find Token")
+      end
+    end
+
+    context 'with a token in the HTTP header HTTP_AUTHORIZATION' do
+      let(:request) do
+        OpenStruct.new(params: {}, body: StringIO.new(request_body),
+                       env: { 'HTTP_AUTHORIZATION' => "Basic #{token.string}" })
+      end
+
+      it 'returns the token' do
+        expect(subject).to eq(token)
+      end
+    end
   end
 end