Skip to content

Commit

Permalink
Don't scope dashboard URLs and cookies to organizations
Browse files Browse the repository at this point in the history
  • Loading branch information
marcospri committed Jul 24, 2024
1 parent 028562a commit 403c34e
Show file tree
Hide file tree
Showing 21 changed files with 141 additions and 154 deletions.
2 changes: 1 addition & 1 deletion lms/js_config_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class DashboardRoutes(TypedDict):

course_assignments_metrics: str

organization_courses: str
organization_metrics: str

courses: str
"""Paginated endpoint to fetch courses"""
Expand Down
4 changes: 2 additions & 2 deletions lms/resources/_js_config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,8 @@ def enable_dashboard_mode(self) -> None:
course_assignments_metrics=self._to_frontend_template(
"api.dashboard.course.assignments.metrics"
),
organization_courses=self._to_frontend_template(
"api.dashboard.organizations.courses"
courses_metrics=self._to_frontend_template(
"api.dashboard.courses.metrics"
),
courses=self._to_frontend_template("api.dashboard.courses"),
assignments=self._to_frontend_template(
Expand Down
21 changes: 7 additions & 14 deletions lms/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,37 +247,30 @@ def includeme(config): # noqa: PLR0915

config.add_route(
"dashboard.assignment",
"/dashboard/organizations/{public_id}/assignments/{assignment_id}",
"/dashboard/assignments/{assignment_id}",
factory="lms.resources.dashboard.DashboardResource",
)
config.add_route(
"dashboard.course",
"/dashboard/organizations/{public_id}/courses/{course_id}",
"/dashboard/courses/{course_id}",
factory="lms.resources.dashboard.DashboardResource",
)

config.add_route(
"dashboard.organization",
"/dashboard/organizations/{organization_public_id}",
factory="lms.resources.dashboard.DashboardResource",
"dashboard", "/dashboard", factory="lms.resources.dashboard.DashboardResource"
)

config.add_route("api.dashboard.courses.metrics", "/api/dashboard/courses/metrics")
config.add_route("api.dashboard.courses", "/api/dashboard/courses")
config.add_route("api.dashboard.course", r"/api/dashboard/courses/{course_id}")

config.add_route(
"api.dashboard.organizations.courses",
"/api/dashboard/organizations/{organization_public_id}/courses",
"api.dashboard.assignment", r"/api/dashboard/assignments/{assignment_id}"
)

config.add_route(
"api.dashboard.assignment", "/api/dashboard/assignments/{assignment_id}"
)
config.add_route("api.dashboard.course", "/api/dashboard/courses/{course_id}")

config.add_route("api.dashboard.assignments", "/api/dashboard/assignments")
config.add_route(
"api.dashboard.course.assignments.metrics",
"/api/dashboard/courses/{course_id}/assignments/metrics",
r"/api/dashboard/courses/{course_id}/assignments/metrics",
)

config.add_route("api.dashboard.students", "/api/dashboard/students")
Expand Down
2 changes: 1 addition & 1 deletion lms/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def get_policy(request: Request): # noqa: PLR0911
# Routes that require the Google auth policy
return LMSGoogleSecurityPolicy()

if path.startswith(("/dashboard/organization", "/api/dashboard")):
if path.startswith(("/dashboard", "/api/dashboard")):
# For certain routes we only use the google policy in case it resulted
# non empty identity.
# This is useful for routes that can be used by admin pages users on top of
Expand Down
6 changes: 3 additions & 3 deletions lms/services/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,21 +141,21 @@ def search( # noqa: PLR0913, PLR0917

def get_courses(
self,
organization_ids: list[int],
instructor_h_userid: str | None = None,
organization: Organization | None = None,
h_userids: list[str] | None = None,
assignment_ids: list[str] | None = None,
) -> Select[tuple[Course]]:
"""Get a list of unique courses.
:param organization: organization the courses belong to.
:param organization_ids: organizations the courses belong to.
:param instructor_h_userid: return only courses where instructor_h_userid is an instructor.
:param h_userids: return only courses where these users are members.
:param assignment_ids: return only the courses these assignments belong to.
"""
courses_query = (
self._search_query(
organization_ids=[organization.id] if organization else None,
organization_ids=organization_ids,
h_userids=h_userids,
limit=None,
)
Expand Down
42 changes: 30 additions & 12 deletions lms/services/dashboard.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from pyramid.httpexceptions import HTTPNotFound, HTTPUnauthorized
from sqlalchemy import select

from lms.models.dashboard_admin import DashboardAdmin
from lms.models.organization import Organization
Expand Down Expand Up @@ -47,22 +48,39 @@ def get_request_course(self, request):

return course

def get_request_organization(self, request):
"""Get and authorize an organization for the given request."""
organization = self._organization_service.get_by_public_id(
public_id=request.matchdict["organization_public_id"]
)
if not organization:
raise HTTPNotFound()
def get_request_organizations(self, request) -> list[Organization]:
"""Get the relevant organizations for the current requests."""
organizations = []

if request.has_permission(Permissions.STAFF):
# STAFF members in our admin pages can access all organizations
return organization
# If we have an user, include the organization it belongs to
if request.lti_user:
lti_user_organization = request.lti_user.application_instance.organization

if not self._organization_service.is_member(
lti_user_organization, request.user
):
raise HTTPUnauthorized()

if not self._organization_service.is_member(organization, request.user):
organizations.append(lti_user_organization)

# Include any other organizations we are an admin in
admin_organizations = self.get_organizations_by_admin_email(
request.lti_user.email if request.lti_user else request.identity.userid
)
organizations.extend(admin_organizations)

if not organizations:
raise HTTPUnauthorized()

return organization
return organizations

def get_organizations_by_admin_email(self, email: str) -> list[Organization]:
return self._db.scalars(
select(Organization)
.join(DashboardAdmin)
.where(DashboardAdmin.email == email)
.distinct()
).all()

def add_dashboard_admin(
self, organization: Organization, email: str, created_by: str
Expand Down
2 changes: 1 addition & 1 deletion lms/static/scripts/frontend_apps/api-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export type CourseWithMetrics = Course & {
};

/**
* Response for `/api/dashboard/organizations/{organization_public_id}` call.
* Response for `/api/dashboard/course/metrics` call.
*/
export type CoursesResponse = {
courses: CourseWithMetrics[];
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/organizations/:organizationPublicId" nest>
<Route path="/dashboard" nest>
<DashboardApp />
</Route>
<Route path="/email/preferences">
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Link } from '@hypothesis/frontend-shared';
import { useMemo, useState } from 'preact/hooks';
import { Link as RouterLink, useParams } from 'wouter-preact';
import { Link as RouterLink } from 'wouter-preact';

import type {
Assignment,
Expand All @@ -10,7 +10,6 @@ import type {
} from '../../api-types';
import { useConfig } from '../../config';
import { urlPath, useAPIFetch } from '../../utils/api';
import { replaceURLParams } from '../../utils/url';
import DashboardActivityFilters from './DashboardActivityFilters';
import FormattedDate from './FormattedDate';
import OrderableActivityTable from './OrderableActivityTable';
Expand All @@ -30,9 +29,6 @@ const courseURL = (id: number) => urlPath`/courses/${String(id)}`;
export default function OrganizationActivity() {
const { dashboard } = useConfig(['dashboard']);
const { routes } = dashboard;
const { organizationPublicId } = useParams<{
organizationPublicId: string;
}>();

const [selectedStudents, setSelectedStudents] = useState<Student[]>([]);
const [selectedAssignments, setSelectedAssignments] = useState<Assignment[]>(
Expand All @@ -52,16 +48,11 @@ export default function OrganizationActivity() {
[selectedCourses],
);

const courses = useAPIFetch<CoursesResponse>(
replaceURLParams(routes.organization_courses, {
organization_public_id: organizationPublicId,
}),
{
h_userid: studentIds,
assignment_id: assignmentIds,
course_id: courseIds,
},
);
const courses = useAPIFetch<CoursesResponse>(routes.courses_metrics, {
h_userid: studentIds,
assignment_id: assignmentIds,
course_id: courseIds,
});
const rows: CoursesTableRow[] = useMemo(
() =>
courses.data?.courses.map(({ id, title, course_metrics }) => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ describe('OrganizationActivity', () => {
fakeConfig = {
dashboard: {
routes: {
organization_courses:
'/api/dashboard/organizations/:organization_public_id',
courses_metrics:
'/api/courses/metrics',
},
},
};
Expand Down
2 changes: 1 addition & 1 deletion lms/static/scripts/frontend_apps/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export type DashboardRoutes = {
course_assignments_metrics: string;

/** Fetch courses stats */
organization_courses: string;
courses_metrics: string;

/** Fetch list of courses */
courses: string;
Expand Down
6 changes: 1 addition & 5 deletions lms/views/admin/assignment.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,7 @@ def assignment_dashboard(self):

response = HTTPFound(
location=self.request.route_url(
"dashboard.assignment",
public_id=assignment.groupings[
0
].application_instance.organization.public_id, # type: ignore
assignment_id=assignment.id,
"dashboard.assignment", assignment_id=assignment.id
),
)
return response
Expand Down
11 changes: 7 additions & 4 deletions lms/views/dashboard/api/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ def courses(self) -> APICourses:
filter_by_h_userids = self.request.parsed_params.get("h_userids")
filter_by_assignment_ids = self.request.parsed_params.get("assignment_ids")

organizations = self.dashboard_service.get_request_organizations(self.request)

courses = self.course_service.get_courses(
organization_ids=[org.id for org in organizations],
instructor_h_userid=self.request.user.h_userid
if self.request.user
else None,
Expand All @@ -73,19 +76,19 @@ def courses(self) -> APICourses:
}

@view_config(
route_name="api.dashboard.organizations.courses",
route_name="api.dashboard.courses.metrics",
request_method="GET",
renderer="json",
permission=Permissions.DASHBOARD_VIEW,
schema=CoursesMetricsSchema,
)
def organization_courses(self) -> APICourses:
def courses_metrics(self) -> APICourses:
filter_by_h_userids = self.request.parsed_params.get("h_userids")
filter_by_assignment_ids = self.request.parsed_params.get("assignment_ids")
filter_by_course_ids = self.request.parsed_params.get("course_ids")
org = self.dashboard_service.get_request_organization(self.request)
organizations = self.dashboard_service.get_request_organizations(self.request)
courses_query = self.course_service.get_courses(
organization=org,
organization_ids=[org.id for org in organizations],
instructor_h_userid=self.request.user.h_userid
if self.request.user
else None,
Expand Down
16 changes: 7 additions & 9 deletions lms/views/dashboard/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
renderer="lms:templates/dashboard/forbidden.html.jinja2",
)
@forbidden_view_config(
route_name="dashboard.organization",
route_name="dashboard",
request_method="GET",
renderer="lms:templates/dashboard/forbidden.html.jinja2",
)
Expand Down Expand Up @@ -54,9 +54,7 @@ def assignment_redirect_from_launch(self):
assignment_id = self.request.matchdict["assignment_id"]
response = HTTPFound(
location=self.request.route_url(
"dashboard.assignment",
public_id=self.request.lti_user.application_instance.organization.public_id,
assignment_id=assignment_id,
"dashboard.assignment", assignment_id=assignment_id
),
)
self._set_lti_user_cookie(response)
Expand Down Expand Up @@ -95,18 +93,18 @@ def course_show(self):
return {"title": course.lms_name}

@view_config(
route_name="dashboard.organization",
route_name="dashboard",
permission=Permissions.DASHBOARD_VIEW,
request_method="GET",
renderer="lms:templates/dashboard/index.html.jinja2",
)
def organization_show(self):
def courses(self):
"""Start the dashboard miniapp in the frontend scoped to an organization.
Authenticated via the LTIUser present in a cookie making this endpoint accessible directly in the browser.
"""
# Just get the org for the side effect of checking permissions.
_ = self.dashboard_service.get_request_organization(self.request)
_ = self.dashboard_service.get_request_organizations(self.request)
self.request.context.js_config.enable_dashboard_mode()
self._set_lti_user_cookie(self.request.response)
# Org names are not 100% ready for public consumption, let's hardcode a title for now.
Expand All @@ -128,8 +126,8 @@ 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
path=f"/dashboard/organizations/{lti_user.application_instance.organization.public_id}",
# Scope the cookie to all dashboard views
path="/dashboard",
max_age=60 * 60 * 24, # 24 hours, matches the lifetime of the auth_token
)
return response
2 changes: 1 addition & 1 deletion tests/unit/lms/resources/_js_config/__init___test.py
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ def test_it(self, js_config, lti_user):
"students_metrics": "/api/dashboard/students/metrics",
"course": "/api/dashboard/courses/:course_id",
"course_assignments_metrics": "/api/dashboard/courses/:course_id/assignments/metrics",
"organization_courses": "/api/dashboard/organizations/:organization_public_id/courses",
"courses_metrics": "/api/dashboard/courses/metrics",
"courses": "/api/dashboard/courses",
"assignments": "/api/dashboard/assignments",
"students": "/api/dashboard/students",
Expand Down
Loading

0 comments on commit 403c34e

Please sign in to comment.