Skip to content

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

@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,

@satiowadahc satiowadahc requested review from thedrow and auvipy July 5, 2021 16:16
@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.

3 participants