Skip to content

Commit

Permalink
Add logging to detect concurrent token refreshes for most OAuth APIs
Browse files Browse the repository at this point in the history
As a step towards preventing concurrent refreshes for all OAuth APIs, try to
acquire the advisory lock on all OAuth token refreshes performed by
`OAuthHTTPService`, but only prevent concurrent refreshes when the caller
opts-in, which currently means only Canvas Studio. Otherwise we just write a
log entry so we can count how often concurrent refreshes occur.
  • Loading branch information
robertknight committed Jun 3, 2024
1 parent d8df9d1 commit 1efd7a3
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 13 deletions.
21 changes: 13 additions & 8 deletions lms/services/oauth_http.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import logging
from datetime import datetime, timedelta

from marshmallow import fields
Expand All @@ -13,6 +14,8 @@
from lms.validation import RequestsResponseSchema
from lms.validation.authentication import OAuthTokenResponseSchema

LOG = logging.getLogger(__name__)


class _OAuthAccessTokenErrorResponseSchema(RequestsResponseSchema):
"""Schema for parsing OAuth 2 access token error response bodies."""
Expand Down Expand Up @@ -117,14 +120,16 @@ def refresh_access_token(
):
return old_token.access_token

if prevent_concurrent_refreshes:
# Prevent concurrent refresh attempts. If acquiring the lock fails,
# the client should wait briefly and try again, at which point it
# should find the refreshed token already available and skip the
# refresh.
try:
self._oauth2_token_service.try_lock_for_refresh(self.service)
except CouldNotAcquireLock as exc:
# Check for concurrent refresh attempts.
try:
self._oauth2_token_service.try_lock_for_refresh(self.service)
except CouldNotAcquireLock as exc:
LOG.debug('Concurrent OAuth token refresh with token URL "%s"', token_url)
if prevent_concurrent_refreshes:
# Prevent concurrent refresh attempts. If acquiring the lock
# fails, the client should wait briefly and try again, at which
# point it should find the refreshed token already available and
# skip the refresh.
raise ConcurrentTokenRefreshError() from exc

try:
Expand Down
23 changes: 18 additions & 5 deletions tests/unit/lms/services/oauth_http_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,12 +241,25 @@ def test_refresh_access_token_acquires_refresh_lock(
sentinel.auth,
prevent_concurrent_refreshes=prevent_concurrent_refreshes,
)
if prevent_concurrent_refreshes:
oauth2_token_service.try_lock_for_refresh.assert_called_once()
else:
oauth2_token_service.try_lock_for_refresh.assert_not_called()

def test_refresh_access_token_raises_ConcurrentTokenRefreshError_on_concurrent_refresh(
# We acquire the lock whether or not concurrent refreshes are allowed,
# but only raise if not allowed.
oauth2_token_service.try_lock_for_refresh.assert_called_once()

def test_refresh_access_does_not_raise_if_concurrent_refreshes_allowed(
self,
svc,
oauth2_token_service,
):
oauth2_token_service.try_lock_for_refresh.side_effect = CouldNotAcquireLock()
svc.refresh_access_token(
sentinel.token_url,
sentinel.redirect_uri,
sentinel.auth,
prevent_concurrent_refreshes=False,
)

def test_refresh_access_token_raises_if_concurrent_refreshes_prevented(
self,
svc,
oauth2_token_service,
Expand Down

0 comments on commit 1efd7a3

Please sign in to comment.