ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Share LTI 1.3 role map between NRPS and launch 

- For the `$Membership.role` and `$com.Instructure.membership.roles`
  custom params, calculate role URNs using
  `LIS_V2_LTI_ADVANTAGE_ROLE_MAP` for 1.3 tools, else
  `LIS_V2_ROLE_MAP`.
  - This way the `https://purl.imsglobal.org/spec/lti/claim/roles`
  launch claim matches up with the NRPS `roles` field _except_ that
  NRPS `roles` doesn't include system nor institution roles
  (N+1 issues).
  - Really the only meaningful difference is the TA role URN, where
  `LIS_V2_LTI_ADVANTAGE_ROLE_MAP` corrects a typo in the
  `LIS_V2_ROLE_MAP`. See `SubstitutionsHelper` line ~61 for all the
  differences.
  - Since the contents of `LIS_V2_LTI_ADVANTAGE_ROLE_MAP` changed
  to include additional roles to support launches but which should not
  be used as NRPS role filters, `MembershipsProvider.lis_roles` also
  needed to change to filter those roles out. So took the opportunity
  to rename that method to `queryable_roles` to avoid confusion with
  `GroupMembershipDecorator.lti_roles` and
  `CourseEnrollmentsDecorator.lti_roles` methods that perform the
  mapping in the _opposite_ direction.

Closes LTIA-42

Test Plan

  - Place a 1.3 tool in a `Course` with several active members, at
  least one of which is in a TA role.
  - Ensure the tool's configuration includes mappings for the
  `$Membership.role` and `$com.Instructure.membership.roles` custom
  params.
  - Launch the tool for several members and verify correct contents
  of:
    - `https://purl.imsglobal.org/spec/lti/claim/roles`
    - `https://purl.imsglobal.org/spec/lti/claim/custom['com_instructure_membership_roles']`
    - `https://purl.imsglobal.org/spec/lti/claim/custom['membership_role']`
  In particular, the first two should match and for a TA all three
  should include
  `http://purl.imsglobal.org/vocab/lis/v2/membership/Instructor#TeachingAssistant`
  (capital 'I').
  - Invoke NRPS for this tool and `Course`. For each `member`, the
  `roles` field should match the first two launch fields listed
  above _except_ that system and institution roles are _not_ listed.
  The `com_instructure_membership_roles` and `membership_role` custom
  params should render unexpanded, i.e. literally as
  `$com.Instructure.membership.roles` and `$Membership.role`.

Change-Id: I315220edd0b5500934ede9a82047cc0206fbd8f5
Reviewed-on: https://gerrit.instructure.com/169626
Tested-by: Jenkins
Reviewed-by: Marc Phillips <mphillips@instructure.com>
QA-Review: Bill Smith <bsmith@instructure.com>
Product-Review: Karl Lloyd <karl@instructure.com>
This commit is contained in:
Dan McCallum 2018-10-24 10:32:40 -07:00 committed by Marc Phillips
parent 811d30a96f
commit f89d9ee839
9 changed files with 145 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
app/controllers/lti/ims/providers/course_memberships_provider.rb
View File

@ -58,31 +58,31 @@ module Lti::Ims::Providers
else
# Non-active students get an active ('submitted') Submission, so join on base_users_scope to narrow down
# Submissions to only active students.
students_scope = base_users_scope.where(enrollments: {type: student_lti_roles})
students_scope = base_users_scope.where(enrollments: {type: student_queryable_roles})
narrowed_students_scope = students_scope.where(correlated_assignment_submissions('users.id').exists)
# If we only care about students, this scope is sufficient and can avoid the ugly union down below
return narrowed_students_scope if filter_non_students?
non_students_scope = apply_role_filter(base_users_scope.where.not(enrollments: {type: student_lti_roles}))
non_students_scope = apply_role_filter(base_users_scope.where.not(enrollments: {type: student_queryable_roles}))
non_students_scope.union(narrowed_students_scope).distinct.order(:id).select(:id)
end
end
def student_lti_roles
Lti::SubstitutionsHelper::INVERTED_LIS_ADVANTAGE_ROLE_MAP['http://purl.imsglobal.org/vocab/lis/v2/membership#Learner']
def student_queryable_roles
queryable_roles('http://purl.imsglobal.org/vocab/lis/v2/membership#Learner')
end
def filter_students?
role? && (student_lti_roles != lti_roles)
role? && (student_queryable_roles != queryable_roles(role))
end
def filter_non_students?
role? && (student_lti_roles == lti_roles)
role? && (student_queryable_roles == queryable_roles(role))
end
def apply_role_filter(scope)
return scope unless role?
enrollment_types = lti_roles
enrollment_types = queryable_roles(role)
enrollment_types.present? ? scope.where(enrollments: { type: enrollment_types }) : scope.none
end
@ -123,7 +123,7 @@ module Lti::Ims::Providers
end
def lti_roles
@_lti_roles ||= enrollments.map { |e| Lti::SubstitutionsHelper::LIS_ADVANTAGE_ROLE_MAP[e.class] }.compact.flatten.uniq
@_lti_roles ||= enrollments.map { |e| Lti::SubstitutionsHelper::LIS_V2_LTI_ADVANTAGE_ROLE_MAP[e.class] }.compact.flatten.uniq
end
end

6
app/controllers/lti/ims/providers/group_memberships_provider.rb
View File

@ -47,7 +47,7 @@ module Lti::Ims::Providers
def apply_role_filter(scope)
return scope unless role?
enrollment_types = lti_roles
enrollment_types = queryable_roles(role)
if enrollment_types.present? && group_role?(enrollment_types)
enrollment_types == [:group_leader] ? scope.where(user: context.leader_id) : scope
else scope.none
@ -95,11 +95,11 @@ module Lti::Ims::Providers
private
def group_leader_role_urns
Lti::SubstitutionsHelper::LIS_ADVANTAGE_ROLE_MAP[:group_leader]
Lti::SubstitutionsHelper::LIS_V2_LTI_ADVANTAGE_ROLE_MAP[:group_leader]
end
def group_member_role_urns
Lti::SubstitutionsHelper::LIS_ADVANTAGE_ROLE_MAP[:group_member]
Lti::SubstitutionsHelper::LIS_V2_LTI_ADVANTAGE_ROLE_MAP[:group_member]
end
end

8
app/controllers/lti/ims/providers/memberships_provider.rb
View File

@ -103,12 +103,14 @@ module Lti::Ims::Providers
role.present?
end
def lti_roles
Lti::SubstitutionsHelper::INVERTED_LIS_ADVANTAGE_ROLE_MAP[role]
def queryable_roles(lti_role)
# Roles represented as Strings are for system and institution roles, which we do not care about for
# purposes of NRPS filtering by role
Lti::SubstitutionsHelper::INVERTED_LIS_V2_LTI_ADVANTAGE_ROLE_MAP[lti_role]&.reject { |r| r.is_a?(String) }
end
def nonsense_role_filter?
role? && lti_roles.blank?
role? && queryable_roles(role).blank?
end
def assignment

1
doc/api/tools_variable_substitutions.md
View File

@ -710,6 +710,7 @@ returns the context ids for the groups the user belongs to in the course.
```
## Membership.role
Returns the [IMS LTI membership service](https://www.imsglobal.org/specs/ltimemv1p0/specification-3) roles for filtering via query parameters.
Or, for LTI 1.3 tools, returns the [IMS LTI Names and Role Provisioning Service](https://www.imsglobal.org/spec/lti-nrps/v2p0) roles for filtering via query parameters.
**Availability**: *when launched by a logged in user*
**Launch Parameter**: *roles*

47
lib/lti/substitutions_helper.rb
View File

@ -58,7 +58,19 @@ module Lti
LIS_V2_ROLE_NONE = 'http://purl.imsglobal.org/vocab/lis/v2/person#None'
LIS_ADVANTAGE_ROLE_MAP = {
# Nearly identical to LIS_V2_ROLE_MAP except:
# 1. Corrects typo in first TaEnrollment URI ('instructor'->'Instructor')
# 2. Values uniformly (frozen) Arrays
# 3. Has Group roles
# 4. Has no Course role
LIS_V2_LTI_ADVANTAGE_ROLE_MAP = {
'user' => [ 'http://purl.imsglobal.org/vocab/lis/v2/system/person#User' ].freeze,
'siteadmin' => [ 'http://purl.imsglobal.org/vocab/lis/v2/system/person#SysAdmin' ].freeze,
'teacher' => [ 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Instructor' ].freeze,
'student' => [ 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Student' ].freeze,
'admin' => [ 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Administrator' ].freeze,
AccountUser => [ 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Administrator' ].freeze,
TaEnrollment => [
'http://purl.imsglobal.org/vocab/lis/v2/membership/Instructor#TeachingAssistant',
'http://purl.imsglobal.org/vocab/lis/v2/membership#Instructor'
@ -67,6 +79,7 @@ module Lti
TeacherEnrollment => [ 'http://purl.imsglobal.org/vocab/lis/v2/membership#Instructor' ].freeze,
DesignerEnrollment => [ 'http://purl.imsglobal.org/vocab/lis/v2/membership#ContentDeveloper' ].freeze,
ObserverEnrollment => [ 'http://purl.imsglobal.org/vocab/lis/v2/membership#Mentor' ].freeze,
StudentViewEnrollment => [ 'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner' ].freeze,
:group_member => [ 'http://purl.imsglobal.org/vocab/lis/v2/membership#Member' ].freeze,
:group_leader => [
'http://purl.imsglobal.org/vocab/lis/v2/membership#Member',
@ -74,19 +87,21 @@ module Lti
].freeze
}.freeze
# Inversion of LIS_ADVANTAGE_ROLE_MAP, i.e.:
# Inversion of LIS_V2_LTI_ADVANTAGE_ROLE_MAP, i.e.:
#
# {
# '<lis-url>' => [<enrollment-class>, <enrollment-class>],
# '<lis-url>' => [<enrollment-class>, <logical-sys-or-insitution-role-name-string>, <enrollment-class>],
# '<lis-url>' => [<group-membership-type-symbol>, <group-membership-type-symbol>],
# ...
# }
#
# (Extra copy at the end is to undo the default value ([]))
INVERTED_LIS_ADVANTAGE_ROLE_MAP = LIS_ADVANTAGE_ROLE_MAP.each_with_object(Hash.new([])) do |(key,values), memo|
INVERTED_LIS_V2_LTI_ADVANTAGE_ROLE_MAP = LIS_V2_LTI_ADVANTAGE_ROLE_MAP.each_with_object(Hash.new([])) do |(key,values), memo|
values.each { |value| memo[value] += [key] }
end.reverse_merge({}).freeze
LIS_V2_LTI_ADVANTAGE_ROLE_NONE = 'http://purl.imsglobal.org/vocab/lis/v2/system/person#None'.freeze
def initialize(context, root_account, user, tool = nil)
@context = context
@root_account = root_account
@ -112,20 +127,23 @@ module Lti
def all_roles(version = 'lis1')
case version
when 'lis2'
role_map = LIS_V2_ROLE_MAP
role_none = LIS_V2_ROLE_NONE
else
role_map = LIS_ROLE_MAP
role_none = LtiOutbound::LTIRoles::System::NONE
when 'lis2'
role_map = LIS_V2_ROLE_MAP
role_none = LIS_V2_ROLE_NONE
when 'lti1_3'
role_map = LIS_V2_LTI_ADVANTAGE_ROLE_MAP
role_none = LIS_V2_LTI_ADVANTAGE_ROLE_NONE
else
role_map = LIS_ROLE_MAP
role_none = LtiOutbound::LTIRoles::System::NONE
end
if @user
context_roles = course_enrollments.each_with_object(Set.new) { |role, set| set.add([*role_map[role.class]].join(",")) }
institution_roles = @user.roles(@root_account, true).map { |role| role_map[role] }
institution_roles = @user.roles(@root_account, true).flat_map { |role| role_map[role] }
if Account.site_admin.account_users_for(@user).present?
institution_roles << role_map['siteadmin']
institution_roles.push(*role_map['siteadmin'])
end
(context_roles + institution_roles).to_a.compact.uniq.sort.join(',')
else
@ -173,9 +191,10 @@ module Lti
roles.join(',')
end
def current_canvas_roles_lis_v2
def current_canvas_roles_lis_v2(version = 'lis2')
roles = (course_enrollments + account_enrollments).map(&:class).uniq
roles.map { |r| LIS_V2_ROLE_MAP[r] }.join(',')
role_map = version == 'lti1_3' ? LIS_V2_LTI_ADVANTAGE_ROLE_MAP : LIS_V2_ROLE_MAP
roles.map { |r| role_map[r] }.join(',')
end
def enrollment_state

9
lib/lti/variable_expander.rb
View File

@ -572,7 +572,7 @@ module Lti
# http://purl.imsglobal.org/vocab/lis/v2/institution/person#Student
# ```
register_expansion 'com.Instructure.membership.roles', [],
-> { lti_helper.current_canvas_roles_lis_v2 },
-> { lti_helper.current_canvas_roles_lis_v2(lti_1_3? ? 'lti1_3' : 'lis2')},
ROLES_GUARD,
default_name: 'com_instructure_membership_roles'
@ -784,13 +784,14 @@ module Lti
-> { @current_user && @context.is_a?(Course) }
# Returns the [IMS LTI membership service](https://www.imsglobal.org/specs/ltimemv1p0/specification-3) roles for filtering via query parameters.
# Or, for LTI 1.3 tools, returns the [IMS LTI Names and Role Provisioning Service](https://www.imsglobal.org/spec/lti-nrps/v2p0) roles for filtering via query parameters.
# @launch_parameter roles
# @example
# ```
# http://purl.imsglobal.org/vocab/lis/v2/institution/person#Administrator
# ```
register_expansion 'Membership.role', [],
-> { lti_helper.all_roles('lis2') },
-> { lti_helper.all_roles(lti_1_3? ? 'lti1_3' : 'lis2') },
USER_GUARD,
default_name: 'roles'
@ -1237,5 +1238,9 @@ module Lti
v.gsub(substring, (self[match] || substring).to_s)
end
end
def lti_1_3?
@tool && @tool.respond_to?(:use_1_3?) && @tool.use_1_3?
end
end
end

2
spec/controllers/lti/message_controller_spec.rb
View File

@ -497,7 +497,7 @@ module Lti
tool_proxy.save!
get 'basic_lti_launch_request', params: {account_id: account.id, message_handler_id: message_handler.id,
params: { tool_launch_context: 'my_custom_context' }}
params = assigns[:lti_launch].params.with_indifferent_access
params = assigns[:lti_launch].params.with_indifferent_access
expect(params['oauth_callback']).to eq 'about:blank'
end

61
spec/lib/lti/substitutions_helper_spec.rb
View File

@ -155,6 +155,35 @@ module Lti
expect(roles.split(',')).to match_array expected_roles
end
it 'converts multiple roles for lti 1.3' do
allow(subject).to receive(:course_enrollments).and_return([StudentEnrollment.new, TeacherEnrollment.new, DesignerEnrollment.new, ObserverEnrollment.new, TaEnrollment.new, AccountUser.new])
allow(user).to receive(:roles).and_return(['user', 'student', 'teacher', 'admin'])
roles = subject.all_roles('lti1_3')
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/system/person#User'
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Student'
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Instructor'
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Administrator'
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner'
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/membership#Instructor'
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/membership#ContentDeveloper'
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/membership#Mentor'
expect(roles).to include 'http://purl.imsglobal.org/vocab/lis/v2/membership/Instructor#TeachingAssistant' # only difference btwn lis2 and lti1_3 modes
end
it "returns none if no user for lti 1.3" do
helper = SubstitutionsHelper.new(course, root_account, nil)
expect(helper.all_roles('lti1_3')).to eq 'http://purl.imsglobal.org/vocab/lis/v2/system/person#None'
end
it "includes main and subrole for TeachingAssistant for lti 1.3" do
allow(subject).to receive(:course_enrollments).and_return([TaEnrollment.new])
roles = subject.all_roles('lti1_3')
expected_roles = ["http://purl.imsglobal.org/vocab/lis/v2/membership/Instructor#TeachingAssistant", # only difference btwn lis2 and lti1_3 modes
"http://purl.imsglobal.org/vocab/lis/v2/membership#Instructor",
"http://purl.imsglobal.org/vocab/lis/v2/system/person#User"]
expect(roles.split(',')).to match_array expected_roles
end
it 'does not include admin role if user has a sub-account admin user record in deleted account' do
sub_account = account.sub_accounts.create!
sub_account.account_users.create!(user: user, role: admin_role)
@ -312,6 +341,38 @@ module Lti
expect(roles).to eq 'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner'
end
it 'can be directed to use the LIS 2 role map' do
set_up_persistance!
ta_in_course(user: user, course: course, active_enrollment: true)
course_with_designer(user: user, course: course, active_enrollment: true)
account_admin_user(user: user, account: account)
roles = subject.current_canvas_roles_lis_v2('lis2')
expect(roles.split(',')).to match_array [
"http://purl.imsglobal.org/vocab/lis/v2/membership#ContentDeveloper",
"http://purl.imsglobal.org/vocab/lis/v2/membership/instructor#TeachingAssistant", # only difference btwn lis2 and lti1_3 modes
"http://purl.imsglobal.org/vocab/lis/v2/membership#Instructor",
"http://purl.imsglobal.org/vocab/lis/v2/institution/person#Administrator"
]
end
it 'can be directed to use the LTI 1.3 role map' do
set_up_persistance!
ta_in_course(user: user, course: course, active_enrollment: true)
course_with_designer(user: user, course: course, active_enrollment: true)
account_admin_user(user: user, account: account)
roles = subject.current_canvas_roles_lis_v2('lti1_3')
expect(roles.split(',')).to match_array [
"http://purl.imsglobal.org/vocab/lis/v2/membership#ContentDeveloper",
"http://purl.imsglobal.org/vocab/lis/v2/membership/Instructor#TeachingAssistant", # only difference btwn lis2 and lti1_3 modes
"http://purl.imsglobal.org/vocab/lis/v2/membership#Instructor",
"http://purl.imsglobal.org/vocab/lis/v2/institution/person#Administrator"
]
end
end
describe '#current_canvas_roles' do

31
spec/lib/lti/variable_expander_spec.rb
View File

@ -49,6 +49,7 @@ module Lti
allow(shard_mock).to receive(:settings).and_return({encription_key: 'abc'})
allow(m).to receive(:shard).and_return(shard_mock)
allow(m).to receive(:opaque_identifier_for).and_return("6cd2e0d65bd5aef3b5ee56a64bdcd595e447bc8f")
allow(m).to receive(:use_1_3?).and_return(false)
m
end
let(:controller) do
@ -647,7 +648,7 @@ module Lti
end
context 'context is a group' do
let(:variable_expander) { VariableExpander.new(root_account, group, controller, current_user: user) }
let(:variable_expander) { VariableExpander.new(root_account, group, controller, current_user: user, tool: tool) }
it 'has substitution for $ToolProxyBinding.memberships.url when context is a group' do
exp_hash = { test: '$ToolProxyBinding.memberships.url' }
@ -768,7 +769,7 @@ module Lti
end
context 'context is a course' do
let(:variable_expander) { VariableExpander.new(root_account, course, controller, current_user: user) }
let(:variable_expander) { VariableExpander.new(root_account, course, controller, current_user: user, tool: tool) }
it 'has substitution for $ToolProxyBinding.memberships.url when context is a course' do
exp_hash = { test: '$ToolProxyBinding.memberships.url' }
@ -845,7 +846,18 @@ module Lti
it 'has substitution for $com.Instructure.membership.roles' do
allow(Lti::SubstitutionsHelper).to receive(:new).and_return(substitution_helper)
allow(substitution_helper).to receive(:current_canvas_roles_lis_v2).and_return(
allow(substitution_helper).to receive(:current_canvas_roles_lis_v2).with('lis2').and_return(
'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Student'
)
exp_hash = {test: '$com.Instructure.membership.roles'}
variable_expander.expand_variables!(exp_hash)
expect(exp_hash[:test]).to eq 'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Student'
end
it 'has substitution for foo' do
allow(tool).to receive(:use_1_3?).and_return(true)
allow(Lti::SubstitutionsHelper).to receive(:new).and_return(substitution_helper)
allow(substitution_helper).to receive(:current_canvas_roles_lis_v2).with('lti1_3').and_return(
'http://purl.imsglobal.org/vocab/lis/v2/institution/person#Student'
)
exp_hash = {test: '$com.Instructure.membership.roles'}
@ -1061,7 +1073,7 @@ module Lti
end
context 'context is a course with an assignment' do
let(:variable_expander) { VariableExpander.new(root_account, course, controller, collaboration: collaboration) }
let(:variable_expander) { VariableExpander.new(root_account, course, controller, tool: tool, collaboration: collaboration) }
it 'has substitution for $Canvas.api.collaborationMembers.url' do
allow(collaboration).to receive(:id).and_return(1)
@ -1073,7 +1085,7 @@ module Lti
end
context 'context is a course with an assignment' do
let(:variable_expander) { VariableExpander.new(root_account, course, controller, current_user: user, assignment: assignment) }
let(:variable_expander) { VariableExpander.new(root_account, course, controller, current_user: user, tool: tool, assignment: assignment) }
it 'has substitution for $Canvas.assignment.id' do
allow(assignment).to receive(:id).and_return(2015)
@ -1324,6 +1336,15 @@ module Lti
expect(exp_hash[:test]).to eq 'Admin,User'
end
it 'has substitution for $Membership.role in LTI 1.3 mode' do
allow(tool).to receive(:use_1_3?).and_return(true)
allow(substitution_helper).to receive(:all_roles).with('lti1_3').and_return('Admin,User')
allow(Lti::SubstitutionsHelper).to receive(:new).and_return(substitution_helper)
exp_hash = {test: '$Membership.role'}
variable_expander.expand_variables!(exp_hash)
expect(exp_hash[:test]).to eq 'Admin,User'
end
it 'has substitution for $User.id' do
allow(user).to receive(:id).and_return(456)
exp_hash = {test: '$User.id'}