Skip to content

parameters.py - Expanding Error Code Catching#767

Open
satiowadahc wants to merge 2 commits into
oauthlib:masterfrom
satiowadahc:master
Open

parameters.py - Expanding Error Code Catching#767
satiowadahc wants to merge 2 commits into
oauthlib:masterfrom
satiowadahc:master

Conversation

@satiowadahc
Copy link
Copy Markdown

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
Copy Markdown
Member

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 auvipy and thedrow July 5, 2021 16:16
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

4 participants