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

chore(oauth): show an informative log when OAuthError is raised #3184

Closed
wants to merge 1 commit into from

Conversation

ykdojo
Copy link

@ykdojo ykdojo commented Nov 11, 2022

This is similar to the way facebook/views.py logs an exception.

This feature was requested on #2142.

The original (closed) PR: #2175

Sample log:

Error processing OAuth
Traceback (most recent call last):
  File "/Users/ykdojo/Desktop/django-allauth/allauth/socialaccount/providers/oauth/views.py", line 79, in login
    return client.get_redirect(auth_url, auth_params)
  File "/Users/ykdojo/Desktop/django-allauth/allauth/socialaccount/providers/oauth/client.py", line 157, in get_redirect
    request_token = self._get_request_token()
  File "/Users/ykdojo/Desktop/django-allauth/allauth/socialaccount/providers/oauth/client.py", line 84, in _get_request_token
    raise OAuthError(
allauth.socialaccount.providers.oauth.client.OAuthError: Invalid response while obtaining request token from api.twitter.com. Response text: <?xml version='1.0' encoding='UTF-8'?><errors><error code="415">Callback URL not approved for this client application. Approved callback URLs can be adjusted in your application settings</error></errors>

Submitting Pull Requests

General

  • Make sure you use semantic commit messages.
    Examples: "fix(google): Fixed foobar bug", "feat(accounts): Added foobar feature".
  • All Python code must formatted using Black, and clean from pep8 and isort issues.
  • JavaScript code should adhere to StandardJS.
  • If your changes are significant, please update ChangeLog.rst.
  • If your change is substantial, feel free to add yourself to AUTHORS.

Provider Specifics

In case you add a new provider:

  • Make sure unit tests are available.
  • Add an entry of your provider in test_settings.py::INSTALLED_APPS and docs/installation.rst::INSTALLED_APPS.
  • Add documentation to docs/providers.rst.
  • Add an entry to the list of supported providers over at docs/overview.rst.

This is similar to the way facebook/views.py logs an exception
_("Invalid response while obtaining request token" ' from "%s".')
% get_token_prefix(self.request_token_url)
_("Invalid response while obtaining request token from " +
f"{get_token_prefix(self.request_token_url)}. " +
Copy link
Owner

Choose a reason for hiding this comment

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

Issue: Using f-strings breaks the python 3.5 build.

@@ -76,6 +78,7 @@ def login(self, request, *args, **kwargs):
try:
return client.get_redirect(auth_url, auth_params)
except OAuthError as e:
logger.exception('Error processing OAuth')
Copy link
Owner

Choose a reason for hiding this comment

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

Issue: From a logging perspective, these errors can occur in production and don't necessarily indicate exceptional situations that need direct attention. Hence, I think we need to use the regular error level.

Suggested change
logger.exception('Error processing OAuth')
logger.error("OAuth authentication error", exc_info=True)

@katomaso
Copy link

katomaso commented Aug 2, 2023

Please @ykdojo , could you move on with that? I personally think it is very important to have logging of errors

@varunsaral
Copy link
Contributor

can i take up the work @pennersr ?

@pennersr
Copy link
Owner

@varunsaral Of course, this one seems to be abandoned... thanks.

@varunsaral
Copy link
Contributor

varunsaral commented Aug 31, 2023

@pennersr this can be closed,already fixed by #3392

@pennersr pennersr closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants