From 93339cbbfb86c3bb6bef0b6bb8466713f4251318 Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Sat, 9 May 2015 09:42:48 +1000 Subject: [PATCH] Fix issue 246: Rewrite process for verifying existing repos. Previously it would only compare the details of a new repo to existing ones if the new repo came via an intent. Now it does it whenever you change the text in the new repo dialog (shouldn't be too much of a performance hit, it isn't doing very much). The things it (still) doesn't do is: * verify that the url looks like a url * sanitize the newly input uri and compare it to sanitized saved repos The second point means that you can end up with: http://10.0.1.50/ and http://10.0.1.50 both in the list. I'm going to log an issue for this, because it should be fixed, but doesn't need to hold this up. --- F-Droid/res/layout/addrepo.xml | 9 +- F-Droid/res/values/strings.xml | 6 +- .../fdroid/views/ManageReposActivity.java | 220 +++++++++++++----- 3 files changed, 162 insertions(+), 73 deletions(-) diff --git a/F-Droid/res/layout/addrepo.xml b/F-Droid/res/layout/addrepo.xml index 1fa9ae01f..d94d86a20 100644 --- a/F-Droid/res/layout/addrepo.xml +++ b/F-Droid/res/layout/addrepo.xml @@ -42,13 +42,8 @@ android:id="@+id/overwrite_message" android:layout_width="match_parent" android:layout_height="wrap_content" - android:drawableLeft="@android:drawable/ic_dialog_alert" - android:drawableStart="@android:drawable/ic_dialog_alert" - android:drawablePadding="20dp" - android:padding="20dp" - android:text="@string/repo_delete_to_overwrite" - android:textAppearance="@android:style/TextAppearance.Medium" - android:visibility="gone" /> + android:padding="10dp" + tools:text="This repo is already setup, this will add new key information."/> diff --git a/F-Droid/res/values/strings.xml b/F-Droid/res/values/strings.xml index 7730fc03f..c362c228e 100644 --- a/F-Droid/res/values/strings.xml +++ b/F-Droid/res/values/strings.xml @@ -92,11 +92,11 @@ Repository address Fingerprint (optional) - This repo already exists! + This repo already exists This repo is already setup, this will add new key information. This repo is already setup, confirm that you want to re-enable it. - The incoming repo is already setup and enabled! - You must first delete this repo before you can add one with a different key! + The incoming repo is already setup and enabled. + You must first delete this repo before you can add one with a different key. Ignoring malformed repo URI: %s The list of used repositories has diff --git a/F-Droid/src/org/fdroid/fdroid/views/ManageReposActivity.java b/F-Droid/src/org/fdroid/fdroid/views/ManageReposActivity.java index 32d48da2c..15912d2d1 100644 --- a/F-Droid/src/org/fdroid/fdroid/views/ManageReposActivity.java +++ b/F-Droid/src/org/fdroid/fdroid/views/ManageReposActivity.java @@ -25,6 +25,7 @@ import android.content.Context; import android.content.DialogInterface; import android.content.Intent; import android.content.SharedPreferences; +import android.content.res.ColorStateList; import android.database.Cursor; import android.net.Uri; import android.net.wifi.WifiInfo; @@ -33,6 +34,7 @@ import android.os.AsyncTask; import android.os.Build; import android.os.Bundle; import android.preference.PreferenceManager; +import android.support.annotation.NonNull; import android.support.v4.app.FragmentManager; import android.support.v4.app.ListFragment; import android.support.v4.app.LoaderManager; @@ -40,7 +42,9 @@ import android.support.v4.app.NavUtils; import android.support.v4.content.CursorLoader; import android.support.v4.content.Loader; import android.support.v7.app.ActionBarActivity; +import android.text.Editable; import android.text.TextUtils; +import android.text.TextWatcher; import android.text.format.DateFormat; import android.util.Log; import android.view.Menu; @@ -92,16 +96,17 @@ public class ManageReposActivity extends ActionBarActivity { private static final String DEFAULT_NEW_REPO_TEXT = "https://"; - private enum PositiveAction { - ADD_NEW, ENABLE, IGNORE + private enum AddRepoState { + DOESNT_EXIST, EXISTS_FINGERPRINT_MISMATCH, EXISTS_FINGERPRINT_MATCH, + EXISTS_DISABLED, EXISTS_ENABLED, EXISTS_UPGRADABLE_TO_SIGNED } - private PositiveAction positiveAction; - private UpdateService.UpdateReceiver updateHandler = null; private static boolean changed = false; + private RepoListFragment listFragment; + /** * True if activity started with an intent such as from QR code. False if * opened from, e.g. the main menu. @@ -129,8 +134,10 @@ public class ManageReposActivity extends ActionBarActivity { setContentView(new LinearLayout(this)); } + listFragment = new RepoListFragment(); + fm.beginTransaction() - .add(android.R.id.content, new RepoListFragment()) + .add(android.R.id.content, listFragment) .commit(); } @@ -323,6 +330,12 @@ public class ManageReposActivity extends ActionBarActivity { private final Context context; private final AlertDialog addRepoDialog; + private final TextView overwriteMessage; + private final ColorStateList defaultTextColour; + private final Button addButton; + + private AddRepoState addRepoState; + public AddRepo(String newAddress, String newFingerprint) { context = ManageReposActivity.this; @@ -332,13 +345,6 @@ public class ManageReposActivity extends ActionBarActivity { final EditText uriEditText = (EditText) view.findViewById(R.id.edit_uri); final EditText fingerprintEditText = (EditText) view.findViewById(R.id.edit_fingerprint); - // If the "add new repo" dialog is launched by an action outside of - // FDroid, i.e. a URL, then check to see if any existing repos match, - // and change the action accordingly. - final Repo repo = (newAddress != null && isImportingRepo) - ? RepoProvider.Helper.findByAddress(context, newAddress) - : null; - addRepoDialog.setIcon(android.R.drawable.ic_menu_add); addRepoDialog.setTitle(getString(R.string.repo_add_title)); @@ -382,64 +388,36 @@ public class ManageReposActivity extends ActionBarActivity { public void onClick(View v) { String fp = fingerprintEditText.getText().toString(); + String url = uriEditText.getText().toString(); - // the DB uses null for no fingerprint but the above - // code returns "" rather than null if its blank - if (fp.equals("")) - fp = null; + switch(addRepoState) { + case DOESNT_EXIST: + prepareToCreateNewRepo(url, fp); + break; - if (positiveAction == PositiveAction.ADD_NEW) - prepareToCreateNewRepo(uriEditText.getText().toString(), fp); - else if (positiveAction == PositiveAction.ENABLE) { - enableExistingRepo(repo); - finishedAddingRepo(); + case EXISTS_DISABLED: + case EXISTS_UPGRADABLE_TO_SIGNED: + case EXISTS_FINGERPRINT_MATCH: + updateAndEnableExistingRepo(url, fp); + finishedAddingRepo(); + break; + + default: + finishedAddingRepo(); + break; } } } ); - final TextView overwriteMessage = (TextView) view.findViewById(R.id.overwrite_message); + addButton = addRepoDialog.getButton(DialogInterface.BUTTON_POSITIVE); + overwriteMessage = (TextView) view.findViewById(R.id.overwrite_message); overwriteMessage.setVisibility(View.GONE); - if (repo == null) { - // no existing repo, add based on what we have - positiveAction = PositiveAction.ADD_NEW; - } else { - // found the address in the DB of existing repos - final Button addButton = addRepoDialog.getButton(DialogInterface.BUTTON_POSITIVE); - addRepoDialog.setTitle(R.string.repo_exists); - overwriteMessage.setVisibility(View.VISIBLE); - if (newFingerprint != null) - newFingerprint = newFingerprint.toUpperCase(Locale.ENGLISH); - if (repo.fingerprint == null && newFingerprint != null) { - // we're upgrading from unsigned to signed repo - overwriteMessage.setText(R.string.repo_exists_add_fingerprint); - addButton.setText(R.string.add_key); - positiveAction = PositiveAction.ADD_NEW; - } else if (newFingerprint == null || newFingerprint.equals(repo.fingerprint)) { - // this entry already exists and is not enabled, offer to enable it - if (repo.inuse) { - addRepoDialog.dismiss(); - Toast.makeText(context, R.string.repo_exists_and_enabled, Toast.LENGTH_LONG).show(); - return; - } else { - overwriteMessage.setText(R.string.repo_exists_enable); - addButton.setText(R.string.enable); - positiveAction = PositiveAction.ENABLE; - } - } else { - // same address with different fingerprint, this could be - // malicious, so force the user to manually delete the repo - // before adding this one - overwriteMessage.setTextColor(getResources().getColor(R.color.red)); - overwriteMessage.setText(R.string.repo_delete_to_overwrite); - addButton.setText(R.string.overwrite); - addButton.setEnabled(false); - positiveAction = PositiveAction.IGNORE; - } - } + defaultTextColour = overwriteMessage.getTextColors(); - if (newFingerprint != null) + if (newFingerprint != null) { fingerprintEditText.setText(newFingerprint); + } if (newAddress != null) { // This trick of emptying text then appending, rather than just setting in @@ -447,6 +425,100 @@ public class ManageReposActivity extends ActionBarActivity { uriEditText.setText(""); uriEditText.append(newAddress); } + + final TextWatcher textChangedListener = new TextWatcher() { + + @Override + public void beforeTextChanged(CharSequence s, int start, int count, int after) {} + + @Override + public void onTextChanged(CharSequence s, int start, int before, int count) {} + + @Override + public void afterTextChanged(Editable s) { + validateRepoDetails(uriEditText.getText().toString(), fingerprintEditText.getText().toString()); + } + }; + + uriEditText.addTextChangedListener(textChangedListener); + fingerprintEditText.addTextChangedListener(textChangedListener); + validateRepoDetails(newAddress == null ? "" : newAddress, newFingerprint == null ? "" : newFingerprint); + } + + /** + * Compare the repo and the fingerprint against existing repositories, to see if this + * repo matches and display a relevant message to the user if that is the case. + */ + private void validateRepoDetails(@NonNull final String uri, @NonNull String fingerprint) { + + final Repo repo = uri.length() > 0 ? RepoProvider.Helper.findByAddress(context, uri) : null; + + if (repo == null) { + repoDoesntExist(); + } else { + if (repo.fingerprint == null && fingerprint.length() > 0) { + upgradingToSigned(); + } else if (repo.fingerprint != null && !repo.fingerprint.equalsIgnoreCase(fingerprint)) { + repoFingerprintDoesntMatch(); + } else { + // Could be either an unsigned repo, and no fingerprint was supplied, + // or it could be a signed repo with a matching fingerprint. + if (repo.inuse) { + repoExistsAndEnabled(); + } else { + repoExistsAndDisabled(); + } + } + } + } + + private void repoDoesntExist() { + updateUi(AddRepoState.DOESNT_EXIST, 0, false, R.string.repo_add_add, true); + } + + /** + * Same address with different fingerprint, this could be malicious, so display a message + * force the user to manually delete the repo before adding this one. + */ + private void repoFingerprintDoesntMatch() { + updateUi(AddRepoState.EXISTS_FINGERPRINT_MISMATCH, R.string.repo_delete_to_overwrite, + true, R.string.overwrite, false); + } + + private void repoExistsAndDisabled() { + updateUi(AddRepoState.EXISTS_DISABLED, + R.string.repo_exists_enable, false, R.string.enable, true); + } + + private void repoExistsAndEnabled() { + updateUi(AddRepoState.EXISTS_ENABLED, R.string.repo_exists_and_enabled, false, + R.string.ok, true); + } + + private void upgradingToSigned() { + updateUi(AddRepoState.EXISTS_UPGRADABLE_TO_SIGNED, R.string.repo_exists_add_fingerprint, + false, R.string.add_key, true); + } + + private void updateUi(AddRepoState state, int messageRes, boolean redMessage, int addTextRes, boolean addEnabled) { + if (addRepoState != state) { + addRepoState = state; + + if (messageRes > 0) { + overwriteMessage.setText(messageRes); + overwriteMessage.setVisibility(View.VISIBLE); + if (redMessage) { + overwriteMessage.setTextColor(getResources().getColor(R.color.red)); + } else { + overwriteMessage.setTextColor(defaultTextColour); + } + } else { + overwriteMessage.setVisibility(View.GONE); + } + + addButton.setText(addTextRes); + addButton.setEnabled(addEnabled); + } } /** @@ -548,11 +620,23 @@ public class ManageReposActivity extends ActionBarActivity { /** * Seeing as this repo already exists, we will force it to be enabled again. */ - private void enableExistingRepo(Repo repo) { - ContentValues values = new ContentValues(1); + private void updateAndEnableExistingRepo(String url, String fingerprint) { + if (fingerprint != null) { + fingerprint = fingerprint.trim(); + if (fingerprint.length() == 0) { + fingerprint = null; + } else { + fingerprint = fingerprint.toUpperCase(Locale.ENGLISH); + } + } + + Log.d(TAG, "Enabling existing repo: " + url); + Repo repo = RepoProvider.Helper.findByAddress(context, url); + ContentValues values = new ContentValues(2); values.put(RepoProvider.DataColumns.IN_USE, 1); + values.put(RepoProvider.DataColumns.FINGERPRINT, fingerprint); RepoProvider.Helper.update(context, repo, values); - repo.inuse = true; + listFragment.notifyDataSetChanged(); finishedAddingRepo(); } @@ -743,5 +827,15 @@ public class ManageReposActivity extends ActionBarActivity { intent.putExtra(RepoDetailsFragment.ARG_REPO_ID, repo.getId()); startActivityForResult(intent, SHOW_REPO_DETAILS); } + + /** + * This is necessary because even though the list will listen to content changes + * in the RepoProvider, it doesn't update the list items if they are changed (but not + * added or removed. The example which made this necessary was enabling an existing + * repo, and wanting the switch to be changed to on). + */ + private void notifyDataSetChanged() { + getLoaderManager().restartLoader(0, null, this); + } } }