Skip to content

Commit

Permalink
Apply security policies for /api/dashboard endpoints in a defined order
Browse files Browse the repository at this point in the history
Currently our security policy applies one sub policy based on an URL
path. It applies it regardless of the result, if the path matches, the
security policy is used even if it returns an empty identity.

This is general the right approach as our endpoints almost always
expect to be authenticated in only one way.

However for the dashboard API endpoints we want to first try the LMS
authorization and if that doesn't succeed try the google authentication
for staff members.

We could generalize this behaviour and apply a list of policies (usually of one element) until one succeeds for all paths.
We'll leave that for a follow up PR if this is necessary in more
scenarios.
  • Loading branch information
marcospri committed Aug 7, 2024
1 parent 5bcd876 commit c9e6018
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 13 deletions.
17 changes: 10 additions & 7 deletions lms/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,16 @@ def get_policy(request: Request): # noqa: PLR0911
return LMSGoogleSecurityPolicy()

if path.startswith(("/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
# other type of access
policy = LMSGoogleSecurityPolicy()
if policy.identity(request):
return policy
# For the dashboard API we prefer, in this order:
# - HeadersBearerTokenLTIUserPolicy()
# For requests authorized via an API token, this applies to LMS users
#
# - LMSGoogleSecurityPolicy()
# If the header token policy didn't succeed we try the google auth cookie, for staff users
policies = [HeadersBearerTokenLTIUserPolicy(), LMSGoogleSecurityPolicy()]
for policy in policies:
if policy.identity(request): # type: ignore
return policy

if path in {"/lti_launches", "/content_item_selection"}:
# Actual LTI backed authentication
Expand Down
31 changes: 25 additions & 6 deletions tests/unit/lms/security_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -429,17 +429,32 @@ def test_forgets(self, policy, pyramid_request, get_policy):
assert user_id == get_policy.return_value.forget.return_value

@pytest.mark.parametrize(
"path", ["/api/dashboard/assignments/10", "/dashboard/orgs/ORGID"]
"header_value,google_value,expected_policy",
[
(sentinel.ok, None, HeadersBearerTokenLTIUserPolicy),
(None, sentinel.ok, LMSGoogleSecurityPolicy),
(sentinel.ok, sentinel.ok, HeadersBearerTokenLTIUserPolicy),
(None, None, HeadersBearerTokenLTIUserPolicy),
],
)
def test_get_policy_google_when_available(
self, pyramid_request, path, LMSGoogleSecurityPolicy
def test_api_dashboard_policy(
self,
pyramid_request,
header_value,
google_value,
expected_policy,
HeadersBearerTokenLTIUserPolicy,
LMSGoogleSecurityPolicy,
):
pyramid_request.path = path
LMSGoogleSecurityPolicy.return_value.identity.return_value = sentinel.identity
pyramid_request.path = "/api/dashboard/assignments/10"
HeadersBearerTokenLTIUserPolicy.return_value.identity.return_value = (
header_value
)
LMSGoogleSecurityPolicy.return_value.identity.return_value = google_value

policy = SecurityPolicy.get_policy(pyramid_request)

assert policy == LMSGoogleSecurityPolicy.return_value
assert policy.__class__ == expected_policy

@pytest.mark.parametrize(
"path",
Expand Down Expand Up @@ -533,6 +548,10 @@ def LTIUserSecurityPolicy(self, patch):
def LMSGoogleSecurityPolicy(self, patch):
return patch("lms.security.LMSGoogleSecurityPolicy")

@pytest.fixture(autouse=True)
def HeadersBearerTokenLTIUserPolicy(self, patch):
return patch("lms.security.HeadersBearerTokenLTIUserPolicy")

@pytest.fixture(autouse=True)
def EmailPreferencesSecurityPolicy(self, patch):
return patch("lms.security.EmailPreferencesSecurityPolicy")
Expand Down

0 comments on commit c9e6018

Please sign in to comment.