Merge pull request #26 from Drieam/issue/15

Perform launch validation checks (#15)

app/controllers/launches_controller.rb

@@ -38,18 +38,25 @@ def callback # rubocop:todo Metrics/AbcSize
     redirect_to URI(tool.signed_open_id_connect_initiation_url(login_hint: login_hint)).to_s
   end
 
-  def auth
+  def auth # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
     # Parse and validate the login hint
-    login_hint = Keypair.jwt_decode(params[:login_hint])
+    login_hint = Keypair.jwt_decode(params.fetch(:login_hint))
 
     # Find the requested tool based on the login hint
     tool = Tool.find_by!(client_id: login_hint.fetch('tool_client_id'))
 
-    # raise 'Security Error, TOOL_TARGET_LINK_URI != redirect_uri' if TOOL_TARGET_LINK_URI != params['redirect_uri']
-    # raise 'Security Error, params[:login_hint] != cookies[:login_hint]' if params[:login_hint] != cookies[:login_hint]
-    # # TODO: check nonce is not used before
+    # Perform launch validations
+    if params.fetch(:redirect_uri) != tool.target_link_uri
+      raise Launch::InvalidError, 'redirect_uri does not match tool setting'
+    end
+    if params.fetch(:login_hint) != cookies[:login_hint]
+      raise Launch::InvalidError, 'open id connect flow could not be validated'
+    end
+    raise Launch::InvalidError, 'nonce is used before' unless Nonce.verify(tool, params.fetch(:nonce))
 
     # Prepare the launch
     @launch = Launch.new(tool: tool, context: login_hint.fetch('context'))
+  rescue Launch::InvalidError => e
+    render plain: "Invalid launch since #{e.message}", status: :unprocessable_entity
   end
 end

app/models/launch.rb

@@ -1,6 +1,14 @@
 # frozen_string_literal: true
 
 class Launch
+  class InvalidError < StandardError
+    attr_accessor :message
+
+    def initialize(message)
+      self.message = message
+    end
+  end
+
   # The context requires to have a sub key since that should originate from the OIDC flow
   def initialize(tool:, context:)
     @tool = tool

app/models/nonce.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class Nonce < ApplicationRecord
+  belongs_to :tool, inverse_of: :nonces
+
+  self.primary_key = :created_at
+
+  validates :key, presence: true
+
+  # Returns a boolean if the passed in string is used before
+  # This check is handled by the unique constraint on the database
+  def self.verify(tool, nonce)
+    create!(tool: tool, key: nonce).persisted?
+  rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique
+    false
+  end
+end

app/models/tool.rb

@@ -2,6 +2,7 @@
 
 class Tool < ApplicationRecord
   belongs_to :auth_server, inverse_of: :tools
+  has_many :nonces, inverse_of: :tool, dependent: :delete_all
 
   validates :name, presence: true
   validates :client_id, presence: true, uniqueness: true

db/migrate/20200207081734_create_nonces.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+class CreateNonces < ActiveRecord::Migration[6.0]
+  def change
+    create_table :nonces, id: false do |t|
+      t.belongs_to :tool, null: false, foreign_key: true, index: false, type: :uuid
+
+      t.string :key, null: false
+      t.timestamps
+
+      t.index %i[tool_id key], unique: true
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_02_04_122725) do
+ActiveRecord::Schema.define(version: 2020_02_07_081734) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
@@ -39,6 +39,14 @@
     t.index ["jwk_kid"], name: "index_keypairs_on_jwk_kid"
   end
 
+  create_table "nonces", id: false, force: :cascade do |t|
+    t.uuid "tool_id", null: false
+    t.string "key", null: false
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+    t.index ["tool_id", "key"], name: "index_nonces_on_tool_id_and_key", unique: true
+  end
+
   create_table "tools", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
     t.uuid "auth_server_id", null: false
     t.string "client_id", null: false
@@ -53,5 +61,6 @@
     t.index ["client_id"], name: "index_tools_on_client_id", unique: true
   end
 
+  add_foreign_key "nonces", "tools"
   add_foreign_key "tools", "auth_servers"
 end

spec/controllers/launches_controller_spec.rb

@@ -161,9 +161,56 @@
       }
     end
 
-    before { get :auth, params: params }
-
     context 'with valid params' do
+      before { request.cookies[:login_hint] = login_hint }
+      before { get :auth, params: params }
+      it { is_expected.to respond_with 200 }
+      it 'sets the launch instance variable' do
+        expect(assigns(:launch)).to be_a Launch
+      end
+      it 'sets the correct tool on the launch' do
+        expect(assigns(:launch).target_link_uri).to eq tool.target_link_uri
+      end
+      it 'sets the context on the launch' do
+        expect(assigns(:launch).payload).to include login_hint_payload[:context]
+      end
+    end
+    context 'when redirect_uri does not match target_link_uri' do
+      before { request.cookies[:login_hint] = login_hint }
+      let(:invalid_params) { params.merge(redirect_uri: 'https://invalid.com/redirected') }
+      before { get :auth, params: invalid_params }
+      it { is_expected.to respond_with 422 }
+      it { expect(response.body).to eq 'Invalid launch since redirect_uri does not match tool setting' }
+      it { expect(assigns(:launch)).to eq nil }
+    end
+    context 'when login_hint does not match login_hint cookie' do
+      let!(:invalid_login_hint) { Keypair.jwt_encode(login_hint_payload.merge(tool_client_id: '0')) }
+      before { request.cookies[:login_hint] = invalid_login_hint }
+      before { get :auth, params: params }
+      it { is_expected.to respond_with 422 }
+      it { expect(response.body).to eq 'Invalid launch since open id connect flow could not be validated' }
+      it { expect(assigns(:launch)).to eq nil }
+    end
+    context 'when request is retried' do
+      before { request.cookies[:login_hint] = login_hint }
+      before { get :auth, params: params }
+      before { get :auth, params: params }
+      it { is_expected.to respond_with 422 }
+      it { expect(response.body).to eq 'Invalid launch since nonce is used before' }
+    end
+    context 'when nonce already used by current tool' do
+      before { Nonce.verify(tool, params[:nonce]) }
+      before { request.cookies[:login_hint] = login_hint }
+      before { get :auth, params: params }
+      it { is_expected.to respond_with 422 }
+      it { expect(response.body).to eq 'Invalid launch since nonce is used before' }
+      it { expect(assigns(:launch)).to eq nil }
+    end
+    context 'when nonce already used by other tool' do
+      before { Nonce.verify(create(:tool), params[:nonce]) }
+      before { request.cookies[:login_hint] = login_hint }
+      before { get :auth, params: params }
+      it { is_expected.to respond_with 200 }
       it 'sets the launch instance variable' do
         expect(assigns(:launch)).to be_a Launch
       end

spec/models/nonce_spec.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Nonce, type: :model do
+  describe 'database' do
+    it { is_expected.to have_db_column(:tool_id).of_type(:uuid).with_options(null: false, foreign_key: true) }
+    it { is_expected.to have_db_column(:key).of_type(:string).with_options(null: false) }
+    it { is_expected.to have_db_column(:created_at).of_type(:datetime).with_options(null: false) }
+    it { is_expected.to have_db_column(:updated_at).of_type(:datetime).with_options(null: false) }
+    it { is_expected.to have_db_index(%i[tool_id key]).unique }
+    it { is_expected.to_not have_db_index(:tool_id) }
+    it { is_expected.to_not have_db_index(:key) }
+  end
+
+  describe 'relations' do
+    it { is_expected.to belong_to(:tool).inverse_of(:nonces) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:key) }
+  end
+
+  describe 'methods' do
+    describe '.verify' do
+      context 'without a tool' do
+        it 'returns false' do
+          expect(described_class.verify(nil, SecureRandom.hex)).to eq false
+        end
+      end
+      context 'within a single tool' do
+        let!(:tool) { create :tool }
+        context 'with empty nonce' do
+          it 'returns false' do
+            expect(described_class.verify(tool, nil)).to eq false
+          end
+        end
+        context 'with new nonce' do
+          it 'it returns true' do
+            expect(described_class.verify(tool, SecureRandom.hex)).to eq true
+          end
+        end
+        context 'with used nonce' do
+          let(:nonce) { SecureRandom.uuid }
+          it 'it returns false' do
+            described_class.verify(tool, nonce)
+            expect(described_class.verify(tool, nonce)).to eq false
+          end
+        end
+      end
+      context 'with multiple tools' do
+        let!(:tool1) { create :tool }
+        let!(:tool2) { create :tool }
+        let(:nonce) { SecureRandom.uuid }
+        it 'can have the same nonce for multiple tools' do
+          expect(described_class.verify(tool1, nonce)).to eq true
+          expect(described_class.verify(tool2, nonce)).to eq true
+          expect(described_class.verify(tool2, nonce)).to eq false
+        end
+      end
+    end
+  end
+end

spec/models/tool_spec.rb

@@ -20,6 +20,7 @@
 
   describe 'relations' do
     it { is_expected.to belong_to(:auth_server).inverse_of(:tools) }
+    it { is_expected.to have_many(:nonces).inverse_of(:tool).dependent(:delete_all) }
   end
 
   describe 'validations' do