From 1d6dab29562eca7978f179be5f5c75f22f44d734 Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Mon, 14 Jun 2010 12:24:02 -0700 Subject: [PATCH] Handle "Allow non-provisionable devices" properly * Send policy key of "0" when validating; this gets us the policies even if "Allow..." is enabled (currently, we simply don't see the policies) * If we don't support all of the policies, send back the response code indicating support for partial support. If we get a positive response back, then we're good to go - the server allows devices with partial support. Otherwise, we fail as we always have - with the toast indicating that the device doesn't support required policies * Remove PolicySet.isSupported() and ensure proper field ranges within the constructor * Update tests as appropriate Bug: 2759782 Change-Id: I5f354a0e2d81844aff75d8a8a6de3b97f0020c1f --- src/com/android/email/SecurityPolicy.java | 79 ++++++++----------- src/com/android/exchange/EasSyncService.java | 69 +++++++++------- .../exchange/adapter/CalendarSyncAdapter.java | 2 +- .../exchange/adapter/ProvisionParser.java | 17 ++-- .../android/email/SecurityPolicyTests.java | 42 +++++----- .../android/exchange/EasSyncServiceTests.java | 48 +++++++++++ .../adapter/ProvisionParserTests.java | 22 +++--- 7 files changed, 163 insertions(+), 116 deletions(-) diff --git a/src/com/android/email/SecurityPolicy.java b/src/com/android/email/SecurityPolicy.java index 2b0f55239..28d7e4e87 100644 --- a/src/com/android/email/SecurityPolicy.java +++ b/src/com/android/email/SecurityPolicy.java @@ -74,15 +74,6 @@ public class SecurityPolicy { private static final int ACCOUNT_FLAGS_COLUMN_ID = 0; private static final int ACCOUNT_FLAGS_COLUMN_FLAGS = 1; - /** - * These are hardcoded limits based on knowledge of the current DevicePolicyManager - * and screen lock mechanisms. Wherever possible, these should be replaced with queries of - * dynamic capabilities of the device (e.g. what password modes are supported?) - */ - private static final int LIMIT_MIN_PASSWORD_LENGTH = 16; - private static final int LIMIT_PASSWORD_MODE = PolicySet.PASSWORD_MODE_STRONG; - private static final int LIMIT_SCREENLOCK_TIME = PolicySet.SCREEN_LOCK_TIME_MAX; - /** * Get the security policy instance */ @@ -188,33 +179,6 @@ public class SecurityPolicy { return mDPM; } - /** - * API: Query used to determine if a given policy is "possible" (irrespective of current - * device state. This is used when creating new accounts. - * - * TODO: This is hardcoded based on knowledge of the current DevicePolicyManager - * and screen lock mechanisms. It would be nice to replace these tests with something - * more dynamic. - * - * @param policies the policies requested - * @return true if the policies are supported, false if not supported - */ - public boolean isSupported(PolicySet policies) { - if (policies.mMinPasswordLength > LIMIT_MIN_PASSWORD_LENGTH) { - return false; - } - if (policies.mPasswordMode > LIMIT_PASSWORD_MODE ) { - return false; - } - // No limit on password fail count - if (policies.mMaxScreenLockTime > LIMIT_SCREENLOCK_TIME ) { - return false; - } - // No limit on remote wipe capable - - return true; - } - /** * API: Report that policies may have been updated due to rewriting values in an Account. * @param accountId the account that has been updated, -1 if unknown/deleted @@ -435,7 +399,6 @@ public class SecurityPolicy { private static final int PASSWORD_LENGTH_MASK = 31; private static final int PASSWORD_LENGTH_SHIFT = 0; public static final int PASSWORD_LENGTH_MAX = 30; - private static final int PASSWORD_LENGTH_EXCEEDED = 31; // bits 5..8: password mode private static final int PASSWORD_MODE_SHIFT = 5; private static final int PASSWORD_MODE_MASK = 15 << PASSWORD_MODE_SHIFT; @@ -453,11 +416,31 @@ public class SecurityPolicy { // bit 25: remote wipe capability required private static final int REQUIRE_REMOTE_WIPE = 1 << 25; - public final int mMinPasswordLength; - public final int mPasswordMode; - public final int mMaxPasswordFails; - public final int mMaxScreenLockTime; - public final boolean mRequireRemoteWipe; + /*package*/ final int mMinPasswordLength; + /*package*/ final int mPasswordMode; + /*package*/ final int mMaxPasswordFails; + /*package*/ final int mMaxScreenLockTime; + /*package*/ final boolean mRequireRemoteWipe; + + public int getMinPasswordLength() { + return mMinPasswordLength; + } + + public int getPasswordMode() { + return mPasswordMode; + } + + public int getMaxPasswordFails() { + return mMaxPasswordFails; + } + + public int getMaxScreenLockTime() { + return mMaxScreenLockTime; + } + + public boolean isRequireRemoteWipe() { + return mRequireRemoteWipe; + } /** * Create from raw values. @@ -470,15 +453,17 @@ public class SecurityPolicy { */ public PolicySet(int minPasswordLength, int passwordMode, int maxPasswordFails, int maxScreenLockTime, boolean requireRemoteWipe) throws IllegalArgumentException { - // This value has a hard limit which cannot be supported if exceeded. Setting the - // exceeded value will force isSupported() to return false. + // Check against hard limits + // EAS doesn't generate values outside these limits anyway if (minPasswordLength > PASSWORD_LENGTH_MAX) { - minPasswordLength = PASSWORD_LENGTH_EXCEEDED; + throw new IllegalArgumentException("password length"); } - if (passwordMode < PASSWORD_MODE_NONE - || passwordMode > PASSWORD_MODE_STRONG) { + if (passwordMode < PASSWORD_MODE_NONE || passwordMode > PASSWORD_MODE_STRONG) { throw new IllegalArgumentException("password mode"); } + if (maxScreenLockTime > SCREEN_LOCK_TIME_MAX) { + throw new IllegalArgumentException("screen lock time"); + } // This value can be reduced (which actually increases security) if necessary if (maxPasswordFails > PASSWORD_MAX_FAILS_MAX) { maxPasswordFails = PASSWORD_MAX_FAILS_MAX; diff --git a/src/com/android/exchange/EasSyncService.java b/src/com/android/exchange/EasSyncService.java index 62d780442..6374fdf75 100644 --- a/src/com/android/exchange/EasSyncService.java +++ b/src/com/android/exchange/EasSyncService.java @@ -175,12 +175,17 @@ public class EasSyncService extends AbstractSyncService { // MSFT's custom HTTP result code indicating the need to provision static private final int HTTP_NEED_PROVISIONING = 449; + // The EAS protocol Provision status for "we implement all of the policies" + static private final String PROVISION_STATUS_OK = "1"; + // The EAS protocol Provision status meaning "we partially implement the policies" + static private final String PROVISION_STATUS_PARTIAL = "2"; + // Reasonable default public String mProtocolVersion = Eas.DEFAULT_PROTOCOL_VERSION; public Double mProtocolVersionDouble; protected String mDeviceId = null; - private String mDeviceType = "Android"; - private String mAuthString = null; + /*package*/ String mDeviceType = "Android"; + /*package*/ String mAuthString = null; private String mCmdString = null; public String mHostAddress; public String mUserName; @@ -780,8 +785,7 @@ public class EasSyncService extends AbstractSyncService { * TODO: make watchdog actually work (it doesn't understand our service w/Mailbox == 0) * TODO: figure out why sendHttpClientPost() hangs - possibly pool exhaustion */ - static public GalResult searchGal(Context context, long accountId, String filter) - { + static public GalResult searchGal(Context context, long accountId, String filter) { Account acct = SyncManager.getAccountById(accountId); if (acct != null) { HostAuth ha = HostAuth.restoreHostAuthWithId(context, acct.mHostAuthKeyRecv); @@ -1107,18 +1111,21 @@ public class EasSyncService extends AbstractSyncService { * @param method the method we are going to send * @param usePolicyKey whether or not a policy key should be sent in the headers */ - private void setHeaders(HttpRequestBase method, boolean usePolicyKey) { + /*package*/ void setHeaders(HttpRequestBase method, boolean usePolicyKey) { method.setHeader("Authorization", mAuthString); method.setHeader("MS-ASProtocolVersion", mProtocolVersion); method.setHeader("Connection", "keep-alive"); method.setHeader("User-Agent", mDeviceType + '/' + Eas.VERSION); - if (usePolicyKey && (mAccount != null)) { - String key = mAccount.mSecuritySyncKey; - if (key == null || key.length() == 0) { - return; - } - if (Eas.PARSER_LOG) { - userLog("Policy key: " , key); + if (usePolicyKey) { + // If there's an account in existence, use its key; otherwise (we're creating the + // account), send "0". The server will respond with code 449 if there are policies + // to be enforced + String key = "0"; + if (mAccount != null) { + String accountKey = mAccount.mSecuritySyncKey; + if (!TextUtils.isEmpty(accountKey)) { + key = accountKey; + } } method.setHeader("X-MS-PolicyKey", key); } @@ -1285,7 +1292,7 @@ public class EasSyncService extends AbstractSyncService { } else if (sp.isActive(ps)) { // See if the required policies are in force; if they are, acknowledge the policies // to the server and get the final policy key - String policyKey = acknowledgeProvision(pp.getPolicyKey()); + String policyKey = acknowledgeProvision(pp.getPolicyKey(), PROVISION_STATUS_OK); if (policyKey != null) { // Write the final policy key to the Account and say we've been successful ps.writeAccount(mAccount, policyKey, true, mContext); @@ -1322,15 +1329,20 @@ public class EasSyncService extends AbstractSyncService { InputStream is = resp.getEntity().getContent(); ProvisionParser pp = new ProvisionParser(is, this); if (pp.parse()) { - // If true, we received policies from the server; see if they are supported by - // the framework; if so, return the ProvisionParser containing the policy set and - // temporary key - PolicySet ps = pp.getPolicySet(); - // The PolicySet can be null if there are policies we don't know about (e.g. ones - // from Exchange 12.1) If we have a PolicySet, then we ask whether the device can - // support the actual parameters of those policies. - if ((ps != null) && SecurityPolicy.getInstance(mContext).isSupported(ps)) { + // The PolicySet in the ProvisionParser will have the requirements for all KNOWN + // policies. If others are required, hasSupportablePolicySet will be false + if (pp.hasSupportablePolicySet()) { + // If the policies are supportable (in this context, meaning that there are no + // completely unimplemented policies required), just return the parser itself return pp; + } else { + // Try to acknowledge using the "partial" status (i.e. we can partially + // accommodate the required policies). The server will agree to this if the + // "allow non-provisionable devices" setting is enabled on the server + String policyKey = acknowledgeProvision(pp.getPolicyKey(), + PROVISION_STATUS_PARTIAL); + // Return either the parser (success) or null (failure) + return (policyKey != null) ? pp : null; } } } @@ -1346,14 +1358,15 @@ public class EasSyncService extends AbstractSyncService { * @throws IOException */ private void acknowledgeRemoteWipe(String tempKey) throws IOException { - acknowledgeProvisionImpl(tempKey, true); + acknowledgeProvisionImpl(tempKey, PROVISION_STATUS_OK, true); } - private String acknowledgeProvision(String tempKey) throws IOException { - return acknowledgeProvisionImpl(tempKey, false); + private String acknowledgeProvision(String tempKey, String result) throws IOException { + return acknowledgeProvisionImpl(tempKey, result, false); } - private String acknowledgeProvisionImpl(String tempKey, boolean remoteWipe) throws IOException { + private String acknowledgeProvisionImpl(String tempKey, String status, + boolean remoteWipe) throws IOException { Serializer s = new Serializer(); s.start(Tags.PROVISION_PROVISION).start(Tags.PROVISION_POLICIES); s.start(Tags.PROVISION_POLICY); @@ -1362,11 +1375,11 @@ public class EasSyncService extends AbstractSyncService { s.data(Tags.PROVISION_POLICY_TYPE, getPolicyType()); s.data(Tags.PROVISION_POLICY_KEY, tempKey); - s.data(Tags.PROVISION_STATUS, "1"); + s.data(Tags.PROVISION_STATUS, status); s.end().end(); // PROVISION_POLICY, PROVISION_POLICIES if (remoteWipe) { s.start(Tags.PROVISION_REMOTE_WIPE); - s.data(Tags.PROVISION_STATUS, "1"); + s.data(Tags.PROVISION_STATUS, PROVISION_STATUS_OK); s.end(); } s.end().done(); // PROVISION_PROVISION @@ -1376,7 +1389,7 @@ public class EasSyncService extends AbstractSyncService { InputStream is = resp.getEntity().getContent(); ProvisionParser pp = new ProvisionParser(is, this); if (pp.parse()) { - // Return the final polic key from the ProvisionParser + // Return the final policy key from the ProvisionParser return pp.getPolicyKey(); } } diff --git a/src/com/android/exchange/adapter/CalendarSyncAdapter.java b/src/com/android/exchange/adapter/CalendarSyncAdapter.java index 7869ea745..210b62396 100644 --- a/src/com/android/exchange/adapter/CalendarSyncAdapter.java +++ b/src/com/android/exchange/adapter/CalendarSyncAdapter.java @@ -1251,7 +1251,7 @@ public class CalendarSyncAdapter extends AbstractSyncAdapter { } } - private class CalendarOperations extends ArrayList { + protected class CalendarOperations extends ArrayList { private static final long serialVersionUID = 1L; public int mCount = 0; private ContentProviderResult[] mResults = null; diff --git a/src/com/android/exchange/adapter/ProvisionParser.java b/src/com/android/exchange/adapter/ProvisionParser.java index 530463933..6b88c80e3 100644 --- a/src/com/android/exchange/adapter/ProvisionParser.java +++ b/src/com/android/exchange/adapter/ProvisionParser.java @@ -37,11 +37,11 @@ public class ProvisionParser extends Parser { PolicySet mPolicySet = null; String mPolicyKey = null; boolean mRemoteWipe = false; + boolean mIsSupportable = true; public ProvisionParser(InputStream in, EasSyncService service) throws IOException { super(in); mService = service; - setDebug(true); } public PolicySet getPolicySet() { @@ -56,12 +56,15 @@ public class ProvisionParser extends Parser { return mRemoteWipe; } + public boolean hasSupportablePolicySet() { + return (mPolicySet != null) && mIsSupportable; + } + private void parseProvisionDocWbxml() throws IOException { int minPasswordLength = 0; int passwordMode = PolicySet.PASSWORD_MODE_NONE; int maxPasswordFails = 0; int maxScreenLockTime = 0; - boolean canSupport = true; while (nextTag(Tags.PROVISION_EAS_PROVISION_DOC) != END) { boolean supported = true; @@ -181,15 +184,13 @@ public class ProvisionParser extends Parser { } if (!supported) { - log("** Policy not supported"); - canSupport = false; + log("Policy not supported: " + tag); + mIsSupportable = false; } } - if (canSupport) { - mPolicySet = new SecurityPolicy.PolicySet(minPasswordLength, passwordMode, + mPolicySet = new SecurityPolicy.PolicySet(minPasswordLength, passwordMode, maxPasswordFails, maxScreenLockTime, true); - } } /** @@ -412,10 +413,10 @@ public class ProvisionParser extends Parser { case Tags.PROVISION_STATUS: int status = getValueInt(); mService.userLog("Provision status: ", status); + res = (status == 1); break; case Tags.PROVISION_POLICIES: parsePolicies(); - res = (mPolicySet != null) || (mPolicyKey != null); break; case Tags.PROVISION_REMOTE_WIPE: // Indicate remote wipe command received diff --git a/tests/src/com/android/email/SecurityPolicyTests.java b/tests/src/com/android/email/SecurityPolicyTests.java index 2c5185394..105e656ed 100644 --- a/tests/src/com/android/email/SecurityPolicyTests.java +++ b/tests/src/com/android/email/SecurityPolicyTests.java @@ -95,6 +95,27 @@ public class SecurityPolicyTests extends ProviderTestCase2 { return sp; } + public void testPolicySetConstructor() { + // We know that EMPTY_POLICY_SET doesn't generate an Exception or we wouldn't be here + // Try some illegal parameters + try { + new PolicySet(100, PolicySet.PASSWORD_MODE_NONE, 0, 0, false); + fail("Too-long password allowed"); + } catch (IllegalArgumentException e) { + } + try { + new PolicySet(0, PolicySet.PASSWORD_MODE_STRONG + 1, 0, 0, false); + fail("Illegal password mode allowed"); + } catch (IllegalArgumentException e) { + } + try { + new PolicySet(0, PolicySet.PASSWORD_MODE_NONE, 0, + PolicySet.SCREEN_LOCK_TIME_MAX + 1, false); + fail("Too-long screen lock time allowed"); + } catch (IllegalArgumentException e) { + } + } + /** * Test business logic of aggregating accounts with policies */ @@ -221,27 +242,6 @@ public class SecurityPolicyTests extends ProviderTestCase2 { assertTrue(p.mRequireRemoteWipe); } - /** - * Test creation of policies with unsupported ranges - */ - @SmallTest - public void testFieldRanges() { - SecurityPolicy sp = getSecurityPolicy(); - // Overlong password length cannot be supported - PolicySet p = new PolicySet(PolicySet.PASSWORD_LENGTH_MAX + 1, 0, 0, 0, false); - assertFalse(sp.isSupported(p)); - - // Too many wipes before reboot can be supported (by reducing to the max) - p = new PolicySet(0, 0, PolicySet.PASSWORD_MAX_FAILS_MAX + 1, 0, false); - assertTrue(sp.isSupported(p)); - assertEquals(PolicySet.PASSWORD_MAX_FAILS_MAX, p.mMaxPasswordFails); - - // Too long lock time can be supported (by reducing to the max) - p = new PolicySet(0, 0, 0, PolicySet.SCREEN_LOCK_TIME_MAX + 1, false); - assertTrue(sp.isSupported(p)); - assertEquals(PolicySet.SCREEN_LOCK_TIME_MAX, p.mMaxScreenLockTime); - } - /** * Test encoding into an Account and out again */ diff --git a/tests/src/com/android/exchange/EasSyncServiceTests.java b/tests/src/com/android/exchange/EasSyncServiceTests.java index d4372a57b..dbb9bbb50 100644 --- a/tests/src/com/android/exchange/EasSyncServiceTests.java +++ b/tests/src/com/android/exchange/EasSyncServiceTests.java @@ -17,12 +17,23 @@ package com.android.exchange; +import com.android.email.provider.EmailContent.Account; + +import org.apache.http.Header; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.client.methods.HttpRequestBase; + import android.content.Context; import android.test.AndroidTestCase; import java.io.File; import java.io.IOException; +/** + * You can run this entire test case with: + * runtest -c com.android.exchange.EasSyncServiceTests email + */ + public class EasSyncServiceTests extends AndroidTestCase { Context mMockContext; @@ -73,4 +84,41 @@ public class EasSyncServiceTests extends AndroidTestCase { } } } + + public void testAddHeaders() { + HttpRequestBase method = new HttpPost(); + EasSyncService svc = new EasSyncService(); + svc.mAuthString = "auth"; + svc.mProtocolVersion = "12.1"; + svc.mDeviceType = "android"; + svc.mAccount = null; + // With second argument false, there should be no header + svc.setHeaders(method, false); + Header[] headers = method.getHeaders("X-MS-PolicyKey"); + assertEquals(0, headers.length); + // With second argument true, there should always be a header + // The value will be "0" without an account + method.removeHeaders("X-MS-PolicyKey"); + svc.setHeaders(method, true); + headers = method.getHeaders("X-MS-PolicyKey"); + assertEquals(1, headers.length); + assertEquals("0", headers[0].getValue()); + // With an account, but null security key, the header's value should be "0" + Account account = new Account(); + account.mSecuritySyncKey = null; + svc.mAccount = account; + method.removeHeaders("X-MS-PolicyKey"); + svc.setHeaders(method, true); + headers = method.getHeaders("X-MS-PolicyKey"); + assertEquals(1, headers.length); + assertEquals("0", headers[0].getValue()); + // With an account and security key, the header's value should be the security key + account.mSecuritySyncKey = "key"; + svc.mAccount = account; + method.removeHeaders("X-MS-PolicyKey"); + svc.setHeaders(method, true); + headers = method.getHeaders("X-MS-PolicyKey"); + assertEquals(1, headers.length); + assertEquals("key", headers[0].getValue()); + } } diff --git a/tests/src/com/android/exchange/adapter/ProvisionParserTests.java b/tests/src/com/android/exchange/adapter/ProvisionParserTests.java index 09a9d96d2..842e4b659 100644 --- a/tests/src/com/android/exchange/adapter/ProvisionParserTests.java +++ b/tests/src/com/android/exchange/adapter/ProvisionParserTests.java @@ -113,11 +113,11 @@ public class ProvisionParserTests extends SyncAdapterTestCase { PolicySet ps = parser.getPolicySet(); assertNotNull(ps); // Check the settings to make sure they were parsed correctly - assertEquals(5*60, ps.mMaxScreenLockTime); // Screen lock time is in seconds - assertEquals(8, ps.mMinPasswordLength); - assertEquals(PolicySet.PASSWORD_MODE_STRONG, ps.mPasswordMode); - assertEquals(20, ps.mMaxPasswordFails); - assertTrue(ps.mRequireRemoteWipe); + assertEquals(5*60, ps.getMaxScreenLockTime()); // Screen lock time is in seconds + assertEquals(8, ps.getMinPasswordLength()); + assertEquals(PolicySet.PASSWORD_MODE_STRONG, ps.getPasswordMode()); + assertEquals(20, ps.getMaxPasswordFails()); + assertTrue(ps.isRequireRemoteWipe()); } public void testWapProvisionParser2() throws IOException { @@ -126,7 +126,7 @@ public class ProvisionParserTests extends SyncAdapterTestCase { PolicySet ps = parser.getPolicySet(); assertNotNull(ps); // Password should be set to none; others are ignored in this case. - assertEquals(PolicySet.PASSWORD_MODE_NONE, ps.mPasswordMode); + assertEquals(PolicySet.PASSWORD_MODE_NONE, ps.getPasswordMode()); } public void testWapProvisionParser3() throws IOException { @@ -135,10 +135,10 @@ public class ProvisionParserTests extends SyncAdapterTestCase { PolicySet ps = parser.getPolicySet(); assertNotNull(ps); // Password should be set to simple - assertEquals(2*60, ps.mMaxScreenLockTime); // Screen lock time is in seconds - assertEquals(4, ps.mMinPasswordLength); - assertEquals(PolicySet.PASSWORD_MODE_SIMPLE, ps.mPasswordMode); - assertEquals(5, ps.mMaxPasswordFails); - assertTrue(ps.mRequireRemoteWipe); + assertEquals(2*60, ps.getMaxScreenLockTime()); // Screen lock time is in seconds + assertEquals(4, ps.getMinPasswordLength()); + assertEquals(PolicySet.PASSWORD_MODE_SIMPLE, ps.getPasswordMode()); + assertEquals(5, ps.getMaxPasswordFails()); + assertTrue(ps.isRequireRemoteWipe()); } }