Skip to content

Commit

Permalink
allow reg_key to be used for lti2 registration
Browse files Browse the repository at this point in the history
fixes PLAT-2422

test plan:

- use the oauth2 flow to register a tool proxy using just
the reg_key and reg_password

-it should work

-the developer credentials should still work

-the legacy oauth1 should still work

Change-Id: I74f754c06f0d4a46158588f39bee75321fad3a3d
Reviewed-on: https://gerrit.instructure.com/106623
Tested-by: Jenkins
Reviewed-by: Weston Dransfield <[email protected]>
QA-Review: August Thornton <[email protected]>
Product-Review: Nathan Mills <[email protected]>
  • Loading branch information
rivernate committed Mar 29, 2017
1 parent 2564a54 commit 236fc45
Show file tree
Hide file tree
Showing 14 changed files with 122 additions and 80 deletions.
2 changes: 1 addition & 1 deletion Gemfile.d/app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
gem 'ruby2ruby', '2.3.1', require: false
gem 'ruby_parser', '3.8.4', require: false
gem 'icalendar', '1.5.4', require: false
gem 'ims-lti', '2.1.0.beta.4', require: 'ims'
gem 'ims-lti', '2.1.2', require: 'ims'
gem 'json', '2.0.3'
gem 'oj', '2.17.1'
gem 'jwt', '1.2.1', require: false
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/lti/ims/access_token_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def authorized_lti2_tool

def validate_access_token!
access_token.validate!
raise Lti::Oauth2::InvalidTokenError 'Developer Key is not active' unless developer_key&.active?
raise Lti::Oauth2::InvalidTokenError 'Developer Key is not active' if developer_key && !developer_key.active?
rescue Lti::Oauth2::InvalidTokenError
raise
rescue StandardError => e
Expand Down
11 changes: 7 additions & 4 deletions app/controllers/lti/ims/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ module Ims
class AuthorizationController < ApplicationController

skip_before_action :load_user
before_action :require_context

SERVICE_DEFINITIONS = [
{
id: 'vnd.Canvas.authorization',
endpoint: "api/lti/authorize",
endpoint: -> (context) {"api/lti/#{context.class.name.downcase}s/#{context.id}/authorize"},
format: ['application/json'].freeze,
action: ['POST'].freeze
}.freeze
Expand Down Expand Up @@ -96,14 +97,16 @@ def authorize
code = params[:code]
jwt_validator = Lti::Oauth2::AuthorizationValidator.new(
jwt: params[:assertion],
authorization_url: lti_oauth2_authorize_url,
code: code
authorization_url: polymorphic_url([@context, :lti_oauth2_authorize]),
code: code,
context: @context
)
jwt_validator.validate!
file_host, _ = HostUrl.file_host_with_shard(@domain_root_account || Account.default, request.host_with_port)
aud = [request.host, request.protocol + file_host]
reg_key = code || jwt_validator.sub
render json: {
access_token: Lti::Oauth2::AccessToken.create_jwt(aud: aud, sub: jwt_validator.sub, reg_key: code).to_s,
access_token: Lti::Oauth2::AccessToken.create_jwt(aud: aud, sub: jwt_validator.sub, reg_key: reg_key).to_s,
token_type: 'bearer',
expires_in: Setting.get('lti.oauth2.access_token.expiration', 1.hour.to_s)
}
Expand Down
3 changes: 1 addition & 2 deletions app/controllers/lti/ims/tool_proxy_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,11 @@ def show

def create
if oauth2_request?
dev_key = DeveloperKey.find_cached(access_token.sub)
begin
validate_access_token!
reg_key = access_token.reg_key
reg_secret = RegistrationRequestService.retrieve_registration_password(context, reg_key) if reg_key
render_new_tool_proxy(context, reg_key, dev_key) and return if reg_secret.present?
render_new_tool_proxy(context, reg_key, developer_key) and return if reg_secret.present?
rescue Lti::Oauth2::InvalidTokenError
render_unauthorized and return
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/lti/message_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def registration

@lti_launch.params = message.post_params
if @context.root_account.feature_enabled?(:lti_2_auth_url_registration)
@lti_launch.params['oauth2_access_token_url'] = lti_oauth2_authorize_url
@lti_launch.params['oauth2_access_token_url'] = polymorphic_url([@context, :lti_oauth2_authorize])
end
@lti_launch.params['ext_tool_consumer_instance_guid'] = @context.root_account.lti_guid
@lti_launch.params['ext_api_domain'] = HostUrl.context_host(@context, request.host)
Expand Down
14 changes: 14 additions & 0 deletions app/models/lti/tool_consumer_profile_creator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ def create
profile.service_offered = services
profile.capability_offered = capabilities
profile.guid = (tool_consumer_profile && tool_consumer_profile.uuid) || Lti::ToolConsumerProfile::DEFAULT_TCP_UUID
profile.security_profile = security_profiles

# TODO: Extract this
if @root_account.feature_enabled?(:lti2_rereg)
Expand Down Expand Up @@ -93,5 +94,18 @@ def services
end
end

def security_profiles
[
IMS::LTI::Models::SecurityProfile.new(
security_profile_name: 'lti_oauth_hash_message_security',
digest_algorithm: 'HMAC-SHA1'
),
IMS::LTI::Models::SecurityProfile.new(
security_profile_name: 'oauth2_access_token_ws_security',
digest_algorithm: 'HS256'
)
]
end

end
end
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1947,7 +1947,6 @@
end

ApiRouteSet.draw(self, "/api/lti") do
post "authorize", controller: 'lti/ims/authorization', action: :authorize, as: 'lti_oauth2_authorize'

scope(controller: 'lti/subscriptions_api') do
post "subscriptions", action: :create
Expand All @@ -1959,6 +1958,7 @@

%w(course account).each do |context|
prefix = "#{context}s/:#{context}_id"
post "#{prefix}/authorize", controller: 'lti/ims/authorization', action: :authorize, as: "#{context}_lti_oauth2_authorize"
get "#{prefix}/tool_consumer_profile(/:tool_consumer_profile_id)", controller: 'lti/ims/tool_consumer_profile',
action: 'show', as: "#{context}_tool_consumer_profile"
post "#{prefix}/tool_proxy", controller: 'lti/ims/tool_proxy', action: :re_reg,
Expand Down
9 changes: 6 additions & 3 deletions lib/lti/oauth2/authorization_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ class InvalidAuthJwt < StandardError
class MissingAuthorizationCode < StandardError
end

def initialize(jwt:, authorization_url:, code: nil)
def initialize(jwt:, authorization_url:, code: nil, context:)
@raw_jwt = jwt
@authorization_url = authorization_url
@code = code
@context = context
end

def jwt
Expand Down Expand Up @@ -65,13 +66,15 @@ def developer_key
end

def sub
tool_proxy&.guid || developer_key&.global_id
tool_proxy&.guid || developer_key&.global_id || unverified_jwt[:sub]
end

private

def jwt_secret
secret = tool_proxy&.shared_secret || developer_key&.api_key
secret = tool_proxy&.shared_secret
secret ||= developer_key&.api_key
secret ||= RegistrationRequestService.retrieve_registration_password(@context, unverified_jwt[:sub])
return secret if secret.present?
raise SecretNotFound, "either the tool proxy or developer key were not found"
end
Expand Down
2 changes: 1 addition & 1 deletion spec/apis/lti/ims/access_token_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def lti2_service_name
expect(response.code).to eq '401'
end

it 'requires an active developer key' do
it 'requires the developer key to be active' do
@request.headers.merge!(request_headers)
developer_key.deactivate
get :index, format: :json
Expand Down
53 changes: 46 additions & 7 deletions spec/apis/lti/ims/authorization_api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ module Lti
module Ims
describe AuthorizationController, type: :request do

let(:account) { Account.new }
let(:account) { Account.create! }

let (:developer_key) { DeveloperKey.create!(redirect_uri: 'http://example.com/redirect') }
let(:developer_key) { DeveloperKey.create!(redirect_uri: 'http://example.com/redirect') }

let(:product_family) do
ProductFamily.create(
Expand Down Expand Up @@ -36,7 +36,7 @@ module Ims
raw_jwt = JSON::JWT.new(
{
sub: tool_proxy.guid,
aud: lti_oauth2_authorize_url,
aud: polymorphic_url([account, :lti_oauth2_authorize]),
exp: 1.minute.from_now,
iat: Time.zone.now.to_i,
jti: SecureRandom.uuid
Expand All @@ -45,7 +45,7 @@ module Ims
raw_jwt
end

let(:auth_endpoint) { '/api/lti/authorize' }
let(:auth_endpoint) { polymorphic_url([account, :lti_oauth2_authorize]) }
let(:jwt_string) do
raw_jwt.sign(tool_proxy.shared_secret, :HS256).to_s
end
Expand Down Expand Up @@ -77,7 +77,7 @@ module Ims
it 'returns an access_token' do
post auth_endpoint, params
access_token = Lti::Oauth2::AccessToken.create_jwt(aud: @request.host, sub: tool_proxy.guid)
expect{access_token.validate!}.not_to raise_error
expect { access_token.validate! }.not_to raise_error
end

it "allows the use of the 'OAuth.splitSecret'" do
Expand Down Expand Up @@ -107,13 +107,52 @@ module Ims
expect(jwt["aud"]).to match_array [request.host, request.protocol + file_host]
end

context "reg_key" do

let(:reg_key) { SecureRandom.uuid }

let(:reg_password) { SecureRandom.uuid }

let(:raw_jwt) do
RegistrationRequestService.cache_registration(account, reg_key, reg_password)
raw_jwt = JSON::JWT.new(
{
sub: reg_key,
aud: polymorphic_url([account, :lti_oauth2_authorize]),
exp: 1.minute.from_now,
iat: Time.zone.now.to_i,
jti: SecureRandom.uuid,
}
)
raw_jwt
end

let(:jwt_string) do
raw_jwt.sign(reg_password, :HS256).to_s
end

let(:params) do
{
grant_type: 'authorization_code',
assertion: jwt_string,
}
end

it 'accepts a valid reg_key' do
enable_cache do
post auth_endpoint, params
expect(response.code).to eq '200'
end
end
end

context "developer credentials" do

let(:raw_jwt) do
raw_jwt = JSON::JWT.new(
{
sub: developer_key.global_id,
aud: lti_oauth2_authorize_url,
aud: polymorphic_url([account, :lti_oauth2_authorize]),
exp: 1.minute.from_now,
iat: Time.zone.now.to_i,
jti: SecureRandom.uuid,
Expand All @@ -134,7 +173,7 @@ module Ims
}
end

it "rejects the request if a reg_key isn't provided and grant_type is auth code" do
it "rejects the request if a valid reg_key isn't provided and grant_type is auth code" do
post auth_endpoint, params.delete(:code)
expect(response.code).to eq '400'
end
Expand Down
44 changes: 0 additions & 44 deletions spec/apis/lti/subscriptions_api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,6 @@ module Lti
expect(response).to be_success
end

it 'checks that the tool proxy has an active developer key' do
product_family.update_attributes(developer_key: nil)
tool_proxy[:raw_data]['enabled_capability'] = %w(vnd.instructure.webhooks.assignment.attachment_created)
tool_proxy.save!
post create_endpoint, { subscription: subscription }, request_headers
expect(response).to be_unauthorized
end

it 'checks that the tool proxy has the correct enabled capabilities' do
post create_endpoint, { subscription: subscription }, request_headers
expect(response).to be_unauthorized
Expand Down Expand Up @@ -124,15 +116,6 @@ module Lti
expect(response).not_to be_success
end

it 'checks that the tool proxy has an active developer key' do
product_family.update_attributes(developer_key: nil)
allow(subscription_service).to receive_messages(tool_proxy_subscription: ok_response)
tool_proxy[:raw_data]['enabled_capability'] = %w(vnd.instructure.webhooks.assignment.attachment_created)
tool_proxy.save!
delete delete_endpoint, {}, request_headers
expect(response).to be_unauthorized
end

it 'requires JWT Access token' do
delete delete_endpoint, {}
expect(response).to be_unauthorized
Expand Down Expand Up @@ -171,15 +154,6 @@ module Lti
expect(response).not_to be_success
end

it 'checks that the tool proxy has an active developer key' do
product_family.update_attributes(developer_key: nil)
allow(subscription_service).to receive_messages(tool_proxy_subscription: ok_response)
tool_proxy[:raw_data]['enabled_capability'] = %w(vnd.instructure.webhooks.assignment.attachment_created)
tool_proxy.save!
get show_endpoint, {}, request_headers
expect(response).to be_unauthorized
end

it 'requires JWT Access token' do
get show_endpoint, {}
expect(response).to be_unauthorized
Expand Down Expand Up @@ -248,15 +222,6 @@ module Lti
expect(JSON.parse(response.body)['error']).to eq 'Unauthorized subscription'
end

it 'checks that the tool proxy has an active developer key' do
product_family.update_attributes(developer_key: nil)
allow(subscription_service).to receive_messages(update_tool_proxy_subscription: ok_response)
tool_proxy[:raw_data]['enabled_capability'] = %w(vnd.instructure.webhooks.assignment.attachment_created)
tool_proxy.save!
put update_endpoint, {subscription: subscription}, request_headers
expect(response).to be_unauthorized
end

it 'requires JWT Access token' do
put update_endpoint, {subscription: subscription}
expect(response).to be_unauthorized
Expand Down Expand Up @@ -288,15 +253,6 @@ module Lti
expect(response).to be_success
end

it 'checks that the tool proxy has an active developer key' do
product_family.update_attributes(developer_key: nil)
allow(subscription_service).to receive_messages(tool_proxy_subscription: ok_response)
tool_proxy[:raw_data]['enabled_capability'] = %w(vnd.instructure.webhooks.assignment.attachment_created)
tool_proxy.save!
get index_endpoint, {}, request_headers
expect(response).to be_unauthorized
end

it 'requires JWT Access token' do
get index_endpoint, {}
expect(response).to be_unauthorized
Expand Down
4 changes: 3 additions & 1 deletion spec/controllers/lti/message_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ def wrap_params(params)
get 'registration', course_id: @course.id, tool_consumer_url: 'http://tool.consumer.url'
lti_launch = assigns[:lti_launch]
launch_params = lti_launch.params
expect(launch_params['oauth2_access_token_url']).to eq lti_oauth2_authorize_url
expect(launch_params['oauth2_access_token_url']).to(
eq controller.polymorphic_url([@course, :lti_oauth2_authorize])
)
end

end
Expand Down
Loading

0 comments on commit 236fc45

Please sign in to comment.