Added exception for Twitter and OAuth missing options #3676
Conversation
montymxb
changed the title
src/Adapters/Auth/OAuth1Client.js
Outdated
|
|
||
| var OAuth = function(options) { | ||
| if(!options) { | ||
| throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'No options passed to OAuth'); |
There was a problem hiding this comment.
this should be a -1 don't you think? As it's still a server internal error?
There was a problem hiding this comment.
Yea that makes sense, considering case there's no way to fire the exception unless you call it without the proper object.
src/Adapters/Auth/twitter.js
Outdated
| @@ -5,6 +5,9 @@ var logger = require('../../logger').default; | |||
|
|
|||
| // Returns a promise that fulfills iff this user id is valid. | |||
| function validateAuthData(authData, options) { | |||
| if(!options) { | |||
| throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'Twitter auth configuration missing'); | |||
There was a problem hiding this comment.
same here, it should probably be a -1 as it's a server misconfiguration. easy to spot with the message.
There was a problem hiding this comment.
Yea this will work as well. I took a quick look at all the error codes seeing if there was something more relevant for a misconfig, nothing looks to fit the bill. -1 it is!
There was a problem hiding this comment.
Yeah, -1 is a go to error for server misconfiguration.
|
@flovilmart updated with new error codes. Noted that INTERNAL_SERVER_ERROR is a code 1 instead of a -1, just using the provided definition. |
|
Good call :) |
|
No problem! Let me know if you need those commits squashed or anything else before this goes in. |
|
We squash on merge! It's ready to go! |
|
@montymxb updated the pull request - view changes |
|
Thanks again for the quick fix! |
|
Excellent! No problem and thanks for the quick response @flovilmart |
Bringing over a patch for a noted issue in a PR from the php sdk.
It looks like while attempting to utilize third party authentication via twitter there is the possibility for the server to experience an error. This can be caused if the server is either missing it's
authconfiguration option entirely or just thetwittercomponent.Taking a look at
src/Adapters/Auth/twitter.jsundervalidateAuthDataon line 9 you can see where anoptionsobject (from the server config) of undefined may be passed unchecked into the constructor ofOAuth.Internally OAuth won't attempt to discern whether it's been passed a valid object or not, and will attempt to extract the properties regardless. This ends up with a
TypeErrorbeing thrown with the messageCannot read property 'consumer_key' of undefinedwhen access is attempted on the first property.Ultimately the exception is noted in the server logs but the client receives the following response:
{"code":1,"message":"Internal server error."}This is particularly troublesome as this is a valid response to most SDKs, in this case to the php sdk. Without any
errorproperty in sight the sdk will proceed to return the results for further use as legitimate data.While the SDKs could attempt to screen for this type of response it's not very specific or helpful without the server logs. This PR attempts to address that by adding a check and exception throw in both
twitter.jsand inOAuth1Client.js.The exception thrown in OAuth is more general, simply indicating that no options were passed for OAuth. This is mostly to prevent unintended usage in the future.
However, for the sdks this response may not be specific enough for say, a missing twitter auth. In this regard
twitter.validateAuthDatacan check and throw a specific exception'Twitter auth configuration missing'upon a missing options parameter. This exception is more pointed towards notifying the SDK (and it's developers) that their server is missing the required configuration. This not only plugs up the current internal server error, but, ideally, promotes correction of it as well.Tests have been updated to account for this behavior in both cases. Also a little typo fix in
TwitterAuth.spec.js, missing semi-colon :).