Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: fetch LMS_USER_ID from LMS #584

Merged
merged 3 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions license_manager/apps/api/mixins.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
from functools import cached_property

from rest_framework.exceptions import ParseError

from license_manager.apps.api import utils
from license_manager.apps.api_client.lms import LMSApiClient


class UserDetailsFromJwtMixin:
Expand All @@ -18,9 +21,17 @@ def decoded_jwt(self):

return utils.get_decoded_jwt(self.request)

@property
@cached_property
def lms_user_id(self):
return utils.get_key_from_jwt(self.decoded_jwt, 'user_id')
"""
Retrieve the LMS user ID.
"""
try:
return utils.get_key_from_jwt(self.decoded_jwt, 'user_id')
Copy link
Member

@adamstankiewicz adamstankiewicz Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] It may be worth following the pattern of integrating the lms_user_id as a model field on the core User model, similar to how enterprise-access is set up.

The benefit of this is that the user id is persisted in the database for each User record, which is created whenever an authenticated API request is made to the service.

For example, the EDX_DRF_EXTENSIONS setting can be modified to include lms_user_id (example source) and the lms_user_id field can be added to the core User model (example source).

Copy link
Member

@adamstankiewicz adamstankiewicz Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that said, I suppose this would still assume a JWT cookie with a user_id claim...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use the pattern Adam described above and do something like this:

  1. Try to lookup the lms_user_id by email from the model in this service.
  2. If it's not present, then go fetch from the LMS.
  3. Populate the model record with the fetched lms_user_id.

except ParseError:
lms_client = LMSApiClient()
user_id = lms_client.fetch_lms_user_id(self.request.user.email)
return user_id

@property
def user_email(self):
Expand Down
14 changes: 10 additions & 4 deletions license_manager/apps/api/v1/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3227,17 +3227,23 @@ def test_get_subsidy_missing_course_key(self):
assert response.status_code == status.HTTP_400_BAD_REQUEST

@mock.patch('license_manager.apps.api.v1.views.utils.get_decoded_jwt')
def test_get_subsidy_no_jwt(self, mock_get_decoded_jwt):
@mock.patch('license_manager.apps.api.mixins.LMSApiClient')
def test_get_subsidy_no_jwt(self, MockLMSApiClient, mock_get_decoded_jwt):
"""
Verify the view returns a 400 if the user_id could not be found in the JWT.
Verify the view makes an API call to fetch lmsUserId if user_id could not be found in the JWT.
"""
self._assign_learner_roles()
mock_get_decoded_jwt.return_value = {}
url = self._get_url_with_params()

# Mock the behavior of LMSApiClient to return a sample user ID
mock_lms_client = MockLMSApiClient.return_value
mock_lms_client.fetch_lms_user_id.return_value = 443
response = self.api_client.get(url)
assert status.HTTP_404_NOT_FOUND == response.status_code

assert status.HTTP_400_BAD_REQUEST == response.status_code
assert '`user_id` is required and could not be found in your jwt' in str(response.content)
# Assert that the LMSApiClient.fetch_lms_user_id method was called once with the correct argument
mock_lms_client.fetch_lms_user_id.assert_called_once_with(self.user.email)

def test_get_subsidy_no_subscription_for_enterprise_customer(self):
"""
Expand Down
43 changes: 43 additions & 0 deletions license_manager/apps/api_client/lms.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import logging

import requests
from django.conf import settings

from license_manager.apps.api_client.base_oauth import BaseOAuthClient


logger = logging.getLogger(__name__)


class LMSApiClient(BaseOAuthClient):
"""
API client for calls to the LMS.
"""
api_base_url = settings.LMS_URL
user_details_endpoint = api_base_url + '/api/user/v1/accounts'

def fetch_lms_user_id(self, email):
"""
Fetch user details for the specified user email.

Arguments:
email (str): Email of the user for which we want to fetch details for.

Returns:
str: lms_user_id of the user.
"""
# {base_api_url}/api/user/v1/[email protected]
try:
query_params = {'email': email}
response = self.client.get(self.user_details_endpoint, params=query_params)
response.raise_for_status()
response_json = response.json()
return response_json[0].get('id')
except requests.exceptions.HTTPError as exc:
logger.error(

Check warning on line 37 in license_manager/apps/api_client/lms.py

View check run for this annotation

Codecov / codecov/patch

license_manager/apps/api_client/lms.py#L30-L37

Added lines #L30 - L37 were not covered by tests
'Failed to fetch user details for user {email} because {reason}'.format(
email=email,
reason=str(exc),
)
)
raise exc

Check warning on line 43 in license_manager/apps/api_client/lms.py

View check run for this annotation

Codecov / codecov/patch

license_manager/apps/api_client/lms.py#L43

Added line #L43 was not covered by tests
Loading