search

numbas / numbas-lti-provider

An LTI tool provider to run Numbas exams
https://docs.numbas.org.uk/lti/en/latest/
Other
11 stars 12 forks source link

Check for fully-qualified instructor role #51

Closed wade-tattersall closed 3 years ago

wade-tattersall commented 6 years ago

In numbas_lti/views/mixins.py, the request_is_instructor method checks return 'Instructor' in request.LTI.get('roles'), and the MustBeInstructorMixin defines the only allowed role as "Instructor" too.

However, the LTI standard says that the role, when fully qualified, should be urn:lti:role:ims/lis/Instructor, but that the default namespace is the ims namespace so just 'Instructor' is enough. However, Blackboard Learn includes the fully-qualified role, which means that currently numbas-lti is only usable if the instructor has a superuser account on the numbas-lti server.

I think the arguably best solution would be for the django.lti-auth package to fully qualify all roles when it constructs them, and then you can test against those. However, an alternative would be for the numbas package to set allowed_roles = ["Instructor", "urn:lti:role:ims/lis/Instructor"] and return ("Instructor" in request.LTI.get('roles')) or ("urn:lti:role:ims/lis/Instructor" in request.LTI.get('roles')) on the relevant lines in numbas_lti/views/mixins.py. I've done this, and it seems to work so far.

More generally, there are a lot of roles that might be acceptable, such as urn:lti:role:ims/lis/ContentDeveloper, so perhaps including that as a setting might be worthwhile. Some LMSs might have options to configure this on their end, but Blackboard is not one of them.

christianp commented 6 years ago

I think the most straightforward way to solve this is to have an INSTRUCTOR_ROLES array in settings.py, maybe with a default of ['Instructor','urn:lti:role:ims/lis/Instructor']. Then if your VLE is sending something different, you can add it to that list without modifying the code.

christianp commented 6 years ago

The relevant part of the LTI spec is Appendix A, which lists fully qualified names for different roles.

christianp commented 6 years ago

GitHub auto-closed this. @wade-tattersall, I'd appreciate if you could update your instance and confirm it works with fully-qualified role names. I also updated the django-auth-lti package, which is installed from GitHub. You might need to remove it and rerun pip install -r requirements.txt - I don't fully understand how pip decides to reinstall packages from git!

christianp commented 5 years ago

@wade-tattersall have you had a chance to check this?

christianp commented 3 years ago

I'm closing this because I think it works.