Merge pull request #1154 from openstax/contracts

Contracts signing for Assignable

Gemfile.lock

@@ -307,10 +307,10 @@ GEM
       eventmachine (>= 0.12.0)
       websocket-driver (>= 0.5.1)
     ffi (1.15.4)
-    fine_print (5.0.0)
+    fine_print (6.0.1)
       action_interceptor
       jquery-rails
-      rails
+      rails (< 7)
       responders
     font-awesome-rails (4.7.0.5)
       railties (>= 3.2, < 6.1)

app/controllers/api/v1/users_controller.rb

@@ -222,6 +222,22 @@ def find_or_create
     end
   end
 
+  ###############################################################
+  # external_id
+  ###############################################################
+
+  api :POST, '/user/external_id', 'Creates an external id for a given user.'
+  description <<-EOS
+    Creates an external id for a given user.
+
+    Both external_id and user_id must be supplied.
+
+    Only trusted apps may call this API. All others will receive a 403 error.
+
+    If the external_id does not exist, it is created and 201 is returned.
+
+    #{json_schema(Api::V1::ExternalIdRepresenter, include: [:readable, :writable])}
+  EOS
   def create_external_id
     standard_create ExternalId.new
   end

app/controllers/terms_controller.rb

@@ -1,10 +1,16 @@
 class TermsController < ApplicationController
-  skip_before_action :authenticate_user!, :complete_signup_profile, only: [:index, :show]
+  # Allow us to sign terms in an iframe
+  # Unlikely that attackers would want to trick our browsers into signing terms
+  skip_forgery_protection only: :agree
 
-  fine_print_skip :general_terms_of_use, :privacy_policy
+  skip_before_action :authenticate_user!, only: [:index, :show, :pose_by_name, :agree]
+  skip_before_action :complete_signup_profile, only: [:index, :show]
 
+  before_action :authenticate_user_with_token!, :allow_iframe_access, only: [:pose_by_name, :agree]
   before_action :get_contract, only: [:show]
 
+  fine_print_skip :general_terms_of_use, :privacy_policy
+
   def index
     @contracts = [FinePrint.get_contract(:general_terms_of_use),
                   FinePrint.get_contract(:privacy_policy)].compact
@@ -29,12 +35,31 @@ def pose
     @contract = FinePrint.get_contract(params[:terms].first)
   end
 
+  def pose_by_name
+    @contract = FinePrint::Contract.published.latest.find_by! name: params[:name]
+    render :pose
+  end
+
   def agree
-    handle_with(TermsAgree, complete: lambda { fine_print_return })
+    handle_with(
+      TermsAgree, complete: -> do
+        params[:r].present? && Host.trusted?(params[:r]) ?
+          redirect_to(params[:r]) : fine_print_return
+      end
+    )
   end
 
   protected
 
+  def authenticate_user_with_token!
+    if params[:token].present?
+      token = Doorkeeper::AccessToken.select(:resource_owner_id).find_by! token: params[:token]
+      @current_user = User.find token.resource_owner_id
+    else
+      authenticate_user!
+    end
+  end
+
   def get_contract
     @contract = FinePrint.get_contract(params[:id])
   end

app/handlers/terms_agree.rb

@@ -1,5 +1,4 @@
 class TermsAgree
-
   lev_handler
 
   paramify :agreement do

app/representers/api/v1/user_representer.rb

@@ -191,5 +191,12 @@ class UserRepresenter < Roar::Decorator
                  description: "A list of the applications the user has accessed",
                  required: false
                }
+
+    collection :signed_contract_names,
+               type: String,
+               readable: true,
+               writeable: false,
+               if: ->(user_options:, **) { user_options.try(:fetch, :include_private_data, false) },
+               getter: ->(*) { FinePrint::Contract.published.latest.signed_by(self).pluck(:name) }
   end
 end

app/views/terms/pose.html.erb

@@ -7,10 +7,14 @@
   <% end %>
 
   <div class="well">
-    <%= simple_format(@contract.content) %>
+    <%= simple_format @contract.content, {}, sanitize: false %>
   </div>
 
-  <%= lev_form_for :agreement, url: agree_to_terms_path, method: :post do |f| %>
+  <%= lev_form_for(
+    :agreement,
+    url: agree_to_terms_path(r: params[:r], token: params[:token]),
+    method: :post
+  ) do |f| %>
     <div class="checkbox">
       <label>
         <%= f.check_box :i_agree %> <%= t :".have_read_terms_and_agree" %>

config/initializers/access_policies.rb

@@ -15,4 +15,4 @@
 OSU::AccessPolicy.register(GroupOwner, GroupOwnerAccessPolicy)
 OSU::AccessPolicy.register(GroupNesting, GroupNestingAccessPolicy)
 OSU::AccessPolicy.register(ApplicationGroup, ApplicationGroupAccessPolicy)
-OSU::AccessPolicy.register(ExternalId, ExternalIdAccessPolicy)
+OSU::AccessPolicy.register(ExternalId, ExternalIdAccessPolicy)

config/initializers/fine_print.rb

@@ -59,3 +59,14 @@
   }
 
 end
+
+# Patch that allows us to use FinePrint 6 on Rails 5
+Rails.application.config.to_prepare do
+  FinePrint::Contract.class_exec do
+    scope :signed_by, ->(user) do
+      joins(:signatures).where(
+        fine_print_signatures: { user_id: user.id, user_type: class_name_for(user) }
+      )
+    end
+  end
+end

config/routes.rb

@@ -210,6 +210,9 @@
       post 'agree', as: 'agree_to'
     end
   end
+  resources :terms, param: :name, only: [] do
+    get :pose, action: :pose_by_name, on: :member
+  end
 
   scope controller: 'static_pages' do
     get 'copyright'

spec/controllers/terms_controller_spec.rb

@@ -0,0 +1,104 @@
+require 'rails_helper'
+require 'byebug'
+
+RSpec.describe TermsController, type: :controller do
+  let(:contract) { FactoryBot.create :fine_print_contract, :published }
+  let!(:user_1)  { create_user 'user1' }
+  let!(:user_2)  { create_user 'user2' }
+
+  let(:trusted_return_url)   { 'https://openstax.org/example' }
+  let(:untrusted_return_url) { 'https://www.example.com' }
+
+  let(:token)    { FactoryBot.create(:doorkeeper_access_token, resource_owner_id: user_2.id).token }
+
+  before         { controller.sign_in! user_1 }
+
+  context 'pose_by_name' do
+    context 'no params' do
+      it 'renders pose form' do
+        get :pose_by_name, params: { name: contract.name }
+        expect(response).to render_template(:pose)
+      end
+    end
+
+    context 'redirect and token params' do
+      it 'passes params to form url' do
+        get :pose_by_name, params: { name: contract.name, r: trusted_return_url, token: token }
+        expect(response).to render_template(:pose)
+      end
+    end
+
+    context 'invalid token' do
+      it 'fails with 404 Not Found' do
+        get :pose_by_name, params: { name: contract.name, token: SecureRandom.hex }
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+  end
+
+  context 'agree' do
+    context 'agreed' do
+      context 'no token or return url' do
+        it 'records the signature and redirects back' do
+          expect do
+            post :agree, params: { agreement: { contract_id: contract.id, i_agree: true } }
+          end.to change { FinePrint::Signature.count }.by(1)
+          signature = FinePrint::Signature.order(:created_at).last
+          expect(signature.user).to eq user_1
+          expect(response).to redirect_to('/')
+        end
+      end
+
+      context 'token and trusted return url' do
+        it 'records the signature for the token user and redirects back to the trusted url' do
+          expect do
+            post :agree, params: {
+              agreement: { contract_id: contract.id, i_agree: true },
+              r: trusted_return_url,
+              token: token
+            }
+          end.to change { FinePrint::Signature.count }.by(1)
+          signature = FinePrint::Signature.order(:created_at).last
+          expect(signature.user).to eq user_2
+          expect(response).to redirect_to(trusted_return_url)
+        end
+      end
+
+      context 'invalid token' do
+        it 'fails with 404 Not Found' do
+          expect do
+            post :agree, params: {
+              agreement: { contract_id: contract.id, i_agree: true },
+              token: SecureRandom.hex
+            }
+          end.not_to change { FinePrint::Signature.count }
+          expect(response).to have_http_status(:not_found)
+        end
+      end
+
+      context 'untrusted return url' do
+        it 'ignores the untrusted url' do
+          expect do
+            post :agree, params: {
+              agreement: { contract_id: contract.id, i_agree: true },
+              r: untrusted_return_url
+            }
+          end.to change { FinePrint::Signature.count }.by(1)
+          signature = FinePrint::Signature.order(:created_at).last
+          expect(signature.user).to eq user_1
+          expect(response).to redirect_to('/')
+        end
+      end
+    end
+
+    context 'did not agree' do
+      it 'does not record a signature' do
+        expect do
+          post :agree, params: { agreement: { contract_id: contract.id } }
+        end.not_to change { FinePrint::Signature.count }
+        expect(response).to redirect_to('/')
+      end
+    end
+
+  end
+end

spec/representers/api/v1/user_representer_spec.rb

@@ -2,9 +2,11 @@
 
 RSpec.describe Api::V1::UserRepresenter, type: :representer do
   let(:user)            { FactoryBot.create :user  }
-  let(:school)            { FactoryBot.create :school  }
+  let(:school)          { FactoryBot.create :school  }
   subject(:representer) { described_class.new(user) }
 
+  let(:contract)  { FactoryBot.create :fine_print_contract, :published }
+
   context 'uuid' do
     it 'can be read' do
       expect(representer.to_hash['uuid']).to eq user.uuid
@@ -158,4 +160,22 @@
       expect { representer.from_hash(hash) }.not_to change { user.reload.grant_tutor_access }
     end
   end
+
+  context 'signed_contract_names' do
+    it 'can be read' do
+      FactoryBot.create :fine_print_signature, contract: contract, user: user
+
+      expect(
+        representer.to_hash(user_options: { include_private_data: true })['signed_contract_names']
+      ).to eq [ contract.name ]
+    end
+
+    it 'cannot be written (attempts are silently ignored)' do
+      hash = { 'signed_contract_names' => [ contract.name ] }
+
+      expect do
+        representer.from_hash(hash, user_options: { include_private_data: true })
+      end.not_to change { FinePrint::Signature.count }
+    end
+  end
 end

spec/support/user_hash.rb

@@ -37,6 +37,7 @@ def user_matcher(user, include_private_data: false)
             Api::V1::ContactInfoRepresenter.new(ci).to_hash.symbolize_keys
           end
         )
+    base_hash[:signed_contract_names] = FinePrint::Contract.published.latest.signed_by(user).pluck(:name)
   end
 
   base_hash.delete_if { |k,v| v.nil? }