celtic-project / LTI-PHP

PHP class library for building LTI integrations
GNU Lesser General Public License v3.0
48 stars 38 forks source link

System::parseRoles fails to parse institution-level roles in LTI 1.3 #53

Closed chrispittman closed 1 year ago

chrispittman commented 1 year ago

LTI 1.3 allows system- and institution-level roles. When passed to System::parseRoles(), the return value contains things which are not valid LTI roles (and which are not recognized by helper functions such as User::isAdmin()). For example: http://purl.imsglobal.org/vocab/lis/v2/institution/person#Administrator gets parsed into /http://purl.imsglobal.org/vocab/lis/v2/institution#person#Administrator (Note the leading slash and the slash-to-hash substitution at the end.)

This happens because parseRoles() is a big if/else chain which computes a $prefix variable based on what the role starts with, and there's nothing tellilng it what to do with system- or institution-level roles. In my own testing, the problem seems to be fixed by adding handling for those roles at the end of the if/else chain here: https://github.com/celtic-project/LTI-PHP/blob/709ac0ebe56b42bfc6342ee8f2252aa51ae938a8/src/System.php#L806

                    } elseif (substr($role, 0, 52) === 'http://purl.imsglobal.org/vocab/lis/v2/system/person') {
                        $prefix = 'http://purl.imsglobal.org/vocab/lis/v2/system/person';
                        $role = substr($role, 52);
                    } elseif (substr($role, 0, 57) === 'http://purl.imsglobal.org/vocab/lis/v2/institution/person') {
                        $prefix = 'http://purl.imsglobal.org/vocab/lis/v2/institution/person';
                        $role = substr($role, 57);
                    }

This problem may exist in the parsing for LTI 1.0 and LTI 2 as well - I didn't look into those too deeply.

spvickers commented 1 year ago

Thanks for reporting this issue. I have committed a change which should fix this, but I'll do some more testing before making a new release.

chrispittman commented 1 year ago

I haven't tested this extensively myself either, but I can confirm that this appears to fix this specific problem.