From 32bfb594af7f5dc0be608393200060458cd84631 Mon Sep 17 00:00:00 2001 From: Svet Ganov Date: Fri, 23 Sep 2016 21:30:19 -0700 Subject: [PATCH] [DO NOT MERGE] Use tokens instead of account access trackers We keep track which process saw and account to whitelist the app for future access as an optimization to avoid prompting the user for account access approval. Some apps use SefeParcelable where the parcels are marshalled which does not allow the parcel to contain IBinders. To avoid this we are switching from account tracker remote objects to unforgeable tokens. bug:31162498 Change-Id: I3b52bff720655f695ad0c58d420eb35ef93161b9 --- Android.mk | 1 - core/java/android/accounts/Account.java | 26 ++++--- .../java/android/accounts/AccountManager.java | 22 +++--- .../accounts/IAccountAccessTracker.aidl | 26 ------- .../android/accounts/IAccountManager.aidl | 2 + .../accounts/AccountManagerService.java | 67 +++++++++---------- 6 files changed, 58 insertions(+), 86 deletions(-) delete mode 100644 core/java/android/accounts/IAccountAccessTracker.aidl diff --git a/Android.mk b/Android.mk index d765e9c568491..6f3eb21617ee4 100644 --- a/Android.mk +++ b/Android.mk @@ -63,7 +63,6 @@ LOCAL_SRC_FILES += \ core/java/android/accessibilityservice/IAccessibilityServiceClient.aidl \ core/java/android/accounts/IAccountManager.aidl \ core/java/android/accounts/IAccountManagerResponse.aidl \ - core/java/android/accounts/IAccountAccessTracker.aidl \ core/java/android/accounts/IAccountAuthenticator.aidl \ core/java/android/accounts/IAccountAuthenticatorResponse.aidl \ core/java/android/app/IActivityContainer.aidl \ diff --git a/core/java/android/accounts/Account.java b/core/java/android/accounts/Account.java index 6c16e322e9784..b6e85f18a6952 100644 --- a/core/java/android/accounts/Account.java +++ b/core/java/android/accounts/Account.java @@ -18,9 +18,11 @@ package android.accounts; import android.annotation.NonNull; import android.annotation.Nullable; +import android.content.Context; import android.os.Parcelable; import android.os.Parcel; import android.os.RemoteException; +import android.os.ServiceManager; import android.text.TextUtils; import android.util.ArraySet; import android.util.Log; @@ -41,7 +43,7 @@ public class Account implements Parcelable { public final String name; public final String type; - private final @Nullable IAccountAccessTracker mAccessTracker; + private final @Nullable String accessId; public boolean equals(Object o) { if (o == this) return true; @@ -64,14 +66,14 @@ public class Account implements Parcelable { /** * @hide */ - public Account(@NonNull Account other, @Nullable IAccountAccessTracker accessTracker) { - this(other.name, other.type, accessTracker); + public Account(@NonNull Account other, @NonNull String accessId) { + this(other.name, other.type, accessId); } /** * @hide */ - public Account(String name, String type, IAccountAccessTracker accessTracker) { + public Account(String name, String type, String accessId) { if (TextUtils.isEmpty(name)) { throw new IllegalArgumentException("the name must not be empty: " + name); } @@ -80,18 +82,20 @@ public class Account implements Parcelable { } this.name = name; this.type = type; - this.mAccessTracker = accessTracker; + this.accessId = accessId; } public Account(Parcel in) { this.name = in.readString(); this.type = in.readString(); - this.mAccessTracker = IAccountAccessTracker.Stub.asInterface(in.readStrongBinder()); - if (mAccessTracker != null) { + this.accessId = in.readString(); + if (accessId != null) { synchronized (sAccessedAccounts) { if (sAccessedAccounts.add(this)) { try { - mAccessTracker.onAccountAccessed(); + IAccountManager accountManager = IAccountManager.Stub.asInterface( + ServiceManager.getService(Context.ACCOUNT_SERVICE)); + accountManager.onAccountAccessed(accessId); } catch (RemoteException e) { Log.e(TAG, "Error noting account access", e); } @@ -101,8 +105,8 @@ public class Account implements Parcelable { } /** @hide */ - public IAccountAccessTracker getAccessTracker() { - return mAccessTracker; + public String getAccessId() { + return accessId; } public int describeContents() { @@ -112,7 +116,7 @@ public class Account implements Parcelable { public void writeToParcel(Parcel dest, int flags) { dest.writeString(name); dest.writeString(type); - dest.writeStrongInterface(mAccessTracker); + dest.writeString(accessId); } public static final Creator CREATOR = new Creator() { diff --git a/core/java/android/accounts/AccountManager.java b/core/java/android/accounts/AccountManager.java index 0a9dc3a183750..72707b7f6663f 100644 --- a/core/java/android/accounts/AccountManager.java +++ b/core/java/android/accounts/AccountManager.java @@ -179,12 +179,12 @@ public class AccountManager { public static final String KEY_ACCOUNT_TYPE = "accountType"; /** - * Bundle key used for the {@link IAccountAccessTracker} account access tracker - * used for noting the account was accessed when unmarshalled from a parcel. + * Bundle key used for the account access id used for noting the + * account was accessed when unmarshalled from a parcel. * * @hide */ - public static final String KEY_ACCOUNT_ACCESS_TRACKER = "accountAccessTracker"; + public static final String KEY_ACCOUNT_ACCESS_ID = "accountAccessId"; /** * Bundle key used for the auth token value in results @@ -821,9 +821,8 @@ public class AccountManager { public Account bundleToResult(Bundle bundle) throws AuthenticatorException { String name = bundle.getString(KEY_ACCOUNT_NAME); String type = bundle.getString(KEY_ACCOUNT_TYPE); - IAccountAccessTracker tracker = IAccountAccessTracker.Stub.asInterface( - bundle.getBinder(KEY_ACCOUNT_ACCESS_TRACKER)); - return new Account(name, type, tracker); + String accessId = bundle.getString(KEY_ACCOUNT_ACCESS_ID); + return new Account(name, type, accessId); } }.start(); } @@ -2279,7 +2278,7 @@ public class AccountManager { result.putString(KEY_ACCOUNT_NAME, null); result.putString(KEY_ACCOUNT_TYPE, null); result.putString(KEY_AUTHTOKEN, null); - result.putBinder(KEY_ACCOUNT_ACCESS_TRACKER, null); + result.putBinder(KEY_ACCOUNT_ACCESS_ID, null); try { mResponse.onResult(result); } catch (RemoteException e) { @@ -2306,9 +2305,7 @@ public class AccountManager { Account account = new Account( value.getString(KEY_ACCOUNT_NAME), value.getString(KEY_ACCOUNT_TYPE), - IAccountAccessTracker.Stub.asInterface( - value.getBinder( - KEY_ACCOUNT_ACCESS_TRACKER))); + value.getString(KEY_ACCOUNT_ACCESS_ID)); mFuture = getAuthToken(account, mAuthTokenType, mLoginOptions, mActivity, mMyCallback, mHandler); @@ -2358,9 +2355,8 @@ public class AccountManager { setException(new AuthenticatorException("account not in result")); return; } - final IAccountAccessTracker tracker = IAccountAccessTracker.Stub.asInterface( - result.getBinder(KEY_ACCOUNT_ACCESS_TRACKER)); - final Account account = new Account(accountName, accountType, tracker); + final String accessId = result.getString(KEY_ACCOUNT_ACCESS_ID); + final Account account = new Account(accountName, accountType, accessId); mNumAccounts = 1; getAuthToken(account, mAuthTokenType, null /* options */, mActivity, mMyCallback, mHandler); diff --git a/core/java/android/accounts/IAccountAccessTracker.aidl b/core/java/android/accounts/IAccountAccessTracker.aidl deleted file mode 100644 index e12b3d1e21c78..0000000000000 --- a/core/java/android/accounts/IAccountAccessTracker.aidl +++ /dev/null @@ -1,26 +0,0 @@ -/* - * Copyright (C) 2016 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package android.accounts; - -/** - * Interface to track which apps accessed an account - * - * @hide - */ -oneway interface IAccountAccessTracker { - void onAccountAccessed(); -} diff --git a/core/java/android/accounts/IAccountManager.aidl b/core/java/android/accounts/IAccountManager.aidl index 56a6488088b58..c271e7ebc52c8 100644 --- a/core/java/android/accounts/IAccountManager.aidl +++ b/core/java/android/accounts/IAccountManager.aidl @@ -110,4 +110,6 @@ interface IAccountManager { /* Crate an intent to request account access for package and a given user id */ IntentSender createRequestAccountAccessIntentSenderAsUser(in Account account, String packageName, in UserHandle userHandle); + + void onAccountAccessed(String token); } diff --git a/services/core/java/com/android/server/accounts/AccountManagerService.java b/services/core/java/com/android/server/accounts/AccountManagerService.java index 82605c62472d3..a7a79cd5b3eb8 100644 --- a/services/core/java/com/android/server/accounts/AccountManagerService.java +++ b/services/core/java/com/android/server/accounts/AccountManagerService.java @@ -26,7 +26,6 @@ import android.accounts.AccountManagerInternal; import android.accounts.AuthenticatorDescription; import android.accounts.CantAddAccountActivity; import android.accounts.GrantCredentialsPermissionActivity; -import android.accounts.IAccountAccessTracker; import android.accounts.IAccountAuthenticator; import android.accounts.IAccountAuthenticatorResponse; import android.accounts.IAccountManager; @@ -89,7 +88,6 @@ import android.os.UserManager; import android.os.storage.StorageManager; import android.text.TextUtils; import android.util.Log; -import android.util.PackageUtils; import android.util.Pair; import android.util.Slog; import android.util.SparseArray; @@ -128,6 +126,8 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; +import java.util.UUID; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -692,7 +692,7 @@ public class AccountManagerService final Account[] accountsForType = new Account[accountNames.size()]; for (int i = 0; i < accountsForType.length; i++) { accountsForType[i] = new Account(accountNames.get(i), accountType, - new AccountAccessTracker()); + UUID.randomUUID().toString()); } accounts.accountCache.put(accountType, accountsForType); } @@ -1502,8 +1502,8 @@ public class AccountManagerService Bundle result = new Bundle(); result.putString(AccountManager.KEY_ACCOUNT_NAME, resultingAccount.name); result.putString(AccountManager.KEY_ACCOUNT_TYPE, resultingAccount.type); - result.putBinder(AccountManager.KEY_ACCOUNT_ACCESS_TRACKER, - resultingAccount.getAccessTracker().asBinder()); + result.putString(AccountManager.KEY_ACCOUNT_ACCESS_ID, + resultingAccount.getAccessId()); try { response.onResult(result); } catch (RemoteException e) { @@ -4079,6 +4079,30 @@ public class AccountManagerService } } + @Override + public void onAccountAccessed(String token) throws RemoteException { + final int uid = Binder.getCallingUid(); + if (UserHandle.getAppId(uid) == Process.SYSTEM_UID) { + return; + } + final int userId = UserHandle.getCallingUserId(); + final long identity = Binder.clearCallingIdentity(); + try { + for (Account account : getAccounts(userId, mContext.getOpPackageName())) { + if (Objects.equals(account.getAccessId(), token)) { + // An app just accessed the account. At this point it knows about + // it and there is not need to hide this account from the app. + if (!hasAccountAccess(account, null, uid)) { + updateAppPermission(account, AccountManager.ACCOUNT_ACCESS_TOKEN_TYPE, + uid, true); + } + } + } + } finally { + Binder.restoreCallingIdentity(identity); + } + } + private abstract class Session extends IAccountAuthenticatorResponse.Stub implements IBinder.DeathRecipient, ServiceConnection { IAccountManagerResponse mResponse; @@ -5714,9 +5738,9 @@ public class AccountManagerService if (accountsForType != null) { System.arraycopy(accountsForType, 0, newAccountsForType, 0, oldLength); } - IAccountAccessTracker accessTracker = account.getAccessTracker() != null - ? account.getAccessTracker() : new AccountAccessTracker(); - newAccountsForType[oldLength] = new Account(account, accessTracker); + String token = account.getAccessId() != null ? account.getAccessId() + : UUID.randomUUID().toString(); + newAccountsForType[oldLength] = new Account(account, token); accounts.accountCache.put(account.type, newAccountsForType); return newAccountsForType[oldLength]; } @@ -5961,33 +5985,6 @@ public class AccountManagerService } } - private final class AccountAccessTracker extends IAccountAccessTracker.Stub { - @Override - public void onAccountAccessed() throws RemoteException { - final int uid = Binder.getCallingUid(); - if (UserHandle.getAppId(uid) == Process.SYSTEM_UID) { - return; - } - final int userId = UserHandle.getCallingUserId(); - final long identity = Binder.clearCallingIdentity(); - try { - for (Account account : getAccounts(userId, mContext.getOpPackageName())) { - IAccountAccessTracker accountTracker = account.getAccessTracker(); - if (accountTracker != null && asBinder() == accountTracker.asBinder()) { - // An app just accessed the account. At this point it knows about - // it and there is not need to hide this account from the app. - if (!hasAccountAccess(account, null, uid)) { - updateAppPermission(account, AccountManager.ACCOUNT_ACCESS_TOKEN_TYPE, - uid, true); - } - } - } - } finally { - Binder.restoreCallingIdentity(identity); - } - } - } - private final class AccountManagerInternalImpl extends AccountManagerInternal { private final Object mLock = new Object();