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 1 commit
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
11 changes: 9 additions & 2 deletions license_manager/apps/api/mixins.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from functools import cached_property
from rest_framework.exceptions import ParseError, status
from license_manager.apps.api_client.lms import LMSApiClient

from license_manager.apps.api import utils

Expand All @@ -18,9 +20,14 @@ 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')
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: 14 additions & 0 deletions license_manager/apps/api/v1/views.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logging
from collections import OrderedDict
from contextlib import suppress
from functools import cached_property
from uuid import uuid4

from celery import chain
Expand Down Expand Up @@ -45,6 +46,7 @@
track_license_changes_task,
update_user_email_for_licenses_task,
)
from license_manager.apps.api_client.lms import LMSApiClient
from license_manager.apps.subscriptions import constants, event_utils
from license_manager.apps.subscriptions.api import revoke_license
from license_manager.apps.subscriptions.exceptions import (
Expand Down Expand Up @@ -1186,6 +1188,18 @@ def _check_param_has_type(self, param_name, required_type):
self.validation_errors.append(param_name)
return param_value

""""
Another approach would be to override this property at child class level.
@cached_property
def lms_user_id(self):
try:
return utils.get_key_from_jwt(self.decoded_jwt, 'user_id')
except ParseError:
lms_client = LMSApiClient()
user_id = lms_client.fetch_lms_user_id(self.request.user.email)
return user_id
"""

Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to check if this is the right way to override a property. Stackoverflow answers suggests to override the property setter method instead. The changes worked locally though. https://stackoverflow.com/questions/3336767/overriding-inherited-properties-getters-and-setters-in-python

@property
def requested_notify_learners(self):
return self._check_param_has_type('notify', bool)
Expand Down
41 changes: 41 additions & 0 deletions license_manager/apps/api_client/lms.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import logging

from django.conf import settings
import requests

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(
'Failed to fetch user details for user {email} because {reason}'.format(
email=email,
reason=str(exc),
)
)
raise exc
Loading