Skip to content

Commit

Permalink
handle expired lti launches
Browse files Browse the repository at this point in the history
closes FOO-1298
flag=none

also covers an already-consumed
launch

TEST PLAN:
  1) use an lti launch
  2) try to use the same launch again
  3) you should not get a 500, but instead
     the error message from the patchset
     in a 200 response.

Change-Id: I885595694f79c0b4bfffa12a86490ab6396dc195
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/254734
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Weston Dransfield <wdransfield@instructure.com>
QA-Review: Ethan Vizitei <evizitei@instructure.com>
Product-Review: Ethan Vizitei <evizitei@instructure.com>
  • Loading branch information
evizitei committed Dec 9, 2020
1 parent 439ea37 commit 31335da
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 10 deletions.
20 changes: 14 additions & 6 deletions app/controllers/lti/ims/authentication_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

module Lti
module Ims
class InvalidLaunch < StandardError; end

class AuthenticationController < ApplicationController
include Lti::RedisMessageClient

Expand Down Expand Up @@ -47,6 +49,7 @@ def authorize
validate_oidc_params!
validate_current_user!
validate_client_id!
validate_launch_eligibility!

render(
'lti/ims/authentication/authorize.html.erb',
Expand Down Expand Up @@ -82,6 +85,14 @@ def validate_oidc_params!
set_oidc_error!('invalid_request_object', "The 'scope' must be '#{SCOPE}'") if oidc_params[:scope] != SCOPE
end

def validate_launch_eligibility!
return if @oidc_error
id_token
rescue InvalidLaunch => e
Canvas::Errors.capture_exception(:lti, e, :info)
set_oidc_error!('launch_no_longer_valid', "The launch has either expired or already been consumed")
end

def set_oidc_error!(error, error_description)
@oidc_error = {
error: error,
Expand All @@ -107,12 +118,9 @@ def context

def cached_launch_with_nonce
@cached_launch_with_nonce ||= begin
JSON.parse(
fetch_and_delete_launch(
context,
verifier
)
).merge({nonce: oidc_params[:nonce]})
launch_payload = fetch_and_delete_launch(context,verifier)
raise InvalidLaunch, "no payload found in cache" if launch_payload.nil?
JSON.parse(launch_payload).merge({nonce: oidc_params[:nonce]})
end
end

Expand Down
30 changes: 26 additions & 4 deletions spec/controllers/lti/ims/authentication_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,15 @@

subject do
get :authorize, params: params
JSON::JWT.decode(assigns.dig(:id_token, :id_token), :skip_verification)
end

let(:id_token) do
token = assigns.dig(:id_token, :id_token)
if token.present?
JSON::JWT.decode(token, :skip_verification)
else
token
end
end

let(:account) { context }
Expand All @@ -203,11 +211,13 @@
end

it 'correctly sets the nonce of the launch' do
expect(subject['nonce']).to eq nonce
subject
expect(id_token['nonce']).to eq nonce
end

it 'generates an id token' do
expect(subject.except('nonce')).to eq lti_launch.except('nonce')
subject
expect(id_token.except('nonce')).to eq lti_launch.except('nonce')
end

it 'sends the state' do
Expand All @@ -224,7 +234,19 @@
end

it 'launches succesfully' do
expect(subject['nonce']).to eq nonce
subject
expect(id_token['nonce']).to eq nonce
end
end

context "when cached launch has expired" do
before do
fetch_and_delete_launch(context, verifier)
end

it_behaves_like 'non redirect_uri errors' do
let(:expected_message) { "The launch has either expired or already been consumed" }
let(:expected_error) { "launch_no_longer_valid" }
end
end
end
Expand Down

0 comments on commit 31335da

Please sign in to comment.