Skip to content

Commit

Permalink
fix lti launches without user ID
Browse files Browse the repository at this point in the history
  • Loading branch information
emmachughes committed Oct 17, 2023
1 parent 2c23905 commit f3ff6ac
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 12 deletions.
6 changes: 4 additions & 2 deletions sourcecode/apis/contentauthor/app/Http/Kernel.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,10 @@ class Kernel extends HttpKernel
'core.locale' => \App\Http\Middleware\LtiLocale::class,
'core.behavior-settings' => \App\Http\Middleware\LtiBehaviorSettings::class,
'signed.oauth10-request' => \App\Http\Middleware\SignedOauth10Request::class,
'lti.add-auth-to-session' => \App\Http\Middleware\LtiAddAuthToSession::class,
'lti.add-to-session' => \App\Http\Middleware\LtiAddToSession::class,
'lti.question-set' => \App\Http\Middleware\LtiQuestionSet::class,
'lti.qs-to-request' => \App\Http\Middleware\AddExtQuestionSetToRequestMiddleware::class,
'lti.signed-launch' => \App\Http\Middleware\LtiSignedLaunch::class,
'lti.verify-auth' => \App\Http\Middleware\LtiVerifyAuth::class,
'game-access' => GameAccess::class,
'questionset-access' => QuestionSetAccess::class,
Expand All @@ -100,7 +101,8 @@ class Kernel extends HttpKernel
protected $middlewarePriority = [
\Illuminate\Session\Middleware\StartSession::class,
\Illuminate\View\Middleware\ShareErrorsFromSession::class,
\App\Http\Middleware\LtiAddAuthToSession::class,
\App\Http\Middleware\LtiAddToSession::class,
\App\Http\Middleware\LtiSignedLaunch::class,
\App\Http\Middleware\LtiVerifyAuth::class,
\App\Http\Middleware\Authenticate::class,
\Illuminate\Session\Middleware\AuthenticateSession::class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
/**
* Add LTI parameters to session
*/
class LtiAddAuthToSession
final readonly class LtiAddToSession
{
public function __construct(
private readonly H5pLti $h5pLti,
Expand All @@ -27,6 +27,7 @@ public function handle(Request $request, Closure $next): Response
$ltiRequest = $this->h5pLti->getValidatedLtiRequest();

if ($ltiRequest != null) {
Session::put('signedLaunch', true);

Check warning on line 30 in sourcecode/apis/contentauthor/app/Http/Middleware/LtiAddToSession.php

View check run for this annotation

Codecov / codecov/patch

sourcecode/apis/contentauthor/app/Http/Middleware/LtiAddToSession.php#L30

Added line #L30 was not covered by tests
Session::put('userId', $ltiRequest->getUserId());

if ($ltiRequest->getUserId() != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

namespace App\Http\Middleware;

use Closure;
use Illuminate\Http\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Exception\UnauthorizedHttpException;

/**
* Verify that a signed LTI launch happened.
*/
final readonly class LtiSignedLaunch
{
/**
* @param (Closure(Request): Response) $next
*/
public function handle(Request $request, Closure $next): Response
{
if (
$request->session()->get('signedLaunch') ||
// For backwards compat.
// Cannot be set without a signed launch also having occurred.
$request->session()->get('authId')
) {
return $next($request);
}

throw new UnauthorizedHttpException(
challenge: 'OAuth',
message: 'A valid LTI launch is required to have occurred',
);

Check warning on line 32 in sourcecode/apis/contentauthor/app/Http/Middleware/LtiSignedLaunch.php

View check run for this annotation

Codecov / codecov/patch

sourcecode/apis/contentauthor/app/Http/Middleware/LtiSignedLaunch.php#L29-L32

Added lines #L29 - L32 were not covered by tests
}
}
Original file line number Diff line number Diff line change
@@ -1,26 +1,29 @@
<?php

declare(strict_types=1);

namespace App\Http\Middleware;

use Closure;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Session;
use Symfony\Component\HttpFoundation\Response;

use function abort;

/**
* Verify that the user is authenticated via LTI.
*/
class LtiVerifyAuth
final readonly class LtiVerifyAuth
{
/**
* @param (Closure(Request): Response) $next
*/
public function handle(Request $request, Closure $next): Response
{
if (Session::get('authId')) {
return $next($request);
if (!$request->session()->get('authId')) {
abort(Response::HTTP_FORBIDDEN);
}

return redirect('auth/login');
return $next($request);
}
}
2 changes: 1 addition & 1 deletion sourcecode/apis/contentauthor/routes/admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

Route::get('auth/login', [LoginController::class, 'showLoginForm'])->name('login');
Route::get('sso-edlib-admin', [LoginController::class, 'ssoFromEdlibAdmin'])
->middleware('lti.add-auth-to-session', 'lti.verify-auth', 'can:superadmin');
->middleware('lti.add-to-session', 'lti.signed', 'lti.verify-auth', 'can:superadmin');
Route::post('auth/login', [LoginController::class, 'login']);
Route::post('auth/logout', [LoginController::class, 'logout'])->name('logout');

Expand Down
2 changes: 1 addition & 1 deletion sourcecode/apis/contentauthor/routes/web.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
Route::get('h5p/{h5p}/download', [H5PController::class, 'downloadContent'])->name('content-download')->middleware(['adaptermode']);
Route::get('content/upgrade/library', [H5PController::class, 'contentUpgradeLibrary'])->name('content-upgrade-library');

Route::group(['middleware' => ['core.return', 'lti.add-auth-to-session', 'lti.verify-auth', 'core.locale', 'adaptermode']], function () {
Route::middleware(['core.return', 'lti.add-to-session', 'lti.signed-launch', 'core.locale', 'adaptermode'])->group(function () {
Route::post('lti-content/create', [LtiContentController::class, 'create']);
Route::post('lti-content/create/{type}', [LtiContentController::class, 'create']);
Route::post('lti-content/{id}', [LtiContentController::class, 'show'])->middleware(['core.behavior-settings:view']);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,7 @@ public function testViewArticle()

public function testMustBeLoggedInToCreateArticle()
{
$_SERVER['QUERY_STRING'] = 'forTestingPurposes';
$this->get(route('article.create'))
->assertStatus(Response::HTTP_FOUND);
->assertForbidden();
}
}

0 comments on commit f3ff6ac

Please sign in to comment.