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

parameters.py - Expanding Error Code Catching #767

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

satiowadahc
Copy link

One public API I use returns errorCode: . This should now catch a wider variety of errors.

One public API I use returns errorCode: <error>. This should now catch a wider variety of errors.
@auvipy auvipy requested a review from JonathanHuot July 2, 2021 05:28
Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

would be great if you also update the unit tests

Copy link
Collaborator

@thedrow thedrow left a comment

Choose a reason for hiding this comment

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

Could you please provide an example where this is useful?

@satiowadahc
Copy link
Author

If I have time this week I'll investigate your testing code.

From Autodesk API an example error is:

{'developerMessage': 'The required parameter(s) client_id not present in the request', 'errorCode': 'AUTH-008', 'more info': 'https://forge.autodesk.com/en/docs/oauth/v2/developers_guide/error_handling/'}

So in parameters.py:

   if 'error' in params:
       raise_from_error(params.get('error'), params)

'error' is not in params. But it is in an params key. Likewise if an API uses 'ERROR' instead of error. it would never be caught as 'error' != 'ERROR'.

@satiowadahc
Copy link
Author

Or rather I had a few moments now. Note I'm editing on GitHub and haven't cloned locally yet. So this might take a commit or two,

@thedrow thedrow added the OAuth2-Provider This impact the provider part of OAuth2 label Jul 6, 2021
@JonathanHuot JonathanHuot added this to the 3.2.0 milestone Aug 12, 2021
@JonathanHuot
Copy link
Member

Hi,
oauthlib by-default only allows the RFC, however we have several points of extensions/customization possible to better fit to the reality.

I think the PR is too flexible as it does not enforce the list of field names. While the lowercase check might be a good idea (however a CaseInsensitiveDict would be easier to read?), the string search "error" in <string> will be a source of errors (acceptErrormatches).

Copy link
Member

@JonathanHuot JonathanHuot left a comment

Choose a reason for hiding this comment

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

We need to enforce the list of field names.
Change lowercase into a CaseInsensitiveDict.
The string search "error" in <string> cannot work (acceptError matches).

@satiowadahc
Copy link
Author

So if we had a predefined list of error codes? This way it follows RFC but can have added cases for API's that don't follow "standards"

self.reportable_errors = ["error", "Error"]

def get_errors(self)
    return self.reportable_errors
    
def add_error_code(self, err)
     self.reportable_errors.append(err)

.
.
.   
for key in params.keys():
    if key in self.reportable_errors:
        raise_from_error(params.get(key), params)
.
.
.

@JonathanHuot
Copy link
Member

Yes, that could work. However, I think the default's list should be what is in the RFC: only includes error ?

@auvipy auvipy modified the milestones: 3.2.0, 4.0.0 Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OAuth2-Provider This impact the provider part of OAuth2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants