-
Notifications
You must be signed in to change notification settings - Fork 99
[BD-24] [TNL-7330] [BB-2726] LTI Improvements - Implement Context Membership Service #124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| from .key_handlers import ToolKeyHandler, PlatformKeyHandler | ||
| from .ags import LtiAgs | ||
| from .deep_linking import LtiDeepLinking | ||
| from .nprs import LtiNrps | ||
|
|
||
|
|
||
| class LtiConsumer1p3: | ||
|
|
@@ -476,6 +477,9 @@ def __init__(self, *args, **kwargs): | |
| self.ags = None | ||
| self.dl = None | ||
|
|
||
| # LTI NRPS Variables | ||
| self.nrps = None | ||
|
|
||
| @property | ||
| def lti_ags(self): | ||
| """ | ||
|
|
@@ -488,6 +492,18 @@ def lti_ags(self): | |
|
|
||
| return self.ags | ||
|
|
||
| @property | ||
| def lti_nrps(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unusual to have a property just to raise an exception if the attribute is None. Does it simplify much code elsewhere? And the attribute is "nrps", so not private to the class?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| """ | ||
| Returns LTI NRPS class or throw exception if not set up. | ||
| """ | ||
| if not self.nrps: | ||
| raise exceptions.LtiNrpsServiceNotSetUp( | ||
| "The LTI NRPS service was not set up for this consumer." | ||
| ) | ||
|
|
||
| return self.nrps | ||
|
|
||
| def enable_ags( | ||
| self, | ||
| lineitems_url, | ||
|
|
@@ -623,3 +639,16 @@ def set_dl_content_launch_parameters( | |
|
|
||
| if custom: | ||
| self.set_custom_parameters(custom) | ||
|
|
||
| def enable_nrps(self, context_memberships_url): | ||
| """ | ||
| Enable LTI Names and Role Provisioning Service. | ||
|
|
||
| This will include the LTI NRPS Claim in the LTI message | ||
| and set up the required class. | ||
| """ | ||
|
|
||
| self.nrps = LtiNrps(context_memberships_url) | ||
|
|
||
| # Include LTI NRPS claim inside the LTI Launch message | ||
| self.set_extra_claim(self.nrps.get_lti_nrps_launch_claim()) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,36 @@ | |
| from rest_framework import permissions | ||
|
|
||
|
|
||
| class LtiAgsPermissions(permissions.BasePermission): | ||
| class LTIBasePermissions(permissions.BasePermission): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neat 😄
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/LTI/Lti/ :) |
||
| """ | ||
| Base LTI Permissions. | ||
|
|
||
| This checks if the token included in the request | ||
| has the allowed scopes. Allowed scopes should be | ||
| returned by ``get_permission_scopes`` method, which | ||
| should be implemented by child classes. | ||
| """ | ||
| def has_permission(self, request, view): | ||
| # Retrieves token from request, which was already checked by | ||
| # the Authentication class, so we assume it's a sane value. | ||
| auth_token = request.headers['Authorization'].split()[1] | ||
|
|
||
| scopes = self.get_permission_scopes(request, view) | ||
|
|
||
| if scopes: | ||
| return request.lti_consumer.check_token(auth_token, scopes) | ||
|
|
||
| return False | ||
|
|
||
| def get_permission_scopes(self, request, view): | ||
| """ | ||
| This method should be overriden by child classes to return | ||
| a list of allowed scopes. | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
|
|
||
| class LtiAgsPermissions(LTIBasePermissions): | ||
| """ | ||
| LTI AGS Permissions. | ||
|
|
||
|
|
@@ -19,14 +48,11 @@ class LtiAgsPermissions(permissions.BasePermission): | |
| Results: Not implemented yet. | ||
| Score: Not implemented yet. | ||
| """ | ||
| def has_permission(self, request, view): | ||
|
|
||
| def get_permission_scopes(self, request, view): | ||
| """ | ||
| Check if LTI AGS permissions are set in auth token. | ||
| Return LTI AGS allowed scopes. | ||
| """ | ||
| # Retrieves token from request, which was already checked by | ||
| # the Authentication class, so we assume it's a sane value. | ||
| auth_token = request.headers['Authorization'].split()[1] | ||
|
|
||
| scopes = [] | ||
| if view.action in ['list', 'retrieve']: | ||
| # We don't need to wrap this around a try-catch because | ||
|
|
@@ -48,7 +74,28 @@ def has_permission(self, request, view): | |
| 'https://purl.imsglobal.org/spec/lti-ags/scope/score', | ||
| ] | ||
|
|
||
| if scopes: | ||
| return request.lti_consumer.check_token(auth_token, scopes) | ||
| return scopes | ||
|
|
||
| return False | ||
|
|
||
| class LtiNrpsContextMembershipsPermissions(LTIBasePermissions): | ||
| """ | ||
| LTI NRPS Context Memberships Permissions. | ||
|
|
||
| This checks if the token included in the request has the allowed scopes to read/write | ||
| the LTI NRPS Context Memberships Service. | ||
|
|
||
| Context Membership scopes: https://www.imsglobal.org/spec/lti-nrps/v2p0#scope-and-service-security | ||
| """ | ||
|
|
||
| def get_permission_scopes(self, request, view): | ||
| """ | ||
| Return LTI NRPS Context Memberships allowed scopes. | ||
| """ | ||
| scopes = [] | ||
|
|
||
| if view.action == 'list': | ||
| scopes = [ | ||
| 'https://purl.imsglobal.org/spec/lti-nrps/scope/contextmembership.readonly' | ||
| ] | ||
|
|
||
| return scopes | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| """ | ||
| LTI Names and Role Provisioning Service implementation | ||
| """ | ||
|
|
||
|
|
||
| class LtiNrps: | ||
| """ | ||
| LTI NRPS Consumer | ||
|
|
||
| Implements Names and Role Provisioning Services and ties | ||
| them in with the LTI Consumer. | ||
|
|
||
| Available services: | ||
| * Context Membership Service | ||
|
|
||
| Reference: https://www.imsglobal.org/spec/lti-nrps/v2p0#overview | ||
| """ | ||
| def __init__( | ||
| self, | ||
| context_memberships_url, | ||
| ): | ||
| self.context_memberships_url = context_memberships_url | ||
|
|
||
| def get_available_scopes(self): | ||
| """ | ||
| Retrieves list of available token scopes in this instance. | ||
| """ | ||
|
|
||
| return [ | ||
| 'https://purl.imsglobal.org/spec/lti-nrps/scope/contextmembership.readonly' | ||
| ] | ||
|
|
||
| def get_lti_nrps_launch_claim(self): | ||
| """ | ||
| Returns LTI NRPS Claim to be injected in the LTI launch message. | ||
| """ | ||
|
|
||
| return { | ||
| "https://purl.imsglobal.org/spec/lti-nrps/claim/namesroleservice": { | ||
| "context_memberships_url": self.context_memberships_url, | ||
| "service_versions": ["2.0"] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
|
|
||
| from lti_consumer.lti_1p3 import exceptions | ||
| from lti_consumer.lti_1p3.ags import LtiAgs | ||
| from lti_consumer.lti_1p3.nprs import LtiNrps | ||
| from lti_consumer.lti_1p3.constants import LTI_1P3_CONTEXT_TYPE | ||
| from lti_consumer.lti_1p3.consumer import LtiAdvantageConsumer, LtiConsumer1p3 | ||
|
|
||
|
|
@@ -726,3 +727,37 @@ def test_set_dl_content_launch_parameters(self): | |
| {"test": "test"} | ||
| ) | ||
| self.assertEqual(self.lti_consumer.launch_url, "example.com") | ||
|
|
||
| def test_no_nrps_returns_failure(self): | ||
| """ | ||
| Test that when LTI NRPS isn't configured, the class yields an error. | ||
| """ | ||
| with self.assertRaises(exceptions.LtiNrpsServiceNotSetUp): | ||
| self.lti_consumer.lti_nrps # pylint: disable=pointless-statement | ||
|
|
||
| def test_enable_nrps(self): | ||
| """ | ||
| Test enabling LTI NRPS and checking that required parameters are set. | ||
| """ | ||
| self.lti_consumer.enable_nrps("http://example.com/20/membership") | ||
|
|
||
| # Check that the NRPS class was properly instanced and set | ||
| self.assertIsInstance(self.lti_consumer.nrps, LtiNrps) | ||
|
|
||
| # Check retrieving class works | ||
| lti_nrps_class = self.lti_consumer.lti_nrps | ||
| self.assertEqual(self.lti_consumer.nrps, lti_nrps_class) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this testing an internal detail of the class? Is "nrps" private or not?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| # Check that enabling the NRPS adds the LTI NRPS claim | ||
| # in the launch message | ||
| self.assertEqual( | ||
| self.lti_consumer.extra_claims, | ||
| { | ||
| "https://purl.imsglobal.org/spec/lti-nrps/claim/namesroleservice": { | ||
| "context_memberships_url": "http://example.com/20/membership", | ||
| "service_versions": [ | ||
| "2.0" | ||
| ] | ||
| } | ||
| } | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shimulch We already have a role map (right above,
LTI_1P3_ROLE_MAP).For the context membership service, we don't need the "simple" version (it's just used in the
Resource Link Membership Service).Can you remove this and simplify the current implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@giovannicimolin Are
institution rolescan be used interchangeably withcontext roles? IMS seems to have separate vocabulary for both of them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shimulch I've missed that these are context roles as opposed of institution roles. You can keep this dict as is.