-
Notifications
You must be signed in to change notification settings - Fork 33
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
Sakai LTI compatibility. LTI module combined with app. #1428
base: master
Are you sure you want to change the base?
Sakai LTI compatibility. LTI module combined with app. #1428
Conversation
3987cc2
to
cf80cc2
Compare
* Change var to const for js constants being injected into the page from the server * Allows the before_play_start event to determine if the lti content item picker should be displayed * Move picker display function into a common controller * Add an api method that’ll validate and sign a content item selection lti request * injects LTI_MESSAGE_TYPE and LTI_KEY js constants into the picker page template * LTILaunch gains a method to load the lti config that is associated with a lti key * Updated a few strings that specifically mention canvas * Moves all classes, tests, migrations, and configs from fuel/app/modules/lti into fuel/app * overhaul lti test provider to simplify * simplify lti launch picker code when sending content selection messages to lti/assignment * fixed issue with starting plays with referrer urls longer than 255 characters fixes ucfopen#1418 * updates oauth validation in fuel by ensuring the request uri is not masked by FuelPHP * Overhaul lit test provider a bit - mostly to simplify and use lessons * fix code sniff errors * add deps to docker image * attempt to fix yarn cache issue * Changes docker build process to ensure the built image has admin:make_paths_writable run to avoid having to run it every startup
cf80cc2
to
702747b
Compare
2b31389
to
c705e91
Compare
@@ -22,4 +22,4 @@ docker run \ | |||
--mount type=bind,source="$(pwd)"/../,target=/build \ | |||
--mount source=materia-asset-build-vol,target=/build/node_modules \ | |||
node:12.11.1-alpine \ | |||
/bin/ash -c "apk add --no-cache git && cd build && yarn install --frozen-lockfile --non-interactive --production --silent --pure-lockfile --force" |
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.
My Docker build process was dying regularly for some reason, this seems to have helped.
@@ -15,4 +15,4 @@ DCTEST="docker-compose -f docker-compose.yml -f docker-compose.override.test.yml | |||
set -e | |||
set -o xtrace | |||
|
|||
$DCTEST run --rm app /wait-for-it.sh mysql:3306 -t 20 -- composer run testci -- "$@" |
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.
Adding -T allowed the commit hook to work for me
@@ -10,4 +10,4 @@ DCTEST="docker-compose -f docker-compose.yml -f docker-compose.override.test.yml | |||
set -e | |||
set -o xtrace | |||
|
|||
$DCTEST run --rm --no-deps app composer sniff-ci |
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.
Again, working to add better support for pre-commit hooks
@@ -54,6 +54,13 @@ protected function tearDown(): void | |||
} | |||
} | |||
|
|||
protected static function remove_all_roles_for_user($user_id) |
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.
To get the tests to pass, I needed a way to easily wipe all roles for a user
\Js::push_inline('var BASE_URL = "'.\Uri::base().'";'); | ||
\Js::push_inline('var STATIC_CROSSDOMAIN = "'.\Config::get('materia.urls.static').'";'); | ||
\Js::push_inline('const BASE_URL = "'.\Uri::base().'";'); | ||
\Js::push_inline('const STATIC_CROSSDOMAIN = "'.\Config::get('materia.urls.static').'";'); |
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.
hey, const is a thing!
@@ -53,7 +52,7 @@ public static function from_request() | |||
return static::$launch; | |||
} | |||
|
|||
public static function config() | |||
public static function config_from_request() |
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.
Renamed because i added config_from_key
// When sending LTI ContentItem Selection requests, Sakai updated and required | ||
// that GET params in the url aren't duplicated in the body of params | ||
// as they caused oauth signature validation to fail in Sakai. | ||
'enforce_unique_params' => $_ENV['LTI_ENFORCE_UNIQUE_PARAMS'] ?? true, |
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.
I added this option as it's not super clear if other platforms will want this or not.
@@ -34,7 +34,14 @@ | |||
'preview/(:alnum)(/.*)?' => 'widgets/preview_widget/$1', | |||
'preview-embed/(:alnum)(/.*)?' => 'widgets/play_embedded_preview/$1', | |||
'embed/(:alnum)(/.*)?' => 'widgets/play_embedded/$1', | |||
'lti/assignment?' => 'widgets/play_embedded/$1', // legacy LTI url | |||
'lti/assignment?' => function () { |
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.
We don't use this much in Materia, so this may be difficult to find in the future. BUT, Fuel lets us add a function here to determine what controller receives a request. This is used to support lti implementations that only send requests to a single Materia url. To support this, lti/assignment will allow picker or content selection requests as well as widget play requests.
@@ -65,15 +65,15 @@ RUN composer install --no-cache --no-dev --no-progress --no-scripts --prefer-dis | |||
# ===================================================================================================== | |||
FROM node:12.11.1-alpine AS yarn_stage | |||
|
|||
RUN apk add --no-cache git | |||
RUN apk add --no-cache git python make g++ |
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.
I ran into issues on new base php images building that were helped by adding these
@@ -89,3 +89,15 @@ USER www-data | |||
COPY --from=composer_stage --chown=www-data:www-data /var/www/html /var/www/html | |||
COPY --from=yarn_stage --chown=www-data:www-data /build/public /var/www/html/public | |||
COPY --from=yarn_stage --chown=www-data:www-data /build/fuel/app/config/asset_hash.json /var/www/html/fuel/app/config/asset_hash.json | |||
|
|||
# set bogus values so admin task can run | |||
ENV AUTH_SIMPLEAUTH_SALT=TEMPINVALIDVALUE |
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.
php8 was unhappy running admin commands without these environment vars
@@ -19,7 +19,7 @@ | |||
}, | |||
"dependencies": { | |||
"fs-extra": "^8.0.1", | |||
"materia-server-client-assets": "2.4.2", | |||
"materia-server-client-assets": "https://github.com/iturgeon/Materia-Server-Client-Assets.git#842eaadcf38811bbaa37d804049cc36a4f643a9b", |
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.
THIS NEEDS TO BE UPDATED WHEN supporting MSCA PR is merged/tagged
An update on this: I plan on addressing and implementing the changes on this PR after the react branch has shipped (which is hopefully imminent). The major changes required will be converting the PHP partials into their corresponding react views, and integrating the related changes to MSCA, since that repo is being deprecated. |
Sounds good @clpetersonucf I can probably help out, so just ping me when it’s ready |
This is a work in progress.
Add LTI compatibility with The Sakai LMS
ContentItemSelectionRequest
launcheslti_sign_content_item_selection
Some specific testing should take place before merging:
This work is sponsored by LongSight