Skip to content

Commit

Permalink
Include path in dashboard cookies
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
marcospri committed Apr 30, 2024
1 parent 288232b commit 753293e
Show file tree
Hide file tree
Showing 10 changed files with 82 additions and 20 deletions.
2 changes: 1 addition & 1 deletion lms/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
4 changes: 2 additions & 2 deletions lms/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
)
Expand Down
2 changes: 1 addition & 1 deletion lms/static/scripts/frontend_apps/components/AppRoot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export default function AppRoot({ initialConfig, services }: AppRootProps) {
<FilePickerApp />
</DataLoader>
</Route>
<Route path="/dashboard/assignment/:assignmentId">
<Route path="/dashboard/organization/:organizationId/assignment/:assignmentId">
<DashboardApp />
</Route>
<Route path="/email/preferences">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
Expand Down
9 changes: 8 additions & 1 deletion lms/views/admin/assignment.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
16 changes: 14 additions & 2 deletions lms/views/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 ", "")
)
Expand All @@ -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
6 changes: 6 additions & 0 deletions tests/unit/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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()
Expand Down
6 changes: 4 additions & 2 deletions tests/unit/lms/security_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
):
Expand Down Expand Up @@ -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(
Expand Down
26 changes: 21 additions & 5 deletions tests/unit/lms/views/admin/assignment_test.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
from unittest.mock import Mock, sentinel
from unittest.mock import sentinel

import pytest
from h_matchers import Any
from pyramid.httpexceptions import HTTPFound, HTTPNotFound

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
Expand All @@ -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()

Expand All @@ -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": {},
Expand All @@ -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")
Expand Down
29 changes: 24 additions & 5 deletions tests/unit/lms/views/dashboard_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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(
Expand Down

0 comments on commit 753293e

Please sign in to comment.