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: DEPR EdxRestApiClient #303

Merged
merged 1 commit into from
Sep 6, 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
4 changes: 4 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ Unreleased
----------
* Nothing

[6.0.0]
--------
Breaking Changes: The EdxRestApiClient` has been deprecated and removed in this release.

[5.7.1]
--------
chore: Update Requirements specifically to unpin the requests library
Expand Down
71 changes: 2 additions & 69 deletions edx_rest_api_client/client.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
import datetime
import json
import os
import socket
import os

import crum
import requests
import requests.utils
import slumber
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@Yagnesh1998: A reminder to update CHANGELOG.rst. See https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0051-bp-conventional-commits.html for handling backward incompatible changes like this one.

from edx_django_utils.cache import TieredCache
from edx_django_utils.monitoring import set_custom_attribute

from edx_rest_api_client.__version__ import __version__
from edx_rest_api_client.auth import BearerAuth, JwtAuth, SuppliedJwtAuth
from edx_rest_api_client.auth import SuppliedJwtAuth

# When caching tokens, use this value to err on expiring tokens a little early so they are
# sure to be valid at the time they are used.
Expand Down Expand Up @@ -299,69 +298,3 @@ def request(self, method, url, headers=None, **kwargs): # pylint: disable=argum
set_custom_attribute('api_client', 'OAuthAPIClient')
self._ensure_authentication()
return super().request(method, url, headers=headers, **kwargs)


class EdxRestApiClient(slumber.API):
"""
API client for edX REST API.

(deprecated) See docs/decisions/0002-oauth-api-client-replacement.rst.
"""

@classmethod
def user_agent(cls):
return USER_AGENT

@classmethod
def get_oauth_access_token(cls, url, client_id, client_secret, token_type='bearer',
timeout=(REQUEST_CONNECT_TIMEOUT, REQUEST_READ_TIMEOUT)):
# 'To help transition to OAuthAPIClient, use EdxRestApiClient.get_and_cache_jwt_oauth_access_token instead'
# 'of EdxRestApiClient.get_oauth_access_token to share cached jwt token used by OAuthAPIClient.'
return get_oauth_access_token(url, client_id, client_secret, token_type=token_type, timeout=timeout)

@classmethod
def get_and_cache_jwt_oauth_access_token(cls, url, client_id, client_secret,
timeout=(REQUEST_CONNECT_TIMEOUT, REQUEST_READ_TIMEOUT)):
return get_and_cache_oauth_access_token(url, client_id, client_secret, token_type="jwt", timeout=timeout)

def __init__(self, url, signing_key=None, username=None, full_name=None, email=None,
timeout=5, issuer=None, expires_in=30, tracking_context=None, oauth_access_token=None,
session=None, jwt=None, **kwargs):
"""
EdxRestApiClient is deprecated. Use OAuthAPIClient instead.

Instantiate a new client. You can pass extra kwargs to Slumber like
'append_slash'.

Raises:
ValueError: If a URL is not provided.

"""
set_custom_attribute('api_client', 'EdxRestApiClient')
if not url:
raise ValueError('An API url must be supplied!')

if jwt:
auth = SuppliedJwtAuth(jwt)
elif oauth_access_token:
auth = BearerAuth(oauth_access_token)
elif signing_key and username:
auth = JwtAuth(username, full_name, email, signing_key,
issuer=issuer, expires_in=expires_in, tracking_context=tracking_context)
else:
auth = None

session = session or requests.Session()
session.headers['User-Agent'] = self.user_agent()

session.timeout = timeout
super().__init__(
url,
session=session,
auth=auth,
**kwargs
)


EdxRestApiClient.user_agent.__func__.__doc__ = user_agent.__doc__
EdxRestApiClient.get_oauth_access_token.__func__.__doc__ = get_oauth_access_token.__doc__
1 change: 0 additions & 1 deletion edx_rest_api_client/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
# noinspection PyUnresolvedReferences
from requests.exceptions import Timeout # pylint: disable=unused-import
from slumber.exceptions import * # pylint: disable=wildcard-import
143 changes: 2 additions & 141 deletions edx_rest_api_client/tests/test_client.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import datetime
import json
import os
from unittest import TestCase, mock

import ddt
Expand All @@ -10,9 +9,8 @@
from freezegun import freeze_time

from edx_rest_api_client import __version__
from edx_rest_api_client.auth import JwtAuth
from edx_rest_api_client.client import (EdxRestApiClient, OAuthAPIClient, get_and_cache_oauth_access_token,
get_oauth_access_token, user_agent)
from edx_rest_api_client.client import (OAuthAPIClient, get_and_cache_oauth_access_token,
get_oauth_access_token)
from edx_rest_api_client.tests.mixins import AuthenticationTestMixin

URL = 'http://example.com/api/v2'
Expand All @@ -27,124 +25,12 @@
JWT = 'abc.123.doremi'


@ddt.ddt
class EdxRestApiClientTests(TestCase):
"""
Tests for the edX Rest API client.
"""

@ddt.unpack
@ddt.data(
({'url': URL, 'signing_key': SIGNING_KEY, 'username': USERNAME,
'full_name': FULL_NAME, 'email': EMAIL}, JwtAuth),
({'url': URL, 'signing_key': SIGNING_KEY, 'username': USERNAME, 'full_name': None, 'email': EMAIL}, JwtAuth),
({'url': URL, 'signing_key': SIGNING_KEY, 'username': USERNAME,
'full_name': FULL_NAME, 'email': None}, JwtAuth),
({'url': URL, 'signing_key': SIGNING_KEY, 'username': USERNAME, 'full_name': None, 'email': None}, JwtAuth),
({'url': URL, 'signing_key': SIGNING_KEY, 'username': USERNAME}, JwtAuth),
({'url': URL, 'signing_key': None, 'username': USERNAME}, type(None)),
({'url': URL, 'signing_key': SIGNING_KEY, 'username': None}, type(None)),
({'url': URL, 'signing_key': None, 'username': None, 'oauth_access_token': None}, type(None))
)
def test_valid_configuration(self, kwargs, auth_type):
"""
The constructor should return successfully if all arguments are valid.
We also check that the auth type of the api is what we expect.
"""
api = EdxRestApiClient(**kwargs)
self.assertIsInstance(api._store['session'].auth, auth_type) # pylint: disable=protected-access

@ddt.data(
{'url': None, 'signing_key': SIGNING_KEY, 'username': USERNAME},
{'url': None, 'signing_key': None, 'username': None, 'oauth_access_token': None},
)
def test_invalid_configuration(self, kwargs):
"""
If the constructor arguments are invalid, an InvalidConfigurationError should be raised.
"""
self.assertRaises(ValueError, EdxRestApiClient, **kwargs)

@mock.patch('edx_rest_api_client.auth.JwtAuth.__init__', return_value=None)
def test_tracking_context(self, mock_auth):
"""
Ensure the tracking context is included with API requests if specified.
"""
EdxRestApiClient(URL, SIGNING_KEY, USERNAME, FULL_NAME, EMAIL, tracking_context=TRACKING_CONTEXT)
self.assertIn(TRACKING_CONTEXT, mock_auth.call_args[1].values())

def test_oauth2(self):
"""
Ensure OAuth2 authentication is used when an access token is supplied to the constructor.
"""

with mock.patch('edx_rest_api_client.auth.BearerAuth.__init__', return_value=None) as mock_auth:
EdxRestApiClient(URL, oauth_access_token=ACCESS_TOKEN)
mock_auth.assert_called_with(ACCESS_TOKEN)

def test_supplied_jwt(self):
"""Ensure JWT authentication is used when a JWT is supplied to the constructor."""
with mock.patch('edx_rest_api_client.auth.SuppliedJwtAuth.__init__', return_value=None) as mock_auth:
EdxRestApiClient(URL, jwt=JWT)
mock_auth.assert_called_with(JWT)

def test_user_agent(self):
"""Make sure our custom User-Agent is getting built correctly."""
with mock.patch('socket.gethostbyname', return_value='test_hostname'):
default_user_agent = user_agent()
self.assertIn('python-requests', default_user_agent)
self.assertIn(f'edx-rest-api-client/{__version__}', default_user_agent)
self.assertIn('test_hostname', default_user_agent)

with mock.patch('socket.gethostbyname') as mock_gethostbyname:
mock_gethostbyname.side_effect = ValueError()
default_user_agent = user_agent()
self.assertIn('unknown_client_name', default_user_agent)

with mock.patch.dict(os.environ, {'EDX_REST_API_CLIENT_NAME': "awesome_app"}):
uagent = user_agent()
self.assertIn('awesome_app', uagent)

self.assertEqual(user_agent(), EdxRestApiClient.user_agent())


@ddt.ddt
class ClientCredentialTests(AuthenticationTestMixin, TestCase):
"""
Test client credentials requests.
"""

@responses.activate
def test_get_client_credential_access_token_success(self):
"""
Test that the get access token method handles 200 responses and returns the access token.
"""
code = 200
body = {"access_token": "my-token", "expires_in": 1000}
now = datetime.datetime.utcnow()

expected_return = ("my-token", now + datetime.timedelta(seconds=1000))

with freeze_time(now):
self._mock_auth_api(OAUTH_URL, code, body=body)
self.assertEqual(
Yagnesh1998 marked this conversation as resolved.
Show resolved Hide resolved
EdxRestApiClient.get_oauth_access_token(OAUTH_URL, "client_id", "client_secret"),
expected_return
)

@ddt.data(
(400, {"error": "denied"}),
(500, None)
)
@ddt.unpack
@responses.activate
def test_get_client_credential_access_token_failure(self, code, body):
"""
Test that the get access token method handles failure responses.
"""
with self.assertRaises(requests.RequestException):
self._mock_auth_api(OAUTH_URL, code, body=body)
EdxRestApiClient.get_oauth_access_token(OAUTH_URL, "client_id", "client_secret")
Yagnesh1998 marked this conversation as resolved.
Show resolved Hide resolved

Yagnesh1998 marked this conversation as resolved.
Show resolved Hide resolved
def test_refresh_token_required(self):
self._mock_auth_api(OAUTH_URL, 200, body=None)
with self.assertRaises(AssertionError):
Expand All @@ -160,31 +46,6 @@ def setUp(self):
super().setUp()
TieredCache.dangerous_clear_all_tiers()

@responses.activate
def test_shared_client_credential_jwt_access_token(self):
"""
Test that get_and_cache_jwt_oauth_access_token returns the same access token used by the OAuthAPIClient.
"""
body = {'access_token': "my-token", 'expires_in': 1000}
now = datetime.datetime.utcnow()
expected_return = ('my-token', now + datetime.timedelta(seconds=1000))

with freeze_time(now):
self._mock_auth_api(OAUTH_URL, 200, body=body)
actual_return = EdxRestApiClient.get_and_cache_jwt_oauth_access_token(
OAUTH_URL, 'client_id', 'client_secret'
)
self.assertEqual(actual_return, expected_return)
self.assertEqual(len(responses.calls), 1)

# ensure OAuthAPIClient uses the same cached auth token without re-requesting the token from the server
oauth_client = OAuthAPIClient(OAUTH_URL, 'client_id', 'client_secret')
self._mock_auth_api(URL, 200, {'status': 'ok'})
oauth_client.post(URL, data={'test': 'ok'})
self.assertEqual(oauth_client.auth.token, actual_return[0])
self.assertEqual(len(responses.calls), 2)
self.assertEqual(URL, responses.calls[1][0].url)
Yagnesh1998 marked this conversation as resolved.
Show resolved Hide resolved

@responses.activate
def test_token_caching(self):
"""
Expand Down
1 change: 0 additions & 1 deletion requirements/base.in
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,4 @@

edx-django-utils
requests
slumber
PyJWT
4 changes: 0 additions & 4 deletions requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,6 @@ pyjwt==2.8.0
pynacl==1.5.0
# via edx-django-utils
requests==2.32.3
# via
# -r requirements/base.in
# slumber
slumber==0.7.1
# via -r requirements/base.in
sqlparse==0.5.0
# via django
Expand Down
14 changes: 7 additions & 7 deletions requirements/common_constraints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ Django<5.0

# elasticsearch>=7.14.0 includes breaking changes in it which caused issues in discovery upgrade process.
# elastic search changelog: https://www.elastic.co/guide/en/enterprise-search/master/release-notes-7.14.0.html
# See https://github.com/openedx/edx-platform/issues/35126 for more info
elasticsearch<7.14.0

# django-simple-history>3.0.0 adds indexing and causes a lot of migrations to be affected
django-simple-history==3.0.0

# opentelemetry requires version 6.x at the moment:
# https://github.com/open-telemetry/opentelemetry-python/issues/3570
# Normally this could be added as a constraint in edx-django-utils, where we're
# adding the opentelemetry dependency. However, when we compile pip-tools.txt,
# that uses version 7.x, and then there's no undoing that when compiling base.txt.
# So we need to pin it globally, for now.
# Ticket for unpinning: https://github.com/openedx/edx-lint/issues/407
# Cause: https://github.com/openedx/event-tracking/pull/290
# event-tracking 2.4.1 upgrades to pymongo 4.4.0 which is not supported on edx-platform.
# We will pin event-tracking to do not break existing installations
# This can be unpinned once https://github.com/openedx/edx-platform/issues/34586
# has been resolved and edx-platform is running with pymongo>=4.4.0
event-tracking<2.4.1
7 changes: 2 additions & 5 deletions requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ exceptiongroup==1.2.1
# via
# -r requirements/test.txt
# pytest
filelock==3.14.0
filelock==3.15.4
# via
# -r requirements/ci.txt
# tox
Expand Down Expand Up @@ -311,7 +311,6 @@ requests==2.32.3
# -r requirements/test.txt
# requests-toolbelt
# responses
# slumber
# twine
requests-toolbelt==1.0.0
# via
Expand All @@ -336,8 +335,6 @@ six==1.16.0
# -r requirements/test.txt
# edx-lint
# python-dateutil
slumber==0.7.1
# via -r requirements/test.txt
sqlparse==0.5.0
# via
# -r requirements/test.txt
Expand Down Expand Up @@ -384,7 +381,7 @@ urllib3==2.2.2
# requests
# responses
# twine
virtualenv==20.26.2
virtualenv==20.26.3
# via
# -r requirements/ci.txt
# tox
Expand Down
3 changes: 0 additions & 3 deletions requirements/test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ requests==2.32.3
# -r requirements/base.txt
# requests-toolbelt
# responses
# slumber
# twine
requests-toolbelt==1.0.0
# via twine
Expand All @@ -208,8 +207,6 @@ six==1.16.0
# via
# edx-lint
# python-dateutil
slumber==0.7.1
# via -r requirements/base.txt
sqlparse==0.5.0
# via
# -r requirements/base.txt
Expand Down