Skip to content
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.

Sign up for GitHub

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

Handle 2FA error for OAuth client during login #10042

Merged
merged 6 commits into from
Jun 14, 2019
Merged

Handle 2FA error for OAuth client during login #10042

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21897f3
Update FluxC hash to 2FA error fix branch
aforcier Jun 13, 2019
a14c0c8
Show error and continue login on OAuth client 2FA error
aforcier Jun 13, 2019
2776870
Handle OAuth client 2FA error in settings screens
aforcier Jun 13, 2019
5760354
Drop README reference to fixed issue
aforcier Jun 13, 2019
705107b
Update 2FA unavailable API error message
aforcier Jun 14, 2019
8e266c9
Update FluxC hash to latest
aforcier Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ Using another account like [email protected] will cause the `Client cannot use "pass

For security reasons, some account-related actions aren't supported for development
builds when using a WordPress.com account with 2-factor authentication enabled.
There is also currently an [issue](https://github.com/wordpress-mobile/WordPress-Android/issues/8754) where a restart of the app is required to complete login in this case.

Read more about [OAuth2][6] and the [WordPress.com REST endpoint][7].

Expand Down
6 changes: 5 additions & 1 deletion WordPress/src/main/java/org/wordpress/android/ui/prefs/AccountSettingsFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,14 @@ public void onAccountChanged(OnAccountChanged event) {
} else {
if (event.isError()) {
switch (event.error.type) {
case SETTINGS_FETCH_ERROR:
case SETTINGS_FETCH_GENERIC_ERROR:
ToastUtils.showToast(getActivity(), R.string.error_fetch_account_settings,
ToastUtils.Duration.LONG);
break;
case SETTINGS_FETCH_REAUTHORIZATION_REQUIRED_ERROR:
ToastUtils.showToast(getActivity(), R.string.error_disabled_apis,
ToastUtils.Duration.LONG);
break;
case SETTINGS_POST_ERROR:
// We usually rely on event.error.type and provide our own localized message.
// This case is exceptional because:
Expand Down
6 changes: 5 additions & 1 deletion WordPress/src/main/java/org/wordpress/android/ui/prefs/AppSettingsFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,14 @@ public void onAccountChanged(OnAccountChanged event) {

if (event.isError()) {
switch (event.error.type) {
case SETTINGS_FETCH_ERROR:
case SETTINGS_FETCH_GENERIC_ERROR:
ToastUtils
.showToast(getActivity(), R.string.error_fetch_account_settings, ToastUtils.Duration.LONG);
break;
case SETTINGS_FETCH_REAUTHORIZATION_REQUIRED_ERROR:
ToastUtils.showToast(getActivity(), R.string.error_disabled_apis,
ToastUtils.Duration.LONG);
break;
case SETTINGS_POST_ERROR:
ToastUtils.showToast(getActivity(), R.string.error_post_account_settings, ToastUtils.Duration.LONG);
break;
Expand Down
1 change: 1 addition & 0 deletions WordPress/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,6 +1480,7 @@
<string name="error_refresh_unauthorized_posts">You don\'t have permission to view or edit posts</string>
<string name="error_fetch_my_profile">Couldn\'t retrieve your profile</string>
<string name="error_fetch_account_settings">Couldn\'t retrieve your account settings</string>
<string name="error_disabled_apis">Could not fetch settings: Some APIs are disabled for this account + client ID combination.</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some APIs are disabled for this account + client ID combination.

When I saw this, I thought there was something wrong with my actual WordPress user. What do you think about this?

Could not fetch settings: Some APIs are unavailable for this OAuth app ID and client ID combination.

The word "disable" connotes that it can be enabled somewhere but I believe it is impossible or we can't ever allow it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see your point 🤔 but the OAuth app ID and client ID are the same thing. I think OAuth app ID is better for the message than client ID though since that's what we called the param in gradle.properties. 👍

What if we emphasized the 'OAuth app ID' part?

Could not fetch settings: Some APIs are unavailable for this OAuth app ID + account combination.

The word "disable" connotes that it can be enabled somewhere but I believe it is impossible or we can't ever allow it, right?

Basically, yes. I read it more as "works by default but not in this case", but "unavailable" works just as well for me and has less of a "default" connotation, which is maybe better. 👍 Will use it in the final message once we settle on one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, oops. I meant secret instead of client ID there. 😅 But yep, your suggested message is great for me!

Could not fetch settings: Some APIs are unavailable for this OAuth app ID + account combination.

<string name="error_post_my_profile_no_connection">No connection, couldn\'t save your profile</string>
<string name="error_post_account_settings">Couldn\'t save your account settings</string>
<string name="error_generic">An error occurred</string>
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,5 +105,5 @@ buildScan {

ext {
daggerVersion = '2.22.1'
fluxCVersion = '3a44520084671ff0018086933626075b5e323d98'
fluxCVersion = 'a978c7b736e08f0bc7959b1ef6b87d5975abb423'
}
14 changes: 11 additions & 3 deletions ...n/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginBaseFormFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,15 @@
import org.wordpress.android.fluxc.generated.AccountActionBuilder;
import org.wordpress.android.fluxc.generated.SiteActionBuilder;
import org.wordpress.android.fluxc.store.AccountStore;
import org.wordpress.android.fluxc.store.AccountStore.AccountErrorType;
import org.wordpress.android.fluxc.store.AccountStore.OnAccountChanged;
import org.wordpress.android.fluxc.store.SiteStore;
import org.wordpress.android.fluxc.store.SiteStore.OnSiteChanged;
import org.wordpress.android.fluxc.store.SiteStore.SiteErrorType;
import org.wordpress.android.util.AppLog;
import org.wordpress.android.util.EditTextUtils;
import org.wordpress.android.util.ToastUtils;
import org.wordpress.android.util.ToastUtils.Duration;

import javax.inject.Inject;

Expand Down Expand Up @@ -291,9 +293,15 @@ public void onAccountChanged(OnAccountChanged event) {

if (event.isError()) {
AppLog.e(AppLog.T.API, "onAccountChanged has error: " + event.error.type + " - " + event.error.message);
ToastUtils.showToast(getContext(), R.string.error_fetch_my_profile);
onLoginFinished(false);
return;
if (event.error.type == AccountErrorType.SETTINGS_FETCH_REAUTHORIZATION_REQUIRED_ERROR) {
// This probably means we're logging in to 2FA-enabled account with a non-production WP.com client id.
// A few WordPress.com APIs like /me/settings/ won't work for this account.
ToastUtils.showToast(getContext(), R.string.error_disabled_apis, Duration.LONG);
} else {
ToastUtils.showToast(getContext(), R.string.error_fetch_my_profile, Duration.LONG);
onLoginFinished(false);
return;
}
}

if (event.causeOfChange == AccountAction.FETCH_ACCOUNT) {
Expand Down
1 change: 1 addition & 0 deletions libs/login/WordPressLoginFlow/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
<string name="cannot_add_duplicate_site">This site already exists in the app, you can\'t add it.</string>
<string name="duplicate_site_detected">A duplicate site has been detected.</string>
<string name="error_fetch_my_profile">Couldn\'t retrieve your profile</string>
<string name="error_disabled_apis">Could not fetch settings: Some APIs are disabled for this account + client ID combination.</string>
<string name="login_to_to_connect_jetpack">Log in to the WordPress.com account you used to connect Jetpack.</string>
<string name="auth_required">Log in again to continue.</string>
<string name="checking_email">Checking email</string>
Expand Down