-
Notifications
You must be signed in to change notification settings - Fork 37
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
Distinguish 2FA auth errors when fetching account settings #1283
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aforcier Thank you for fixing this! It's looking good. I have some minor questions. 🙂
accountChanged.causeOfChange = AccountAction.FETCH_SETTINGS; | ||
|
||
AccountErrorType errorType; | ||
if (payload.error.apiError.equals("reauthorization_required")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the previous version, we had a null
check for payload.error
, should we keep that in here? Or are we sure that payload.error
and payload.error.apiError
will never be null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be okay to drop the null
check for payload.error
, as we already checked it - if you look a bit further up, we're calling checkError()
, which is calling payload.isError()
, which itself is just a null
check for payload.error
.
We can be pretty confident apiError
is not null because of the way it's initialized and used:
Lines 50 to 53 in 961609f
public WPComGsonNetworkError(BaseNetworkError error) { | |
super(error); | |
this.apiError = ""; | |
} |
Lines 147 to 176 in 961609f
WPComGsonNetworkError returnedError = new WPComGsonNetworkError(error); | |
if (error.hasVolleyError() && error.volleyError.networkResponse != null | |
&& error.volleyError.networkResponse.statusCode >= 400) { | |
String jsonString; | |
try { | |
jsonString = new String(error.volleyError.networkResponse.data, | |
HttpHeaderParser.parseCharset(error.volleyError.networkResponse.headers)); | |
} catch (UnsupportedEncodingException e) { | |
jsonString = ""; | |
} | |
JSONObject jsonObject; | |
try { | |
jsonObject = new JSONObject(jsonString); | |
} catch (JSONException e) { | |
jsonObject = new JSONObject(); | |
} | |
String apiError = jsonObject.optString("error", ""); | |
if (TextUtils.isEmpty(apiError)) { | |
// WP V2 endpoints use "code" instead of "error" | |
apiError = jsonObject.optString("code", ""); | |
} | |
String apiMessage = jsonObject.optString("message", ""); | |
if (TextUtils.isEmpty(apiMessage)) { | |
// Auth endpoints use "error_description" instead of "message" | |
apiMessage = jsonObject.optString("error_description", ""); | |
} | |
// Augment BaseNetworkError by what we can parse from the response | |
returnedError.apiError = apiError; |
True, it's not immutable, but in practice WPComGsonRequest
is never modified once it's delivered to the individual network client functions - it's either passed up the chain like it is here, or interpreted into custom error classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation!
accountChanged.causeOfChange = AccountAction.FETCH_SETTINGS; | ||
|
||
AccountErrorType errorType; | ||
if (payload.error.apiError.equals("reauthorization_required")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth it to add a comment here for what "reauthorization_required" means? Your description of it in the PR description is really good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good idea 👍 Added in 0cc15b1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding the comments!
Adds a distinct error type for the
reauthorization_required
API error, so we can identify it from the host app.Part of the fix for wordpress-mobile/WordPress-Android#8754. There will be a counterpart WPAndroid issue making use of the change.
We get this error for 2FA accounts when using a non-production WordPress.com OAuth client. Essentially, some APIs around account management are disabled in those cases for security reasons.
The error is a bit generic from the server-side - it essentially means the user isn't privileged to do the action (in this case, calling the
/me/settings/
endpoint) and needs to reauthorize. For bearer token-based login, there is no escalation of privileges possible, so the request just fails at that point. If using production credentials, this error should never be seen. This is pretty deeply embedded and I don't think it's worth changing server-side for a development-only issue.To test
Verify the connected account tests are still passing. There isn't otherwise a great way to test this in an automated way due to 2FA being required, but you can test this from the example app:
wp.OAUTH.APP.ID
andwp.OAUTH.APP.SECRET
inexample/gradle.properties
to those values.SETTINGS_FETCH_REAUTHORIZATION_REQUIRED_ERROR
in the log, but otherwise things are working.(These steps are similar to the WPAndroid ones I'll be sharing in the counterpart PR.)