Skip to content

Migrate jobs to new user model #5447

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

Merged
tobiasKaminsky merged 2 commits into from
Feb 17, 2020
Merged

Migrate jobs to new user model #5447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0afc264
Migrated jobs to new user model
ezaquarii Feb 10, 2020
7b4e1b8
change cancelContactBackupJobForAccount to user new User class
tobiasKaminsky Feb 14, 2020
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
8 changes: 8 additions & 0 deletions src/main/java/com/nextcloud/client/account/UserAccountManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ public interface UserAccountManager extends CurrentAccountProvider {
*/
void removeAllAccounts();

/**
* Remove registered user.
*
* @param user user to remove
* @return true if account was removed successfully, false otherwise
*/
boolean removeUser(User user);

/**
* Get configured NextCloud's user accounts.
*
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/com/nextcloud/client/account/UserAccountManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
import android.accounts.Account;
import android.accounts.AccountManager;
import android.app.Activity;
import android.accounts.AccountManagerFuture;
import android.accounts.AuthenticatorException;
import android.accounts.OperationCanceledException;
import android.content.Context;
import android.content.SharedPreferences;
import android.preference.PreferenceManager;
Expand All @@ -43,6 +46,7 @@
import com.owncloud.android.lib.resources.status.OwnCloudVersion;
import com.owncloud.android.lib.resources.users.GetUserInfoRemoteOperation;

import java.io.IOException;
import java.net.URI;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -81,6 +85,18 @@ public void removeAllAccounts() {
}
}

@Override
public boolean removeUser(User user) {
try {
AccountManagerFuture<Boolean> result = accountManager.removeAccount(user.toPlatformAccount(),
null,
null);
return result.getResult();
} catch (OperationCanceledException| AuthenticatorException| IOException ex) {
return false;
}
}

@Override
@NonNull
public Account[] getAccounts() {
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/com/nextcloud/client/di/AppModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@
import com.owncloud.android.ui.activities.data.files.FilesServiceApiImpl;
import com.owncloud.android.ui.activities.data.files.RemoteFilesRepository;

import org.greenrobot.eventbus.EventBus;

import java.io.File;

import javax.inject.Singleton;
Expand Down Expand Up @@ -164,4 +166,10 @@ NotificationManager notificationManager(Context context) {
AudioManager audioManager(Context context) {
return (AudioManager)context.getSystemService(Context.AUDIO_SERVICE);
}

@Provides
@Singleton
EventBus eventBus() {
return EventBus.getDefault();
}
}
7 changes: 6 additions & 1 deletion src/main/java/com/owncloud/android/MainApp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
import com.owncloud.android.utils.SecurityUtils;

import org.conscrypt.Conscrypt;
import org.greenrobot.eventbus.EventBus;

import java.lang.reflect.Method;
import java.security.NoSuchAlgorithmException;
Expand Down Expand Up @@ -166,6 +167,9 @@ public class MainApp extends MultiDexApplication implements HasAndroidInjector {
@Inject
Clock clock;

@Inject
EventBus eventBus;

private PassCodeManager passCodeManager;

@SuppressWarnings("unused")
Expand Down Expand Up @@ -274,7 +278,8 @@ public void onCreate() {
uploadsStorageManager,
connectivityService,
powerManagementService,
clock
clock,
eventBus
)
);

Expand Down
165 changes: 82 additions & 83 deletions src/main/java/com/owncloud/android/jobs/AccountRemovalJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@

import android.accounts.Account;
import android.accounts.AccountManager;
import android.accounts.AccountManagerCallback;
import android.accounts.AccountManagerFuture;
import android.content.Context;
import android.net.Uri;
import android.os.Build;
Expand All @@ -37,9 +35,11 @@
import com.evernote.android.job.Job;
import com.evernote.android.job.util.support.PersistableBundleCompat;
import com.google.gson.Gson;
import com.nextcloud.client.account.User;
import com.nextcloud.client.account.UserAccountManager;
import com.nextcloud.client.core.Clock;
import com.nextcloud.client.preferences.AppPreferencesImpl;
import com.nextcloud.java.util.Optional;
import com.owncloud.android.MainApp;
import com.owncloud.android.R;
import com.owncloud.android.datamodel.ArbitraryDataProvider;
Expand All @@ -49,8 +49,8 @@
import com.owncloud.android.datamodel.SyncedFolder;
import com.owncloud.android.datamodel.SyncedFolderProvider;
import com.owncloud.android.datamodel.UploadsStorageManager;
import com.owncloud.android.lib.common.OwnCloudAccount;
import com.owncloud.android.lib.common.OwnCloudClient;
import com.owncloud.android.lib.common.OwnCloudClientManager;
import com.owncloud.android.lib.common.OwnCloudClientManagerFactory;
import com.owncloud.android.lib.common.utils.Log_OC;
import com.owncloud.android.lib.resources.users.RemoteWipeSuccessRemoteOperation;
Expand All @@ -67,108 +67,128 @@
import java.util.List;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;

import static android.content.Context.ACCOUNT_SERVICE;
import static com.owncloud.android.ui.activity.ManageAccountsActivity.PENDING_FOR_REMOVAL;

/**
* Removes account and all local files
*/
public class AccountRemovalJob extends Job implements AccountManagerCallback<Boolean> {
public class AccountRemovalJob extends Job {
public static final String TAG = "AccountRemovalJob";
public static final String ACCOUNT = "account";
public static final String REMOTE_WIPE = "remote_wipe";

private final UploadsStorageManager uploadsStorageManager;
private final UserAccountManager userAccountManager;
private final Clock clock;
private final EventBus eventBus;
Copy link
Member

@tobiasKaminsky tobiasKaminsky Feb 14, 2020

Choose a reason for hiding this comment

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

I know that this was used before, so I am totally fine to re-use this.
But as our approach as to get rid of 3rd party libs, we should think about removing EventBus and evernote Job scheduling, but rely on WorkManager and
[https://developer.android.com/topic/libraries/architecture/workmanager/how-to/states-and-observation#observing](observing your work)

But let us do this in a following PR, please :-)

Copy link
Contributor

@mario mario Feb 14, 2020

Choose a reason for hiding this comment

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

I understand the evernote thing, but why is it that we hate eventbus these days? It's a relatively established and long-living project that won't disappear tomorrow.

Copy link
Collaborator Author

@ezaquarii ezaquarii Feb 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@tobiasKaminsky I didn't add it - I just extracted a dependency that was explicitly created in a job constructor.

@mario Good question!

EventBus is not a bad piece of engineering in itself. However, how it's being used in practice is another story and mileage will vary.

I wouldn't say I personally hate it. Event bus design pattern has it's place. I think that - as usual with engineering concepts - some people observed abuses and spoke loud about them. Some were more mature at verbalizing the issues, some - maybe pursuing social maedia traction - just vented out anger calling ppl names.

If we ask ourselves "why" in a serious manner, I can find few issues with event bus pattern in general, in no relation to Nextcloud app:

  1. it's very temping to inroduce hidden tight coupling between componets (no direct source-level dependency, but dependencies caused by complex event patterns)
  2. cascade of event posts makes callstack analysis challenging; it becomes very tricky if events are sent bi-directionally between many actors; this is what makes contribues seriously to microservices debugging difficulty and market is raging with fancy distributed-log-as-a-service solutions
  3. event storms :-)
  4. upredictable and untraceable code paths - your component can be called from many places

Those issues don't necessarily make event bus an anti-pattern, but migh influence a decision of using it or not. It can surely save you some typing, but typing is not a bottleneck in more complex systems. Also, at certain level of complexity, team/company social considerations cannot be dismissed lightly and more involving patterns, but requiring less engineering discipline might be preferred over more convenient ones, but requiring more informed or governed approach.

BTW, we have LocalBroadcastManager that can serve this role as well. The drawback of LMB is that it requires Android platform in tests, so I wouldn't be so sure at all that we are anxious to remove EventBus. :-)

Copy link
Contributor

@mario mario Feb 16, 2020

Choose a reason for hiding this comment

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

I am aware of "people" complaining and the bad patterns surrounding EventBus, some of which are also inside Nextcloud (not the least, some written by me). Nevertheless, I really appreciate you taking the time to explain your thoughts in detail. Thank you once again for being awesome!


public AccountRemovalJob(UploadsStorageManager uploadStorageManager, UserAccountManager accountManager, Clock clock) {
public AccountRemovalJob(UploadsStorageManager uploadStorageManager,
UserAccountManager accountManager,
Clock clock,
EventBus eventBus) {
this.uploadsStorageManager = uploadStorageManager;
this.userAccountManager = accountManager;
this.clock = clock;
this.eventBus = eventBus;
}

@NonNull
@Override
protected Result onRunJob(@NonNull Params params) {
Context context = MainApp.getAppContext();
PersistableBundleCompat bundle = params.getExtras();
Account account = userAccountManager.getAccountByName(bundle.getString(ACCOUNT, ""));
String accountName = bundle.getString(ACCOUNT, "");
if (TextUtils.isEmpty(accountName)) {
// didn't receive account to delete
return Result.FAILURE;
}
Optional<User> optionalUser = userAccountManager.getUser(accountName);
if (!optionalUser.isPresent()) {
// trying to delete non-existing user
return Result.FAILURE;
}
AccountManager accountManager = (AccountManager) context.getSystemService(ACCOUNT_SERVICE);
if (accountManager == null) {
return Result.FAILURE;
}
boolean remoteWipe = bundle.getBoolean(REMOTE_WIPE, false);

if (account != null && accountManager != null) {
ArbitraryDataProvider arbitraryDataProvider = new ArbitraryDataProvider(context.getContentResolver());
ArbitraryDataProvider arbitraryDataProvider = new ArbitraryDataProvider(context.getContentResolver());

// disable contact backup job
ContactsPreferenceActivity.cancelContactBackupJobForAccount(context, account);
User user = optionalUser.get();
// disable contact backup job
ContactsPreferenceActivity.cancelContactBackupJobForAccount(context, user);

removeAccount(account, accountManager);
final boolean userRemoved = userAccountManager.removeUser(user);
if (userRemoved) {
eventBus.post(new AccountRemovedEvent());
}

FileDataStorageManager storageManager = new FileDataStorageManager(account, context.getContentResolver());
FileDataStorageManager storageManager = new FileDataStorageManager(user.toPlatformAccount(), context.getContentResolver());

// remove all files
removeFiles(account, storageManager);
// remove all files
removeFiles(user, storageManager);

// delete all database entries
storageManager.deleteAllFiles();
// delete all database entries
storageManager.deleteAllFiles();

// remove contact backup job
ContactsPreferenceActivity.cancelContactBackupJobForAccount(context, account);
// remove contact backup job
ContactsPreferenceActivity.cancelContactBackupJobForAccount(context, user);

// disable daily backup
arbitraryDataProvider.storeOrUpdateKeyValue(account.name,
ContactsPreferenceActivity.PREFERENCE_CONTACTS_AUTOMATIC_BACKUP,
"false");
// disable daily backup
arbitraryDataProvider.storeOrUpdateKeyValue(user.getAccountName(),
ContactsPreferenceActivity.PREFERENCE_CONTACTS_AUTOMATIC_BACKUP,
"false");

// unregister push notifications
unregisterPushNotifications(context, account, arbitraryDataProvider);
// unregister push notifications
unregisterPushNotifications(context, user, arbitraryDataProvider);

// remove pending account removal
arbitraryDataProvider.deleteKeyForAccount(account.name, PENDING_FOR_REMOVAL);
// remove pending account removal
arbitraryDataProvider.deleteKeyForAccount(user.getAccountName(), PENDING_FOR_REMOVAL);

// remove synced folders set for account
remoceSyncedFolders(context, account, clock);
// remove synced folders set for account
remoceSyncedFolders(context, user.toPlatformAccount(), clock);

// delete all uploads for account
uploadsStorageManager.removeAccountUploads(account);
// delete all uploads for account
uploadsStorageManager.removeAccountUploads(user.toPlatformAccount());

// delete stored E2E keys
arbitraryDataProvider.deleteKeyForAccount(account.name, EncryptionUtils.PRIVATE_KEY);
arbitraryDataProvider.deleteKeyForAccount(account.name, EncryptionUtils.PUBLIC_KEY);
// delete stored E2E keys
arbitraryDataProvider.deleteKeyForAccount(user.getAccountName(), EncryptionUtils.PRIVATE_KEY);
arbitraryDataProvider.deleteKeyForAccount(user.getAccountName(), EncryptionUtils.PUBLIC_KEY);

OwnCloudClient client = createClient(account);
if (remoteWipe && client != null) {
if (remoteWipe) {
Optional<OwnCloudClient> optionalClient = createClient(user);
if (optionalClient.isPresent()) {
OwnCloudClient client = optionalClient.get();
String authToken = client.getCredentials().getAuthToken();
new RemoteWipeSuccessRemoteOperation(authToken).execute(client);
}
}

// notify Document Provider
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
String authority = context.getResources().getString(R.string.document_provider_authority);
Uri rootsUri = DocumentsContract.buildRootsUri(authority);
context.getContentResolver().notifyChange(rootsUri, null);
}

return Result.SUCCESS;
} else {
return Result.FAILURE;
// notify Document Provider
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
String authority = context.getResources().getString(R.string.document_provider_authority);
Uri rootsUri = DocumentsContract.buildRootsUri(authority);
context.getContentResolver().notifyChange(rootsUri, null);
}
}

private void unregisterPushNotifications(Context context, Account account, ArbitraryDataProvider arbitraryDataProvider) {
String arbitraryDataPushString;
return Result.SUCCESS;
}

if (!TextUtils.isEmpty(arbitraryDataPushString = arbitraryDataProvider.getValue(
account, PushUtils.KEY_PUSH)) &&
!TextUtils.isEmpty(context.getResources().getString(R.string.push_server_url))) {
private void unregisterPushNotifications(Context context,
User user,
ArbitraryDataProvider arbitraryDataProvider) {
final String arbitraryDataPushString = arbitraryDataProvider.getValue(user.toPlatformAccount(),
PushUtils.KEY_PUSH);
final String pushServerUrl = context.getResources().getString(R.string.push_server_url);
if (!TextUtils.isEmpty(arbitraryDataPushString) && !TextUtils.isEmpty(pushServerUrl)) {
Gson gson = new Gson();
PushConfigurationState pushArbitraryData = gson.fromJson(arbitraryDataPushString,
PushConfigurationState.class);
pushArbitraryData.setShouldBeDeleted(true);
arbitraryDataProvider.storeOrUpdateKeyValue(account.name, PushUtils.KEY_PUSH,
arbitraryDataProvider.storeOrUpdateKeyValue(user.getAccountName(), PushUtils.KEY_PUSH,
gson.toJson(pushArbitraryData));

PushUtils.pushRegistrationToServer(userAccountManager, pushArbitraryData.getPushToken());
Expand Down Expand Up @@ -198,44 +218,23 @@ private void remoceSyncedFolders(Context context, Account account, Clock clock)
}
}

private void removeFiles(Account account, FileDataStorageManager storageManager) {
File tempDir = new File(FileStorageUtils.getTemporalPath(account.name));
File saveDir = new File(FileStorageUtils.getSavePath(account.name));
private void removeFiles(User user, FileDataStorageManager storageManager) {
File tempDir = new File(FileStorageUtils.getTemporalPath(user.getAccountName()));
File saveDir = new File(FileStorageUtils.getSavePath(user.getAccountName()));

FileStorageUtils.deleteRecursively(tempDir, storageManager);
FileStorageUtils.deleteRecursively(saveDir, storageManager);
}

private void removeAccount(Account account, AccountManager accountManager) {
try {
AccountManagerFuture<Boolean> accountRemoval = accountManager.removeAccount(account, this, null);
boolean removal = accountRemoval.getResult();

if (!removal) {
Log_OC.e(this, "Account removal of " + account.name + " failed!");
}
} catch (Exception e) {
Log_OC.e(this, "Account removal of " + account.name + " failed!", e);
}
}

@Nullable
private OwnCloudClient createClient(Account account) {
OwnCloudClient client = null;
private Optional<OwnCloudClient> createClient(User user) {
try {
OwnCloudAccount ocAccount = new OwnCloudAccount(account, MainApp.getAppContext());
client = OwnCloudClientManagerFactory.getDefaultSingleton().getClientFor(ocAccount,
MainApp.getAppContext());
Context context = MainApp.getAppContext();
OwnCloudClientManager factory = OwnCloudClientManagerFactory.getDefaultSingleton();
OwnCloudClient client = factory.getClientFor(user.toOwnCloudAccount(), context);
return Optional.of(client);
} catch (Exception e) {
Log_OC.e(this, "Could not create client", e);
}
return client;
}

@Override
public void run(AccountManagerFuture<Boolean> future) {
if (future.isDone()) {
EventBus.getDefault().post(new AccountRemovedEvent());
return Optional.empty();
}
}
}
Loading