Correctly throw an exception when handing a text response #92

benbc · 2018-02-08T18:26:57Z

Hello. We've seen this problem in our running system when calling GetToken.client_credentials(). We currently have to parse the response text to see if it looks like an error message rather than a token.

lbalmaceda

Hey! 👋 I left you a few comments. Please add the remaining tests.

lbalmaceda · 2018-03-08T16:52:03Z

auth0/v3/authentication/base.py

        try:
-            text = json.loads(response.text) if response.text else {}
+            result = json.loads(response.text) if response.text else {}


result is the "original" error response body?

lbalmaceda · 2018-03-08T16:52:56Z

auth0/v3/authentication/base.py

        try:
-            text = json.loads(response.text) if response.text else {}
+            result = json.loads(response.text) if response.text else {}
+            error_code = result.get('error', '')


I've seen some error responses return code instead of error. Both meaning the same thing. If this value is not present I'd anyway set something like "a0.sdk.internal.unknown".

lbalmaceda · 2018-03-08T16:57:06Z

auth0/v3/authentication/base.py

-                                 message=text.get('error_description', ''))
-        return text
+            result = response.text
+            error_code = ''


I'd set something like "a0.sdk.internal.plain"

Previously the text of the response was always returned if it could not be interpreted as JSON.

benbc · 2018-03-13T16:14:25Z

@lbalmaceda I've addressed your comments. I ended up rewriting it completely because I didn't like the overlapping logic. Maybe seems a bit over-engineered now, but at least it's clear what's going on in each case. See what you think.

lbalmaceda

Since python is not my main language I don't know how pythonish this code is. But it looks good and I'm OK with the changes. Thanks 🌮
@cocojoe Go ahead ⚡️

cocojoe · 2018-03-20T09:39:22Z

@benbc I had one question regarding the response, could you provide a sample of when you are receiving a plan text response for a client_credentials exchange? Thanks

cocojoe · 2018-03-20T10:06:08Z

@lbalmaceda I've ran tests locally, is OK

benbc · 2018-03-20T17:22:58Z

@cocojoe I'm afraid I don't know what that means.

lbalmaceda · 2018-03-20T19:36:20Z

I think what @cocojoe wants is a real example on when a response would be of type "plain text" like the one tested here. I asked @benbc to add it because some endpoints return plain text such as this one (check the sample response on the right). If this doesn't apply to this SDK feel free to remove it.
AFAIK the changes introduced on this PR affect all the endpoints as this is a general "error response parser", correct me if I'm wrong.

benbc · 2018-03-21T11:36:33Z

@cocojoe @lbalmaceda I'm afraid I don't know what to do here. We see an occasional problem in our running system where Auth0 returns a plain text error reponse and the library treats it as non-error. I've fixed that here because the bug was obvious. But I have no knowledge of the Auth0 APIs.

If you can tell me exactly what test you want me to add then I can do it. But otherwise I'm just going to have to leave this proposed fix in your hands and hope you can make something of it.

cocojoe · 2018-03-21T14:45:31Z

@benbc Sorry, I should have perhaps been more clear. In your original description you mentioned that this happened when you were calling client_credentials I would expect it to return JSON during an error. So the fact that it was throwing out plain/text was surprising. I was hoping you had a log of these (as this was the reason you raised the PR). So we could look into this in more detail. Thanks

benbc · 2018-03-28T08:39:13Z

@cocojoe I'm afraid I don't have an example of this to hand. It hasn't happened for a few weeks.

lbalmaceda · 2018-04-19T20:13:31Z

@cocojoe like I said, I'm OK with the changes. Can we please merge this one so we can have a clearer diff on the other error parsing fixing PRs?

benbc · 2018-05-09T09:12:46Z

@cocojoe We've just seen another of these errors. It was thrown from GetToken#client_credentials. We're on auth0-python==3.1.4.

<!doctype html>
E                 <!--[if lt IE 7 ]><html class="ie ie6" lang="en"> <![endif]-->
E                 <!--[if IE 7 ]><html class="ie ie7" lang="en"> <![endif]-->
E                 <!--[if IE 8 ]><html class="ie ie8" lang="en"> <![endif]-->
E                 <!--[if (gte IE 9)|!(IE)]><!-->   <html lang="en"> <!--<![endif]-->
E                 <head>
E               
E                   <!-- General Metas -->
E                   <meta charset="utf-8" />
E                   <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">  <!-- Force Latest IE rendering engine -->
E                   <title>Auth0 Error</title>
E                   <!--[if lt IE 9]>
E                     <script src="http://html5shim.googlecode.com/svn/trunk/html5.js"></script>
E                   <![endif]-->
E               
E                   <!-- Mobile Specific Metas -->
E                   <meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1" />
E               
E                   <!-- Stylesheets -->
E                   <style type="text/css" media="screen">
E                     body {
E                       background-color: #f1f1f1;
E                       margin: 0;
E                     }
E                     body,
E                     input,
E                     button {
E                       font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
E                     }
E               
E                     .container { margin: 50px auto 40px auto; width: 600px; text-align: center; }
E               
E                     a { color: #4183c4; text-decoration: none; }
E                     a:hover { text-decoration: underline; }
E               
E                     h1 { letter-spacing: -1px; line-height: 160px; font-size: 60px; font-weight: 100; margin: 0px; text-shadow: 0 1px 0 #fff; }
E                     p { color: rgba(0, 0, 0, 0.5); margin: 10px 0 10px; font-size: 18px; font-weight: 200; line-height: 1.6em;}
E               
E                     ul { list-style: none; margin: 25px 0; padding: 0; }
E                     li { display: table-cell; font-weight: bold; width: 1%; }
E               
E                     #suggestions {
E                       margin-top: 35px;
E                       color: #ccc;
E                     }
E                     #suggestions a {
E                       color: #666666;
E                       font-weight: 200;
E                       font-size: 14px;
E                       margin: 0 10px;
E                     }
E               
E                     @media (max-width: 480px) {
E                       h1 {line-height: 100px;}
E                       #suggestions a { font-size: 12px; }
E                     }
E               
E                     @media  (min-width: 281px) and (max-width: 340px) {
E                       .container { margin: 10px auto; width: 300px;}
E                     }
E               
E                     @media  (min-width: 341px) and (max-width: 480px) {
E                       .container { margin: 10px auto; width: 400px;}
E                     }
E                   </style>
E                 </head>
E                 <body>
E                   <div class="container">
E               
E                     <h1>Auth0</h1>
E                     <p><strong>Looks like something went wrong!</strong></p>
E               
E                     <p>We track errors automatically, but if the problem persists feel free to <a href="mailto:support@auth0.com">contact us</a>. In the meantime, try again.</p>
E               
E                     <div id="suggestions">
E                       <a href="mailto:support@auth0.com">contact us</a>
E                       <a href="http://status.auth0.com">Auth0 Status</a>
E                       <a href="https://twitter.com/auth0">@auth0</a>
E                     </div>
E                 </body>
E               </html>

cocojoe · 2018-05-09T10:30:29Z

@benbc thanks for reporting.

lbalmaceda suggested changes Mar 8, 2018

View reviewed changes

Correctly throw an exception when handing a text response

3033634

Previously the text of the response was always returned if it could not be interpreted as JSON.

benbc force-pushed the throw-exception-for-text-response-error branch from 866c726 to 3033634 Compare March 13, 2018 15:50

lbalmaceda approved these changes Mar 13, 2018

View reviewed changes

cocojoe added the CH: Fixed label Mar 20, 2018

cocojoe added this to the v3-Next milestone Mar 20, 2018

cocojoe merged commit 272c6e1 into auth0:master Apr 20, 2018

lbalmaceda modified the milestones: v3-Next, 3.2.0 Jul 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly throw an exception when handing a text response #92

Correctly throw an exception when handing a text response #92

benbc commented Feb 8, 2018

lbalmaceda left a comment

lbalmaceda Mar 8, 2018

lbalmaceda Mar 8, 2018

lbalmaceda Mar 8, 2018

benbc commented Mar 13, 2018

lbalmaceda left a comment

cocojoe commented Mar 20, 2018

cocojoe commented Mar 20, 2018

benbc commented Mar 20, 2018

lbalmaceda commented Mar 20, 2018 •

edited

Loading

benbc commented Mar 21, 2018

cocojoe commented Mar 21, 2018

benbc commented Mar 28, 2018

lbalmaceda commented Apr 19, 2018

benbc commented May 9, 2018

cocojoe commented May 9, 2018

Correctly throw an exception when handing a text response #92

Correctly throw an exception when handing a text response #92

Conversation

benbc commented Feb 8, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

lbalmaceda Mar 8, 2018

Choose a reason for hiding this comment

lbalmaceda Mar 8, 2018

Choose a reason for hiding this comment

lbalmaceda Mar 8, 2018

Choose a reason for hiding this comment

benbc commented Mar 13, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

cocojoe commented Mar 20, 2018

cocojoe commented Mar 20, 2018

benbc commented Mar 20, 2018

lbalmaceda commented Mar 20, 2018 • edited Loading

benbc commented Mar 21, 2018

cocojoe commented Mar 21, 2018

benbc commented Mar 28, 2018

lbalmaceda commented Apr 19, 2018

benbc commented May 9, 2018

cocojoe commented May 9, 2018

lbalmaceda commented Mar 20, 2018 •

edited

Loading