From 753293eefe8436fdaadafacb0edc2fbd9d71c4a5 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 23 Apr 2024 13:34:34 +0200 Subject: [PATCH] Include path in dashboard cookies Scope dashboard cookies to the current organization. This is done to allow multiple cookies to coexists in the same browser allowing multiple identities from different LMSes. We include the organization ID in the URL, not the full one (public_id) but just the last part of the identifier (_public_id). The full public id contains both the region (us / ca) and the type of object (org) but those are already part of the URL (region in the host name and org is in the path) --- lms/routes.py | 2 +- lms/security.py | 4 +-- .../frontend_apps/components/AppRoot.tsx | 2 +- .../components/test/AppRoot-test.js | 2 +- lms/views/admin/assignment.py | 9 +++++- lms/views/dashboard.py | 16 ++++++++-- tests/unit/conftest.py | 6 ++++ tests/unit/lms/security_test.py | 6 ++-- tests/unit/lms/views/admin/assignment_test.py | 26 +++++++++++++---- tests/unit/lms/views/dashboard_test.py | 29 +++++++++++++++---- 10 files changed, 82 insertions(+), 20 deletions(-) diff --git a/lms/routes.py b/lms/routes.py index 3409aa95e5..fa0acce8b0 100644 --- a/lms/routes.py +++ b/lms/routes.py @@ -235,7 +235,7 @@ def includeme(config): # pylint:disable=too-many-statements ) config.add_route( "dashboard.assignment", - "/dashboard/assignment/{id_}", + "/dashboard/organization/{public_id}/assignment/{id_}", factory="lms.resources.dashboard.DashboardResource", ) config.add_route("api.assignment.stats", "/api/assignment/{id_}/stats") diff --git a/lms/security.py b/lms/security.py index 9a2d64996f..b3007afd50 100644 --- a/lms/security.py +++ b/lms/security.py @@ -104,7 +104,7 @@ def get_policy(request: Request): # Routes that require the Google auth policy return LMSGoogleSecurityPolicy() - if path.startswith("/dashboard/assignment/") or path.startswith( + if path.startswith("/dashboard/organization/") or path.startswith( "/api/assignment/" ): # For certain routes we only use the google policy in case it resulted @@ -156,7 +156,7 @@ def get_policy(request: Request): partial(get_lti_user_from_bearer_token, location="form") ) - if path.startswith("/dashboard/assignment/"): + if path.startswith("/dashboard/organization/"): return LTIUserSecurityPolicy( partial(get_lti_user_from_bearer_token, location="cookies") ) diff --git a/lms/static/scripts/frontend_apps/components/AppRoot.tsx b/lms/static/scripts/frontend_apps/components/AppRoot.tsx index 397358a247..43c0a2bcf4 100644 --- a/lms/static/scripts/frontend_apps/components/AppRoot.tsx +++ b/lms/static/scripts/frontend_apps/components/AppRoot.tsx @@ -41,7 +41,7 @@ export default function AppRoot({ initialConfig, services }: AppRootProps) { - + diff --git a/lms/static/scripts/frontend_apps/components/test/AppRoot-test.js b/lms/static/scripts/frontend_apps/components/test/AppRoot-test.js index a37e185328..796a0db53f 100644 --- a/lms/static/scripts/frontend_apps/components/test/AppRoot-test.js +++ b/lms/static/scripts/frontend_apps/components/test/AppRoot-test.js @@ -75,7 +75,7 @@ describe('AppRoot', () => { { config: { mode: 'dashboard' }, appComponent: 'DashboardApp', - route: '/dashboard/assignment/123', + route: '/dashboard/organization/ORG_ID/assignment/123', }, { config: { mode: 'error-dialog' }, diff --git a/lms/views/admin/assignment.py b/lms/views/admin/assignment.py index 75c15903ba..a30d5c1a8f 100644 --- a/lms/views/admin/assignment.py +++ b/lms/views/admin/assignment.py @@ -51,7 +51,14 @@ def assignment_dashboard(self): ) response = HTTPFound( - location=self.request.route_url("dashboard.assignment", id_=assignment.id), + location=self.request.route_url( + "dashboard.assignment", + # pylint:disable=protected-access + public_id=assignment.groupings[ + 0 + ].application_instance.organization._public_id, + id_=assignment.id, + ), ) return response diff --git a/lms/views/dashboard.py b/lms/views/dashboard.py index 04701d99ed..1b29747672 100644 --- a/lms/views/dashboard.py +++ b/lms/views/dashboard.py @@ -40,7 +40,12 @@ def assignment_redirect_from_launch(self): """ assignment_id = self.request.matchdict["id_"] response = HTTPFound( - location=self.request.route_url("dashboard.assignment", id_=assignment_id), + location=self.request.route_url( + "dashboard.assignment", + # pylint:disable=protected-access + public_id=self.request.lti_user.application_instance.organization._public_id, + id_=assignment_id, + ), ) self._set_lti_user_cookie(response) return response @@ -123,8 +128,12 @@ def get_request_assignment(self): return assignment def _set_lti_user_cookie(self, response): + lti_user = self.request.lti_user + if not lti_user: + # An LTIUser might not exists if accessing from the admin pages. + return response auth_token = ( - BearerTokenSchema(self.request).authorization_param(self.request.lti_user) + BearerTokenSchema(self.request).authorization_param(lti_user) # White space is not allowed as a cookie character, remove the leading part .replace("Bearer ", "") ) @@ -133,6 +142,9 @@ def _set_lti_user_cookie(self, response): value=auth_token, secure=not self.request.registry.settings["dev"], httponly=True, + # Scope the cookie to all the org endpoints + # pylint:disable=protected-access + path=f"/dashboard/organization/{lti_user.application_instance.organization._public_id}", max_age=60 * 60 * 24, # 24 hours, matches the lifetime of the auth_token ) return response diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 816e697592..eb04ab7246 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -263,6 +263,7 @@ def application_instance(db_session): developer_key="TEST_DEVELOPER_KEY", provisioning=True, settings=ApplicationSettings({}), + organization=factories.Organization(), ) application_instance.settings.set("canvas", "sections_enabled", True) @@ -273,6 +274,11 @@ def application_instance(db_session): return application_instance +@pytest.fixture +def organization(application_instance): + return application_instance.organization + + @pytest.fixture def lti_registration(): return factories.LTIRegistration() diff --git a/tests/unit/lms/security_test.py b/tests/unit/lms/security_test.py index 557e0baa3b..4daa0d4a69 100644 --- a/tests/unit/lms/security_test.py +++ b/tests/unit/lms/security_test.py @@ -409,7 +409,9 @@ def test_forgets(self, policy, pyramid_request, get_policy): get_policy.return_value.forget.assert_called_once_with(pyramid_request) assert user_id == get_policy.return_value.forget.return_value - @pytest.mark.parametrize("path", ["/api/assignment/10", "/dashboard/assignment/10"]) + @pytest.mark.parametrize( + "path", ["/api/assignment/10", "/dashboard/organization/ORGID"] + ) def test_get_policy_google_when_available( self, pyramid_request, path, LMSGoogleSecurityPolicy ): @@ -488,7 +490,7 @@ def test_get_policy_email_preferences( ("/assignment", "form"), ("/assignment/edit", "form"), ("/dashboard/launch/assignment/10", "form"), - ("/dashboard/assignment/10", "cookies"), + ("/dashboard/organization/ORGID/assignment/10", "cookies"), ], ) def test_picks_lti_launches_with_bearer_token( diff --git a/tests/unit/lms/views/admin/assignment_test.py b/tests/unit/lms/views/admin/assignment_test.py index b446aa20cd..b2b38ca808 100644 --- a/tests/unit/lms/views/admin/assignment_test.py +++ b/tests/unit/lms/views/admin/assignment_test.py @@ -1,4 +1,4 @@ -from unittest.mock import Mock, sentinel +from unittest.mock import sentinel import pytest from h_matchers import Any @@ -6,8 +6,10 @@ from lms.models import EventType from lms.views.admin.assignment import AdminAssignmentViews +from tests import factories +# pylint:disable=protected-access class TestAdminAssignmentViews: def test_show(self, pyramid_request, assignment_service, views): pyramid_request.matchdict["id_"] = sentinel.id @@ -28,10 +30,16 @@ def test_show_not_found(self, pyramid_request, assignment_service, views): views.show() def test_assignment_dashboard( - self, pyramid_request, assignment_service, views, AuditTrailEvent + self, + pyramid_request, + assignment_service, + views, + AuditTrailEvent, + assignment, + organization, ): pyramid_request.matchdict["id_"] = sentinel.id - assignment_service.get_by_id.return_value = Mock(id=sentinel.id) + assignment_service.get_by_id.return_value = assignment response = views.assignment_dashboard() @@ -41,7 +49,7 @@ def test_assignment_dashboard( data={ "action": "view_dashboard", "model": "Assignment", - "id": sentinel.id, + "id": assignment.id, "source": "admin_pages", "userid": "TEST_USER_ID", "changes": {}, @@ -50,10 +58,18 @@ def test_assignment_dashboard( pyramid_request.registry.notify.has_call_with(AuditTrailEvent.return_value) assert response == Any.instance_of(HTTPFound).with_attrs( { - "location": "http://example.com/dashboard/assignment/sentinel.id", + "location": f"http://example.com/dashboard/organization/{organization._public_id}/assignment/{assignment.id}", } ) + @pytest.fixture + def assignment(self, application_instance, db_session): + assignment = factories.Assignment() + course = factories.Course(application_instance=application_instance) + factories.AssignmentGrouping(assignment=assignment, grouping=course) + db_session.flush() # Give the DB objects IDs + return assignment + @pytest.fixture def AuditTrailEvent(self, patch): return patch("lms.views.admin.assignment.AuditTrailEvent") diff --git a/tests/unit/lms/views/dashboard_test.py b/tests/unit/lms/views/dashboard_test.py index 4ba3b50718..80f93f4191 100644 --- a/tests/unit/lms/views/dashboard_test.py +++ b/tests/unit/lms/views/dashboard_test.py @@ -13,10 +13,11 @@ pytestmark = pytest.mark.usefixtures("h_api", "assignment_service") +# pylint:disable=protected-access class TestDashboardViews: @freeze_time("2024-04-01 12:00:00") def test_assignment_redirect_from_launch( - self, views, pyramid_request, BearerTokenSchema + self, views, pyramid_request, BearerTokenSchema, organization ): pyramid_request.matchdict["id_"] = sentinel.id @@ -27,17 +28,19 @@ def test_assignment_redirect_from_launch( ) assert response == Any.instance_of(HTTPFound).with_attrs( { - "location": "http://example.com/dashboard/assignment/sentinel.id", + "location": f"http://example.com/dashboard/organization/{organization._public_id}/assignment/sentinel.id", } ) assert ( response.headers["Set-Cookie"] - == "authorization=TOKEN; Max-Age=86400; Path=/; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" + == f"authorization=TOKEN; Max-Age=86400; Path=/dashboard/organization/{organization._public_id}; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" ) @freeze_time("2024-04-01 12:00:00") @pytest.mark.usefixtures("BearerTokenSchema") - def test_assignment_show(self, views, pyramid_request, assignment_service): + def test_assignment_show( + self, views, pyramid_request, assignment_service, organization + ): context = DashboardResource(pyramid_request) context.js_config = create_autospec(JSConfig, spec_set=True, instance=True) pyramid_request.context = context @@ -51,7 +54,23 @@ def test_assignment_show(self, views, pyramid_request, assignment_service): ) assert ( pyramid_request.response.headers["Set-Cookie"] - == "authorization=TOKEN; Max-Age=86400; Path=/; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" + == f"authorization=TOKEN; Max-Age=86400; Path=/dashboard/organization/{organization._public_id}; expires=Tue, 02-Apr-2024 12:00:00 GMT; secure; HttpOnly" + ) + + def test_assignment_show_with_no_lti_user( + self, views, pyramid_request, assignment_service + ): + context = DashboardResource(pyramid_request) + context.js_config = create_autospec(JSConfig, spec_set=True, instance=True) + pyramid_request.context = context + pyramid_request.lti_user = None + pyramid_request.matchdict["id_"] = sentinel.id + + views.assignment_show() + + assignment_service.get_by_id.assert_called_once_with(sentinel.id) + pyramid_request.context.js_config.enable_dashboard_mode.assert_called_once_with( + assignment_service.get_by_id.return_value ) def test_api_assignment_stats(