Mholloway has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/334672 )
Change subject: [Needs discussion] Stop storing password and user ID
......................................................................
[Needs discussion] Stop storing password and user ID
= Password =
We currently store the logged-in user's password in clear text, which
is bad practice. With this patch it's only used to complete the login
flow, and no longer stored.
This means that it's no longer possible to offer the automatic login and
retry functionality when the login session fails while editing a section
or description.
If we want to continue offering this, we should encrypt the user's
credentials for storage.
I suspect we can drop the custom SharedPreferences-based UserInfoStorage
class and interact directly with the platform AccountManager for any such
needs.
= User ID =
This is WIP. Rather than attempting to maintain the numeric user ID
(which varies by wiki) as a bit of state, the iOS app just gets it at the
point of editing. My efforts to update the user ID at the point of
language change have been failing so far, but their approach works, so I'm
going to try implementing it.
Change-Id: Ie5dce0cdd133084e219eb3ea48acb04d460ca34c
---
M app/src/main/java/org/wikipedia/analytics/EditFunnel.java
M app/src/main/java/org/wikipedia/auth/AccountUtil.java
M app/src/main/java/org/wikipedia/descriptions/DescriptionEditFragment.java
M app/src/main/java/org/wikipedia/edit/EditSectionActivity.java
M app/src/main/java/org/wikipedia/login/LoginActivity.java
M app/src/main/java/org/wikipedia/login/LoginClient.java
M app/src/main/java/org/wikipedia/login/User.java
M app/src/main/java/org/wikipedia/login/UserInfoStorage.java
M app/src/main/java/org/wikipedia/settings/Prefs.java
M app/src/main/res/values/preference_keys.xml
M app/src/main/res/xml/developer_preferences.xml
M app/src/test/java/org/wikipedia/login/UserTest.java
12 files changed, 39 insertions(+), 229 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/apps/android/wikipedia
refs/changes/72/334672/1
diff --git a/app/src/main/java/org/wikipedia/analytics/EditFunnel.java
b/app/src/main/java/org/wikipedia/analytics/EditFunnel.java
index 5bbd88c..ca1e263 100644
--- a/app/src/main/java/org/wikipedia/analytics/EditFunnel.java
+++ b/app/src/main/java/org/wikipedia/analytics/EditFunnel.java
@@ -6,7 +6,6 @@
import org.json.JSONObject;
import org.wikipedia.R;
import org.wikipedia.WikipediaApp;
-import org.wikipedia.login.User;
import org.wikipedia.page.PageTitle;
public class EditFunnel extends Funnel {
@@ -150,15 +149,7 @@
@Override
protected JSONObject preprocessData(@NonNull JSONObject eventData) {
- if (User.isLoggedIn()) {
- if (User.getUser().getUserID() == 0) {
- // Means we are logged in, but before we started counting
UserID.
- // Send -1 to record these
- preprocessData(eventData, "userID", -1);
- } else {
- preprocessData(eventData, "userID",
User.getUser().getUserID());
- }
- }
+ // TODO: Get the user ID (via meta=userinfo) from MW API for logging
when making an edit
preprocessData(eventData, "pageNS", title.getNamespace());
return super.preprocessData(eventData);
}
diff --git a/app/src/main/java/org/wikipedia/auth/AccountUtil.java
b/app/src/main/java/org/wikipedia/auth/AccountUtil.java
index 11e0492..b3f13a7 100644
--- a/app/src/main/java/org/wikipedia/auth/AccountUtil.java
+++ b/app/src/main/java/org/wikipedia/auth/AccountUtil.java
@@ -19,15 +19,17 @@
public static void createAccountForLoggedInUser() {
User user = User.getUser();
if (user != null && account() == null) {
- createAccount(null, user.getUsername(), user.getPassword());
+ createAccount(null, user.getUsername());
}
}
- public static void createAccount(@Nullable AccountAuthenticatorResponse
response,
- String username, String password) {
+ public static void createAccount(@Nullable AccountAuthenticatorResponse
response, String username) {
+ // Create an account without storing the password. If, in the future,
we need to store the
+ // password because we want to offer something like one-click logins,
let's encrypt this
+ // info. AccountManager doesn't do it on its own.
Account account = new Account(username, accountType());
- boolean created = accountManager().addAccountExplicitly(account,
password, null);
+ boolean created = accountManager().addAccountExplicitly(account, null,
null);
L.i("account creation " + (created ? "successful" : "failure"));
@@ -73,14 +75,12 @@
return account.equals(AccountUtil.account());
}
- @Nullable
- public static Account account() {
+ @Nullable public static Account account() {
User user = User.getUser();
return user == null ? null : account(user.getUsername());
}
- @Nullable
- private static Account account(@NonNull String username) {
+ @Nullable private static Account account(@NonNull String username) {
try {
Account[] accounts =
accountManager().getAccountsByType(accountType());
for (Account account : accounts) {
@@ -94,8 +94,7 @@
return null;
}
- @NonNull
- public static String accountType() {
+ @NonNull public static String accountType() {
return app().getString(R.string.account_type);
}
@@ -103,8 +102,7 @@
return AccountManager.get(app());
}
- @NonNull
- private static WikipediaApp app() {
+ @NonNull private static WikipediaApp app() {
return WikipediaApp.getInstance();
}
diff --git
a/app/src/main/java/org/wikipedia/descriptions/DescriptionEditFragment.java
b/app/src/main/java/org/wikipedia/descriptions/DescriptionEditFragment.java
index 57f66fb..85eb43e 100644
--- a/app/src/main/java/org/wikipedia/descriptions/DescriptionEditFragment.java
+++ b/app/src/main/java/org/wikipedia/descriptions/DescriptionEditFragment.java
@@ -19,9 +19,6 @@
import org.wikipedia.dataclient.WikiSite;
import org.wikipedia.json.GsonMarshaller;
import org.wikipedia.json.GsonUnmarshaller;
-import org.wikipedia.login.LoginClient;
-import org.wikipedia.login.LoginClient.LoginFailedException;
-import org.wikipedia.login.LoginResult;
import org.wikipedia.login.User;
import org.wikipedia.page.PageTitle;
import org.wikipedia.settings.Prefs;
@@ -139,10 +136,7 @@
}
private class EditViewCallback implements DescriptionEditView.Callback {
- private static final int MAX_RETRIES = 1;
- private int retries = 0;
private final WikiSite wikiData = new WikiSite("www.wikidata.org", "");
- private final WikiSite loginWiki = pageTitle.getWikiSite();
@Override
public void onSaveClick() {
@@ -165,59 +159,6 @@
@Override public void failure(@NonNull Call<CsrfToken>
call,
@NonNull Throwable
caught) {
L.w("could not get edit token: ", caught);
- if (retries < MAX_RETRIES && User.getUser() !=
null) {
- retries++;
- refreshLoginTokens(User.getUser(), new
RetryCallback() {
- @Override public void retry() {
- L.i("retrying...");
- requestEditToken();
- }
- });
- } else {
- editFailed(caught);
- }
- }
- });
- }
-
- /**
- * Refresh all tokens/cookies, then retry the previous request.
- * This is needed when the edit token was not retrieved because the
login session expired.
- *
- * @param user user info to be able to log in
- * @param retryCallback a repeat of the action which failed before
- * @throws IllegalArgumentException if user is not logged in
- */
- private void refreshLoginTokens(User user, @NonNull RetryCallback
retryCallback) {
- WikipediaApp app = WikipediaApp.getInstance();
- app.getCsrfTokenStorage().clearAllTokens();
- app.getCookieManager().clearAllCookies();
-
- login(user, retryCallback);
- }
-
- private void login(@NonNull final User user, @NonNull final
RetryCallback retryCallback) {
- new LoginClient().request(loginWiki,
- user.getUsername(),
- user.getPassword(),
- new LoginClient.LoginCallback() {
- @Override
- public void success(@NonNull LoginResult loginResult) {
- if (loginResult.pass()) {
- retryCallback.retry();
- } else {
- editFailed(new
LoginFailedException(loginResult.getMessage()));
- }
- }
-
- @Override
- public void twoFactorPrompt(@NonNull Throwable caught,
@Nullable String token) {
- editFailed(new LoginFailedException(getResources()
-
.getString(R.string.login_2fa_other_workflow_error_msg)));
- }
-
- @Override
- public void error(@NonNull Throwable caught) {
editFailed(caught);
}
});
@@ -265,9 +206,5 @@
public void onCancelClick() {
finish();
}
- }
-
- private interface RetryCallback {
- void retry();
}
}
\ No newline at end of file
diff --git a/app/src/main/java/org/wikipedia/edit/EditSectionActivity.java
b/app/src/main/java/org/wikipedia/edit/EditSectionActivity.java
index c18d37a..592b9e1 100644
--- a/app/src/main/java/org/wikipedia/edit/EditSectionActivity.java
+++ b/app/src/main/java/org/wikipedia/edit/EditSectionActivity.java
@@ -45,8 +45,6 @@
import org.wikipedia.edit.richtext.SyntaxHighlighter;
import org.wikipedia.edit.summaries.EditSummaryFragment;
import org.wikipedia.login.LoginActivity;
-import org.wikipedia.login.LoginClient;
-import org.wikipedia.login.LoginResult;
import org.wikipedia.login.User;
import org.wikipedia.page.LinkMovementMethodExt;
import org.wikipedia.page.PageProperties;
@@ -355,8 +353,6 @@
if (caught instanceof ApiException) {
// This is a fairly standard editing
exception. Handle it appropriately.
handleEditingException((ApiException)
caught);
- } else if (caught instanceof
UserNotLoggedInException) {
- retry();
} else {
// If it's not an API exception, we have
no idea what's wrong.
// Show the user a generic error message.
@@ -397,24 +393,13 @@
retryDialog.show();
}
- private void retry() {
- // looks like our session expired.
- app.getCsrfTokenStorage().clearAllTokens();
- app.getCookieManager().clearAllCookies();
-
- User user = User.getUser();
- doLoginAndSave(user);
- }
-
/**
* Processes API error codes encountered during editing, and handles them
as appropriate.
* @param e The ApiException to handle.
*/
private void handleEditingException(@NonNull ApiException e) {
String code = e.getCode();
- if (User.isLoggedIn() && ("badtoken".equals(code) ||
"assertuserfailed".equals(code))) {
- retry();
- } else if ("blocked".equals(code) ||
"wikimedia-globalblocking-ipblocked".equals(code)) {
+ if ("blocked".equals(code) ||
"wikimedia-globalblocking-ipblocked".equals(code)) {
// User is blocked, locally or globally
// If they were anon, canedit does not catch this, so we can't
show them the locked pencil
// If they not anon, this means they were blocked in the interim
between opening the edit
@@ -454,49 +439,6 @@
progressDialog.dismiss();
FeedbackUtil.showError(this, e);
}
- }
-
- private void doLoginAndSave(final User user) {
- new LoginClient().request(WikipediaApp.getInstance().getWikiSite(),
- user.getUsername(),
- user.getPassword(),
- new LoginClient.LoginCallback() {
- @Override
- public void success(@NonNull LoginResult result) {
- if (result.pass()) {
- doSave();
- } else {
- onLoginError();
- }
- }
-
- @Override
- public void twoFactorPrompt(@NonNull Throwable caught,
@Nullable String token) {
- FeedbackUtil.showError(EditSectionActivity.this,
- new
LoginClient.LoginFailedException(getResources()
-
.getString(R.string.login_2fa_other_workflow_error_msg)));
- onLoginError();
- }
-
- @Override
- public void error(@NonNull Throwable caught) {
- onLoginError();
- }
-
- private void onLoginError() {
- if (!progressDialog.isShowing()) {
- // no longer attached to activity!
- return;
- }
- progressDialog.dismiss();
-
- // TODO: The view shown here states "cannot connect to
the Internet" regardless
- // of the actual cause of failure (which could also be
an API error or malformed
- // response). Update this to provide more specificity
and accuracy. (T154805)
- ViewAnimations.crossFade(sectionText, sectionError);
- sectionError.setVisibility(View.VISIBLE);
- }
- });
}
private void handleAbuseFilter() {
diff --git a/app/src/main/java/org/wikipedia/login/LoginActivity.java
b/app/src/main/java/org/wikipedia/login/LoginActivity.java
index 4a4c26f..beb92d0 100644
--- a/app/src/main/java/org/wikipedia/login/LoginActivity.java
+++ b/app/src/main/java/org/wikipedia/login/LoginActivity.java
@@ -218,15 +218,14 @@
if (!twoFactorCode.isEmpty()) {
loginClient.login(WikipediaApp.getInstance().getWikiSite(),
username, password,
- twoFactorCode, firstStepToken, getCallback(username,
password));
+ twoFactorCode, firstStepToken, getCallback(username));
} else {
loginClient.request(WikipediaApp.getInstance().getWikiSite(),
username, password,
- getCallback(username, password));
+ getCallback(username));
}
}
- private LoginClient.LoginCallback getCallback(@NonNull final String
username,
- @NonNull final String
password) {
+ private LoginClient.LoginCallback getCallback(@NonNull final String
username) {
return new LoginClient.LoginCallback() {
@Override
public void success(@NonNull LoginResult result) {
@@ -242,7 +241,7 @@
AccountAuthenticatorResponse response = extras == null
? null
:
extras.<AccountAuthenticatorResponse>getParcelable(AccountManager.KEY_ACCOUNT_AUTHENTICATOR_RESPONSE);
- AccountUtil.createAccount(response, username, password);
+ AccountUtil.createAccount(response, username);
hideSoftKeyboard(LoginActivity.this);
setResult(RESULT_LOGIN_SUCCESS);
diff --git a/app/src/main/java/org/wikipedia/login/LoginClient.java
b/app/src/main/java/org/wikipedia/login/LoginClient.java
index 6909692..dbf6dc1 100644
--- a/app/src/main/java/org/wikipedia/login/LoginClient.java
+++ b/app/src/main/java/org/wikipedia/login/LoginClient.java
@@ -89,7 +89,7 @@
public void onResponse(Call<LoginResponse> call,
Response<LoginResponse> response) {
if (response.isSuccessful()) {
LoginResponse loginResponse = response.body();
- LoginResult loginResult =
loginResponse.toLoginResult(password);
+ LoginResult loginResult = loginResponse.toLoginResult();
if (loginResult != null) {
if (loginResult.pass() && loginResult.getUser() !=
null) {
// The server could do some transformations on
user names, e.g. on some
@@ -208,8 +208,8 @@
return error;
}
- LoginResult toLoginResult(String password) {
- return clientLogin != null ? clientLogin.toLoginResult(password) :
null;
+ LoginResult toLoginResult() {
+ return clientLogin != null ? clientLogin.toLoginResult() : null;
}
private static class ClientLogin {
@@ -218,11 +218,11 @@
@SerializedName("message") @Nullable private String message;
@SerializedName("username") @Nullable private String userName;
- LoginResult toLoginResult(String password) {
+ LoginResult toLoginResult() {
User user = null;
String userMessage = null;
if ("PASS".equals(status)) {
- user = new User(userName, password, 0);
+ user = new User(userName);
} else if ("FAIL".equals(status)) {
userMessage = message;
} else if ("UI".equals(status)) {
diff --git a/app/src/main/java/org/wikipedia/login/User.java
b/app/src/main/java/org/wikipedia/login/User.java
index b379c60..4929643 100644
--- a/app/src/main/java/org/wikipedia/login/User.java
+++ b/app/src/main/java/org/wikipedia/login/User.java
@@ -19,8 +19,7 @@
}
}
- @Nullable
- public static User getUser() {
+ @Nullable public static User getUser() {
if (CURRENT_USER == null && STORAGE != null) {
CURRENT_USER = STORAGE.getUser();
}
@@ -41,29 +40,24 @@
/**
* Call this before instrumentation tests run
*/
- @VisibleForTesting
- public static void disableStorage() {
+ @VisibleForTesting static void disableStorage() {
STORAGE = null;
}
@NonNull private final String username;
- @NonNull private final String password;
- private final int userID;
@NonNull private final Set<String> groups;
- public User(@NonNull String username, @NonNull String password, int
userID) {
- this(username, password, userID, null);
+ public User(@NonNull String username) {
+ this(username, null);
}
public User(@NonNull User other, @Nullable Set<String> groups) {
- this(other.username, other.password, other.userID, groups);
+ this(other.username, groups);
}
- public User(@NonNull String username, @NonNull String password, int userID,
- @Nullable Set<String> groups) {
+ public User(@NonNull String username, @Nullable Set<String> groups) {
this.username = username;
- this.password = password;
- this.userID = userID;
+
if (groups != null) {
this.groups = Collections.unmodifiableSet(new HashSet<>(groups));
} else {
@@ -71,18 +65,16 @@
}
}
+ // Legacy constructor from when we maintained the password and numeric ID
as part of the User
+ // state. We need to keep it around (at least for a while) to keep from
borking existing
+ // clients that have a legacy-style User stored in SharedPreferences.
+ //public User(@NonNull String username, @NonNull String password, int
userID, @Nullable Set<String> groups) {
+ // this(username, groups);
+ //}
+
@NonNull
public String getUsername() {
return username;
- }
-
- @NonNull
- public String getPassword() {
- return password;
- }
-
- public int getUserID() {
- return userID;
}
public boolean isAllowed(@NonNull Set<String> allowedGroups) {
diff --git a/app/src/main/java/org/wikipedia/login/UserInfoStorage.java
b/app/src/main/java/org/wikipedia/login/UserInfoStorage.java
index a0c6f6b..39bedac 100644
--- a/app/src/main/java/org/wikipedia/login/UserInfoStorage.java
+++ b/app/src/main/java/org/wikipedia/login/UserInfoStorage.java
@@ -9,29 +9,19 @@
void setUser(@NonNull User user) {
Prefs.setLoginUsername(user.getUsername());
- Prefs.setLoginPassword(user.getPassword());
- Prefs.setLoginUserId(user.getUserID());
Prefs.setLoginGroups(user.getGroupMemberships());
}
- @Nullable
- User getUser() {
- if (Prefs.hasLoginUsername() && Prefs.hasLoginPassword()) {
+ @Nullable User getUser() {
+ if (Prefs.hasLoginUsername()) {
//noinspection ConstantConditions
- return new User(
- Prefs.getLoginUsername(),
- Prefs.getLoginPassword(),
- Prefs.getLoginUserId(),
- Prefs.getLoginGroups()
- );
+ return new User(Prefs.getLoginUsername(), Prefs.getLoginGroups());
}
return null;
}
void clearUser() {
Prefs.removeLoginUsername();
- Prefs.removeLoginPassword();
- Prefs.removeLoginUserId();
Prefs.removeLoginGroups();
}
}
diff --git a/app/src/main/java/org/wikipedia/settings/Prefs.java
b/app/src/main/java/org/wikipedia/settings/Prefs.java
index c43c127..0089818 100644
--- a/app/src/main/java/org/wikipedia/settings/Prefs.java
+++ b/app/src/main/java/org/wikipedia/settings/Prefs.java
@@ -156,35 +156,6 @@
}
@Nullable
- public static String getLoginPassword() {
- return getString(R.string.preference_key_login_password, null);
- }
-
- public static void setLoginPassword(@Nullable String password) {
- setString(R.string.preference_key_login_password, password);
- }
-
- public static boolean hasLoginPassword() {
- return contains(R.string.preference_key_login_password);
- }
-
- public static void removeLoginPassword() {
- remove(R.string.preference_key_login_password);
- }
-
- public static int getLoginUserId() {
- return getInt(R.string.preference_key_login_user_id, 0);
- }
-
- public static void setLoginUserId(int id) {
- setInt(R.string.preference_key_login_user_id, id);
- }
-
- public static void removeLoginUserId() {
- remove(R.string.preference_key_login_user_id);
- }
-
- @Nullable
public static String getLoginUsername() {
return getString(R.string.preference_key_login_username, null);
}
diff --git a/app/src/main/res/values/preference_keys.xml
b/app/src/main/res/values/preference_keys.xml
index 552057a..241f91e 100644
--- a/app/src/main/res/values/preference_keys.xml
+++ b/app/src/main/res/values/preference_keys.xml
@@ -30,8 +30,6 @@
<string name="preference_key_retrofit_log_level">retrofitLog</string>
<string
name="preference_key_daily_event_time_task_name">dailyEventTask</string>
<string name="preference_key_login_username">username</string>
- <string name="preference_key_login_password">password</string>
- <string name="preference_key_login_user_id">userID</string>
<string name="preference_key_login_groups">groups</string>
<string
name="preference_key_show_developer_settings">showDeveloperSettings</string>
<string name="preference_key_last_run_time_format">%s-lastrun</string>
diff --git a/app/src/main/res/xml/developer_preferences.xml
b/app/src/main/res/xml/developer_preferences.xml
index 003d9d9..b79d40a 100644
--- a/app/src/main/res/xml/developer_preferences.xml
+++ b/app/src/main/res/xml/developer_preferences.xml
@@ -94,11 +94,6 @@
android:key="@string/preference_key_reading_app_install_id"
android:title="@string/preference_key_reading_app_install_id" />
- <org.wikipedia.settings.IntPreference
- style="@style/IntPreference"
- android:key="@string/preference_key_login_user_id"
- android:title="@string/preference_key_login_user_id" />
-
<org.wikipedia.settings.EditTextAutoSummarizePreference
style="@style/DataStringPreference"
android:key="@string/preference_key_language"
diff --git a/app/src/test/java/org/wikipedia/login/UserTest.java
b/app/src/test/java/org/wikipedia/login/UserTest.java
index fbe2935..0780d19 100644
--- a/app/src/test/java/org/wikipedia/login/UserTest.java
+++ b/app/src/test/java/org/wikipedia/login/UserTest.java
@@ -16,7 +16,6 @@
@RunWith(TestRunner.class)
public class UserTest {
- private static final int USER_ID = 333;
private static final Set<String> GROUPS
= Collections.unmodifiableSet(new HashSet<>(Arrays.asList("*",
"user", "autoconfirmed")));
@@ -24,7 +23,7 @@
public void setUp() {
User.disableStorage();
- User user = new User("name", "pwd", USER_ID, GROUPS);
+ User user = new User("name", GROUPS);
User.setUser(user);
}
@@ -33,8 +32,6 @@
User user = User.getUser();
//noinspection ConstantConditions
assertThat(user.getUsername(), is("name"));
- assertThat(user.getPassword(), is("pwd"));
- assertThat(user.getUserID(), is(USER_ID));
assertThat(user.getGroupMemberships(), is(GROUPS));
}
--
To view, visit https://gerrit.wikimedia.org/r/334672
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie5dce0cdd133084e219eb3ea48acb04d460ca34c
Gerrit-PatchSet: 1
Gerrit-Project: apps/android/wikipedia
Gerrit-Branch: master
Gerrit-Owner: Mholloway <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
