From 17cc6a7b8198ce84c6ffd5c6fc3a9dfbe2c183ef Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Mon, 28 Jun 2010 16:07:36 -0700 Subject: [PATCH 1/7] Fix regression in Exchange calendar attendee response * During the fix of a previous late-Froyo issue, a change was made that appeared superficially correct, but was semantically incorrect. This changed the sense of the test for whether a reply email was required and caused the referenced bug. * The trivial fix is to replace the test with the (older) proper one Bug: 2764551 Change-Id: I7c72366252cf0607aee31ee0d76aca96a7d5fc2b --- src/com/android/exchange/adapter/CalendarSyncAdapter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/com/android/exchange/adapter/CalendarSyncAdapter.java b/src/com/android/exchange/adapter/CalendarSyncAdapter.java index cfefb6798..25b6913e7 100644 --- a/src/com/android/exchange/adapter/CalendarSyncAdapter.java +++ b/src/com/android/exchange/adapter/CalendarSyncAdapter.java @@ -597,7 +597,7 @@ public class CalendarSyncAdapter extends AbstractSyncAdapter { String attendeeEmail = attendee.getAsString(Attendees.ATTENDEE_EMAIL); sb.append(attendeeEmail); sb.append(ATTENDEE_TOKENIZER_DELIMITER); - if (selfOrganizer) { + if (mEmailAddress.equalsIgnoreCase(attendeeEmail)) { int attendeeStatus = CalendarUtilities.attendeeStatusFromBusyStatus(busyStatus); attendee.put(Attendees.ATTENDEE_STATUS, attendeeStatus); From eb2f7bbcb47f55cea181b090a94d4a7cad4f1293 Mon Sep 17 00:00:00 2001 From: Andrew Stadler Date: Wed, 30 Jun 2010 12:31:00 -0700 Subject: [PATCH 2/7] Explicitly verify certificate hostname on SSL connections When connecting to an IMAP, POP3, or SMTP server using SSL, perform an explicit test of the certificate's host name against the server's host name. Refuse connection if they do not match. Bug: 2807409 Change-Id: Ib223170f1a5d57323a88037ad30fec15c6bbce20 --- .../email/mail/transport/MailTransport.java | 59 +++++++++++++++++-- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/src/com/android/email/mail/transport/MailTransport.java b/src/com/android/email/mail/transport/MailTransport.java index a42bd245f..7639f9d73 100644 --- a/src/com/android/email/mail/transport/MailTransport.java +++ b/src/com/android/email/mail/transport/MailTransport.java @@ -34,12 +34,13 @@ import java.net.Socket; import java.net.SocketAddress; import java.net.SocketException; import java.net.URI; -import java.security.GeneralSecurityException; -import java.security.SecureRandom; -import javax.net.ssl.SSLContext; +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLException; -import javax.net.ssl.TrustManager; +import javax.net.ssl.SSLPeerUnverifiedException; +import javax.net.ssl.SSLSession; +import javax.net.ssl.SSLSocket; /** * This class implements the common aspects of "transport", one layer below the @@ -51,6 +52,9 @@ public class MailTransport implements Transport { /*protected*/ public static final int SOCKET_CONNECT_TIMEOUT = 10000; /*protected*/ public static final int SOCKET_READ_TIMEOUT = 60000; + private static final HostnameVerifier HOSTNAME_VERIFIER = + HttpsURLConnection.getDefaultHostnameVerifier(); + private String mDebugLabel; private String mHost; @@ -157,6 +161,10 @@ public class MailTransport implements Transport { mSocket = new Socket(); } mSocket.connect(socketAddress, SOCKET_CONNECT_TIMEOUT); + // After the socket connects to an SSL server, confirm that the hostname is as expected + if (canTrySslSecurity() && !canTrustAllCertificates()) { + verifyHostname(mSocket, getHost()); + } mIn = new BufferedInputStream(mSocket.getInputStream(), 1024); mOut = new BufferedOutputStream(mSocket.getOutputStream(), 512); @@ -176,6 +184,9 @@ public class MailTransport implements Transport { /** * Attempts to reopen a TLS connection using the Uri supplied for connection parameters. * + * NOTE: No explicit hostname verification is required here, because it's handled automatically + * by the call to createSocket(). + * * TODO should we explicitly close the old socket? This seems funky to abandon it. */ public void reopenTls() throws MessagingException { @@ -198,7 +209,45 @@ public class MailTransport implements Transport { throw new MessagingException(MessagingException.IOERROR, ioe.toString()); } } - + + /** + * Lightweight version of SSLCertificateSocketFactory.verifyHostname, which provides this + * service but is not in the public API. + * + * Verify the hostname of the certificate used by the other end of a + * connected socket. You MUST call this if you did not supply a hostname + * to SSLCertificateSocketFactory.createSocket(). It is harmless to call this method + * redundantly if the hostname has already been verified. + * + *

Wildcard certificates are allowed to verify any matching hostname, + * so "foo.bar.example.com" is verified if the peer has a certificate + * for "*.example.com". + * + * @param socket An SSL socket which has been connected to a server + * @param hostname The expected hostname of the remote server + * @throws IOException if something goes wrong handshaking with the server + * @throws SSLPeerUnverifiedException if the server cannot prove its identity + */ + private void verifyHostname(Socket socket, String hostname) throws IOException { + // The code at the start of OpenSSLSocketImpl.startHandshake() + // ensures that the call is idempotent, so we can safely call it. + SSLSocket ssl = (SSLSocket) socket; + ssl.startHandshake(); + + SSLSession session = ssl.getSession(); + if (session == null) { + throw new SSLException("Cannot verify SSL socket without session"); + } + // TODO: Instead of reporting the name of the server we think we're connecting to, + // we should be reporting the bad name in the certificate. Unfortunately this is buried + // in the verifier code and is not available in the verifier API, and extracting the + // CN & alts is beyond the scope of this patch. + if (!HOSTNAME_VERIFIER.verify(hostname, session)) { + throw new SSLPeerUnverifiedException( + "Certificate hostname not useable for server: " + hostname); + } + } + /** * Set the socket timeout. * @param timeoutMilliseconds the read timeout value if greater than {@code 0}, or From 34b5bea1636486591af4b96db76fc1042a6c65ee Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Mon, 12 Jul 2010 15:39:08 -0700 Subject: [PATCH 3/7] Handle correction of rejected Ping heartbeat * Handle status 5 for Ping command (heartbeat of out range) * Write unit test for heartbeat reset Bug: 2834195 Change-Id: Ic7952a4b296cf15c6ba895d6579fe7956b171e5b --- src/com/android/exchange/EasSyncService.java | 74 +++++++++++++++---- .../exchange/IllegalHeartbeatException.java | 27 +++++++ .../android/exchange/adapter/PingParser.java | 9 ++- .../android/exchange/EasSyncServiceTests.java | 48 +++++++++++- 4 files changed, 142 insertions(+), 16 deletions(-) create mode 100644 src/com/android/exchange/IllegalHeartbeatException.java diff --git a/src/com/android/exchange/EasSyncService.java b/src/com/android/exchange/EasSyncService.java index 3eb48c6c5..a95f3049b 100644 --- a/src/com/android/exchange/EasSyncService.java +++ b/src/com/android/exchange/EasSyncService.java @@ -153,10 +153,7 @@ public class EasSyncService extends AbstractSyncService { static private final int PING_MINUTES = 60; // in seconds static private final int PING_FUDGE_LOW = 10; static private final int PING_STARTING_HEARTBEAT = (8*PING_MINUTES)-PING_FUDGE_LOW; - static private final int PING_MIN_HEARTBEAT = (5*PING_MINUTES)-PING_FUDGE_LOW; - static private final int PING_MAX_HEARTBEAT = (17*PING_MINUTES)-PING_FUDGE_LOW; static private final int PING_HEARTBEAT_INCREMENT = 3*PING_MINUTES; - static private final int PING_FORCE_HEARTBEAT = 2*PING_MINUTES; // Maximum number of times we'll allow a sync to "loop" with MoreAvailable true before // forcing it to stop. This number has been determined empirically. @@ -192,12 +189,18 @@ public class EasSyncService extends AbstractSyncService { private ArrayList mPingChangeList; // The HttpPost in progress private volatile HttpPost mPendingPost = null; + // Our heartbeat when we are waiting for ping boxes to be ready + /*package*/ int mPingForceHeartbeat = 2*PING_MINUTES; + // The minimum heartbeat we will send + /*package*/ int mPingMinHeartbeat = (5*PING_MINUTES)-PING_FUDGE_LOW; + // The maximum heartbeat we will send + /*package*/ int mPingMaxHeartbeat = (17*PING_MINUTES)-PING_FUDGE_LOW; // The ping time (in seconds) - private int mPingHeartbeat = PING_STARTING_HEARTBEAT; + /*package*/ int mPingHeartbeat = PING_STARTING_HEARTBEAT; // The longest successful ping heartbeat private int mPingHighWaterMark = 0; // Whether we've ever lowered the heartbeat - private boolean mPingHeartbeatDropped = false; + /*package*/ boolean mPingHeartbeatDropped = false; // Whether a POST was aborted due to alarm (watchdog alarm) private boolean mPostAborted = false; // Whether a POST was aborted due to reset @@ -1541,6 +1544,10 @@ public class EasSyncService extends AbstractSyncService { } catch (StaleFolderListException e) { // We break out if we get told about a stale folder list userLog("Ping interrupted; folder list requires sync..."); + } catch (IllegalHeartbeatException e) { + // If we're sending an illegal heartbeat, reset either the min or the max to + // that heartbeat + resetHeartbeats(e.mLegalHeartbeat); } finally { Thread.currentThread().setName(threadName); } @@ -1561,6 +1568,44 @@ public class EasSyncService extends AbstractSyncService { } } + /** + * Reset either our minimum or maximum ping heartbeat to a heartbeat known to be legal + * @param legalHeartbeat a known legal heartbeat (from the EAS server) + */ + /*package*/ void resetHeartbeats(int legalHeartbeat) { + userLog("Resetting min/max heartbeat, legal = " + legalHeartbeat); + // We are here because the current heartbeat (mPingHeartbeat) is invalid. Depending on + // whether the argument is above or below the current heartbeat, we can infer the need to + // change either the minimum or maximum heartbeat + if (legalHeartbeat > mPingHeartbeat) { + // The legal heartbeat is higher than the ping heartbeat; therefore, our minimum was + // too low. We respond by raising either or both of the minimum heartbeat or the + // force heartbeat to the argument value + if (mPingMinHeartbeat < legalHeartbeat) { + mPingMinHeartbeat = legalHeartbeat; + } + if (mPingForceHeartbeat < legalHeartbeat) { + mPingForceHeartbeat = legalHeartbeat; + } + // If our minimum is now greater than the max, bring them together + if (mPingMinHeartbeat > mPingMaxHeartbeat) { + mPingMaxHeartbeat = legalHeartbeat; + } + } else if (legalHeartbeat < mPingHeartbeat) { + // The legal heartbeat is lower than the ping heartbeat; therefore, our maximum was + // too high. We respond by lowering the maximum to the argument value + mPingMaxHeartbeat = legalHeartbeat; + // If our maximum is now less than the minimum, bring them together + if (mPingMaxHeartbeat < mPingMinHeartbeat) { + mPingMinHeartbeat = legalHeartbeat; + } + } + // Set current heartbeat to the legal heartbeat + mPingHeartbeat = legalHeartbeat; + // Allow the heartbeat logic to run + mPingHeartbeatDropped = false; + } + private void pushFallback(long mailboxId) { Mailbox mailbox = Mailbox.restoreMailboxWithId(mContext, mailboxId); if (mailbox == null) { @@ -1593,7 +1638,8 @@ public class EasSyncService extends AbstractSyncService { return false; } - private void runPingLoop() throws IOException, StaleFolderListException { + private void runPingLoop() throws IOException, StaleFolderListException, + IllegalHeartbeatException { int pingHeartbeat = mPingHeartbeat; userLog("runPingLoop"); // Do push for all sync services here @@ -1693,7 +1739,7 @@ public class EasSyncService extends AbstractSyncService { userLog("Forcing ping after waiting for all boxes to be ready"); } HttpResponse res = - sendPing(s.toByteArray(), forcePing ? PING_FORCE_HEARTBEAT : pingHeartbeat); + sendPing(s.toByteArray(), forcePing ? mPingForceHeartbeat : pingHeartbeat); int code = res.getStatusLine().getStatusCode(); userLog("Ping response: ", code); @@ -1719,11 +1765,11 @@ public class EasSyncService extends AbstractSyncService { mPingHighWaterMark = pingHeartbeat; userLog("Setting high water mark at: ", mPingHighWaterMark); } - if ((pingHeartbeat < PING_MAX_HEARTBEAT) && + if ((pingHeartbeat < mPingMaxHeartbeat) && !mPingHeartbeatDropped) { pingHeartbeat += PING_HEARTBEAT_INCREMENT; - if (pingHeartbeat > PING_MAX_HEARTBEAT) { - pingHeartbeat = PING_MAX_HEARTBEAT; + if (pingHeartbeat > mPingMaxHeartbeat) { + pingHeartbeat = mPingMaxHeartbeat; } userLog("Increasing ping heartbeat to ", pingHeartbeat, "s"); } @@ -1748,12 +1794,12 @@ public class EasSyncService extends AbstractSyncService { // ping. } else if (mPostAborted || isLikelyNatFailure(message)) { long pingLength = SystemClock.elapsedRealtime() - pingTime; - if ((pingHeartbeat > PING_MIN_HEARTBEAT) && + if ((pingHeartbeat > mPingMinHeartbeat) && (pingHeartbeat > mPingHighWaterMark)) { pingHeartbeat -= PING_HEARTBEAT_INCREMENT; mPingHeartbeatDropped = true; - if (pingHeartbeat < PING_MIN_HEARTBEAT) { - pingHeartbeat = PING_MIN_HEARTBEAT; + if (pingHeartbeat < mPingMinHeartbeat) { + pingHeartbeat = mPingMinHeartbeat; } userLog("Decreased ping heartbeat to ", pingHeartbeat, "s"); } else if (mPostAborted) { @@ -1823,7 +1869,7 @@ public class EasSyncService extends AbstractSyncService { private int parsePingResult(InputStream is, ContentResolver cr, HashMap errorMap) - throws IOException, StaleFolderListException { + throws IOException, StaleFolderListException, IllegalHeartbeatException { PingParser pp = new PingParser(is, this); if (pp.parse()) { // True indicates some mailboxes need syncing... diff --git a/src/com/android/exchange/IllegalHeartbeatException.java b/src/com/android/exchange/IllegalHeartbeatException.java new file mode 100644 index 000000000..e480aa7a2 --- /dev/null +++ b/src/com/android/exchange/IllegalHeartbeatException.java @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2010 The Android Open Source Project + * Licensed to 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 com.android.exchange; + +public class IllegalHeartbeatException extends EasException { + private static final long serialVersionUID = 1L; + public final int mLegalHeartbeat; + + public IllegalHeartbeatException(int legalHeartbeat) { + mLegalHeartbeat = legalHeartbeat; + } +} diff --git a/src/com/android/exchange/adapter/PingParser.java b/src/com/android/exchange/adapter/PingParser.java index 0731ac054..f06147256 100644 --- a/src/com/android/exchange/adapter/PingParser.java +++ b/src/com/android/exchange/adapter/PingParser.java @@ -17,6 +17,7 @@ package com.android.exchange.adapter; import com.android.exchange.EasSyncService; +import com.android.exchange.IllegalHeartbeatException; import com.android.exchange.StaleFolderListException; import java.io.IOException; @@ -62,7 +63,7 @@ public class PingParser extends Parser { } @Override - public boolean parse() throws IOException, StaleFolderListException { + public boolean parse() throws IOException, StaleFolderListException, IllegalHeartbeatException { boolean res = false; if (nextTag(START_DOCUMENT) != Tags.PING_PING) { throw new IOException(); @@ -77,9 +78,15 @@ public class PingParser extends Parser { } else if (status == 7 || status == 4) { // Status of 7 or 4 indicate a stale folder list throw new StaleFolderListException(); + } else if (status == 5) { + // Status 5 means our heartbeat is beyond allowable limits + // In this case, there will be a heartbeat interval set } } else if (tag == Tags.PING_FOLDERS) { parsePingFolders(syncList); + } else if (tag == Tags.PING_HEARTBEAT_INTERVAL) { + // Throw an exception, saving away the legal heartbeat interval specified + throw new IllegalHeartbeatException(getValueInt()); } else { skipTag(); } diff --git a/tests/src/com/android/exchange/EasSyncServiceTests.java b/tests/src/com/android/exchange/EasSyncServiceTests.java index d4372a57b..1832d5ae1 100644 --- a/tests/src/com/android/exchange/EasSyncServiceTests.java +++ b/tests/src/com/android/exchange/EasSyncServiceTests.java @@ -23,7 +23,11 @@ import android.test.AndroidTestCase; import java.io.File; import java.io.IOException; -public class EasSyncServiceTests extends AndroidTestCase { +/** + * You can run this entire test case with: + * runtest -c com.android.exchange.EasSyncServiceTests email + */ + public class EasSyncServiceTests extends AndroidTestCase { Context mMockContext; @Override @@ -73,4 +77,46 @@ public class EasSyncServiceTests extends AndroidTestCase { } } } + + public void testResetHeartbeats() { + EasSyncService svc = new EasSyncService(); + // Test case in which the minimum and force heartbeats need to come up + svc.mPingMaxHeartbeat = 1000; + svc.mPingMinHeartbeat = 200; + svc.mPingHeartbeat = 300; + svc.mPingForceHeartbeat = 100; + svc.mPingHeartbeatDropped = true; + svc.resetHeartbeats(400); + assertEquals(400, svc.mPingMinHeartbeat); + assertEquals(1000, svc.mPingMaxHeartbeat); + assertEquals(400, svc.mPingHeartbeat); + assertEquals(400, svc.mPingForceHeartbeat); + assertFalse(svc.mPingHeartbeatDropped); + + // Test case in which the force heartbeat needs to come up + svc.mPingMaxHeartbeat = 1000; + svc.mPingMinHeartbeat = 200; + svc.mPingHeartbeat = 100; + svc.mPingForceHeartbeat = 100; + svc.mPingHeartbeatDropped = true; + svc.resetHeartbeats(150); + assertEquals(200, svc.mPingMinHeartbeat); + assertEquals(1000, svc.mPingMaxHeartbeat); + assertEquals(150, svc.mPingHeartbeat); + assertEquals(150, svc.mPingForceHeartbeat); + assertFalse(svc.mPingHeartbeatDropped); + + // Test case in which the maximum needs to come down + svc.mPingMaxHeartbeat = 1000; + svc.mPingMinHeartbeat = 200; + svc.mPingHeartbeat = 800; + svc.mPingForceHeartbeat = 100; + svc.mPingHeartbeatDropped = true; + svc.resetHeartbeats(600); + assertEquals(200, svc.mPingMinHeartbeat); + assertEquals(600, svc.mPingMaxHeartbeat); + assertEquals(600, svc.mPingHeartbeat); + assertEquals(100, svc.mPingForceHeartbeat); + assertFalse(svc.mPingHeartbeatDropped); + } } From 6c4a49bc3fec50e789185b98ccea8420ed50272e Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Mon, 14 Jun 2010 12:24:02 -0700 Subject: [PATCH 4/7] Backport: Handle "Allow non-provisionable devices" properly * Backport from master branch * 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: Ida5663a9b35c75ecc61a5f442be0bd60b433cb73 --- src/com/android/email/SecurityPolicy.java | 79 ++++++++----------- src/com/android/exchange/EasSyncService.java | 69 +++++++++------- .../exchange/adapter/ProvisionParser.java | 25 +++--- .../android/email/SecurityPolicyTests.java | 42 +++++----- .../android/exchange/EasSyncServiceTests.java | 43 ++++++++++ .../adapter/ProvisionParserTests.java | 22 +++--- 6 files changed, 164 insertions(+), 116 deletions(-) diff --git a/src/com/android/email/SecurityPolicy.java b/src/com/android/email/SecurityPolicy.java index 5dc9c14e5..076949b82 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 a95f3049b..a1a76dce5 100644 --- a/src/com/android/exchange/EasSyncService.java +++ b/src/com/android/exchange/EasSyncService.java @@ -172,12 +172,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; @@ -775,8 +780,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); @@ -1102,18 +1106,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); } @@ -1280,7 +1287,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); @@ -1317,15 +1324,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; } } } @@ -1341,14 +1353,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); @@ -1357,11 +1370,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 @@ -1371,7 +1384,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/ProvisionParser.java b/src/com/android/exchange/adapter/ProvisionParser.java index af1ecddcc..701f621e4 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,16 @@ public class ProvisionParser extends Parser { return mRemoteWipe; } - public void parseProvisionDocWbxml() throws IOException { + 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; + boolean supported = true; while (nextTag(Tags.PROVISION_EAS_PROVISION_DOC) != END) { switch (tag) { @@ -95,7 +99,7 @@ public class ProvisionParser extends Parser { // The following policy, if false, can't be supported at the moment case Tags.PROVISION_ATTACHMENTS_ENABLED: if (getValueInt() == 0) { - canSupport = false; + supported = false; } break; // The following policies, if true, can't be supported at the moment @@ -105,18 +109,21 @@ public class ProvisionParser extends Parser { case Tags.PROVISION_DEVICE_PASSWORD_HISTORY: case Tags.PROVISION_MAX_ATTACHMENT_SIZE: if (getValueInt() == 1) { - canSupport = false; + supported = false; } break; default: skipTag(); } + + if (!supported) { + log("Policy not supported: " + tag); + mIsSupportable = false; + } } - if (canSupport) { - mPolicySet = new SecurityPolicy.PolicySet(minPasswordLength, passwordMode, + mPolicySet = new SecurityPolicy.PolicySet(minPasswordLength, passwordMode, maxPasswordFails, maxScreenLockTime, true); - } } class ShadowPolicySet { @@ -318,10 +325,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 04e8367f6..7d6a8ee2f 100644 --- a/tests/src/com/android/email/SecurityPolicyTests.java +++ b/tests/src/com/android/email/SecurityPolicyTests.java @@ -88,6 +88,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 */ @@ -214,27 +235,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 1832d5ae1..5a6e64928 100644 --- a/tests/src/com/android/exchange/EasSyncServiceTests.java +++ b/tests/src/com/android/exchange/EasSyncServiceTests.java @@ -17,6 +17,12 @@ 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; @@ -119,4 +125,41 @@ import java.io.IOException; assertEquals(100, svc.mPingForceHeartbeat); assertFalse(svc.mPingHeartbeatDropped); } + + 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()); } } From 81fbc1e1b911d7b30a1e6e767478f677dfaebcd0 Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Mon, 2 Aug 2010 13:47:23 -0700 Subject: [PATCH 5/7] Handle inactivity timeout > maximum allowed properly * In a recent change, we mistakenly removed the logic for handling too-long inactivity timeouts; we should just fall back to the maximum since this is stricter than what we're being asked to enforce * Restore this logic and update the unit test * The regression was caused by change Ida5663a9, to wit: Backport: Handle "Allow non-provisionable devices" properly Bug: 2886746 Change-Id: I99cf9a37441b80477cc1c2c7ec2a78f8a14a83da --- src/com/android/email/SecurityPolicy.java | 3 --- .../com/android/email/SecurityPolicyTests.java | 17 ++++++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/com/android/email/SecurityPolicy.java b/src/com/android/email/SecurityPolicy.java index 076949b82..bacb39b53 100644 --- a/src/com/android/email/SecurityPolicy.java +++ b/src/com/android/email/SecurityPolicy.java @@ -461,9 +461,6 @@ public class SecurityPolicy { 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/tests/src/com/android/email/SecurityPolicyTests.java b/tests/src/com/android/email/SecurityPolicyTests.java index 7d6a8ee2f..1e25050ca 100644 --- a/tests/src/com/android/email/SecurityPolicyTests.java +++ b/tests/src/com/android/email/SecurityPolicyTests.java @@ -33,7 +33,10 @@ import android.test.suitebuilder.annotation.SmallTest; /** * This is a series of unit tests for backup/restore of the SecurityPolicy class. - */ + * + * You can run this entire test case with: + * runtest -c com.android.email.SecurityPolicyTests email +*/ @MediumTest public class SecurityPolicyTests extends ProviderTestCase2 { @@ -101,12 +104,12 @@ public class SecurityPolicyTests extends ProviderTestCase2 { 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) { - } + PolicySet ps = new PolicySet(0, PolicySet.PASSWORD_MODE_NONE, 0, + PolicySet.SCREEN_LOCK_TIME_MAX + 1, false); + assertEquals(PolicySet.SCREEN_LOCK_TIME_MAX, ps.getMaxScreenLockTime()); + ps = new PolicySet(0, PolicySet.PASSWORD_MODE_NONE, + PolicySet.PASSWORD_MAX_FAILS_MAX + 1, 0, false); + assertEquals(PolicySet.PASSWORD_MAX_FAILS_MAX, ps.getMaxPasswordFails()); } /** From e87a3ae24c3f93a83b2e60f6694c17bb5787dcd6 Mon Sep 17 00:00:00 2001 From: Marc Blank Date: Fri, 23 Jul 2010 11:10:04 -0700 Subject: [PATCH 6/7] Release held mailboxes after policy refresh * When a sync fails due to a provisioning error (on initial sync or after policies are refreshed on the server), sync mailboxes go into a "hold" state until the security error is resolved. Meanwhile, the account mailbox handles provisioning. If this is NOT successful, we put a hold on the account and go through the UI steps of setting up security on the device. When this is done, we release the hold on the account, which releases the hold on the mailboxes. * If provisioning IS successful, however, a refresh of the existing settings would be an example, we do NOT release the adapters, and this is the bug we're seeing. * This CL simply causes any held mailboxes in a successfully provisioned to be released from the hold Bug: 2865623 Change-Id: I59e780e9bd4ea908182b786dfd0e5851f5bf5f3b --- src/com/android/exchange/EasSyncService.java | 2 ++ src/com/android/exchange/SyncManager.java | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/com/android/exchange/EasSyncService.java b/src/com/android/exchange/EasSyncService.java index a1a76dce5..1feaf01f6 100644 --- a/src/com/android/exchange/EasSyncService.java +++ b/src/com/android/exchange/EasSyncService.java @@ -1291,6 +1291,8 @@ public class EasSyncService extends AbstractSyncService { if (policyKey != null) { // Write the final policy key to the Account and say we've been successful ps.writeAccount(mAccount, policyKey, true, mContext); + // Release any mailboxes that might be in a security hold + SyncManager.releaseSecurityHold(mAccount); return true; } } else { diff --git a/src/com/android/exchange/SyncManager.java b/src/com/android/exchange/SyncManager.java index 7a56cbbd9..dcdf6db76 100644 --- a/src/com/android/exchange/SyncManager.java +++ b/src/com/android/exchange/SyncManager.java @@ -895,6 +895,18 @@ public class SyncManager extends Service implements Runnable { } } + /** + * Release security holds for the specified account + * @param account the account whose Mailboxes should be released from security hold + */ + static public void releaseSecurityHold(Account account) { + SyncManager syncManager = INSTANCE; + if (syncManager != null) { + syncManager.releaseSyncHolds(INSTANCE, AbstractSyncService.EXIT_SECURITY_FAILURE, + account); + } + } + /** * Release a specific type of hold (the reason) for the specified Account; if the account * is null, mailboxes from all accounts with the specified hold will be released From 97b8f590e1ccfaac4bd4a3d12144ec11e8a629d7 Mon Sep 17 00:00:00 2001 From: Andrew Stadler Date: Thu, 12 Aug 2010 14:56:32 -0700 Subject: [PATCH 7/7] Clear password related policies in PolicySet when p/w not required Merge from master of c263810b08943541135a24e2b7520692152455cc Bug: 2883736 Change-Id: Iec4ed0e320d67aee8a89092ac650c0960540057b --- src/com/android/email/SecurityPolicy.java | 39 +++++++++++-------- .../android/email/SecurityPolicyTests.java | 31 +++++++++------ 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/src/com/android/email/SecurityPolicy.java b/src/com/android/email/SecurityPolicy.java index bacb39b53..35613ece0 100644 --- a/src/com/android/email/SecurityPolicy.java +++ b/src/com/android/email/SecurityPolicy.java @@ -453,23 +453,30 @@ public class SecurityPolicy { */ public PolicySet(int minPasswordLength, int passwordMode, int maxPasswordFails, int maxScreenLockTime, boolean requireRemoteWipe) throws IllegalArgumentException { - // Check against hard limits - // EAS doesn't generate values outside these limits anyway - if (minPasswordLength > PASSWORD_LENGTH_MAX) { - throw new IllegalArgumentException("password length"); + // If we're not enforcing passwords, make sure we clean up related values, since EAS + // can send non-zero values for any or all of these + if (passwordMode == PASSWORD_MODE_NONE) { + maxPasswordFails = 0; + maxScreenLockTime = 0; + minPasswordLength = 0; + } else { + if ((passwordMode != PASSWORD_MODE_SIMPLE) && + (passwordMode != PASSWORD_MODE_STRONG)) { + throw new IllegalArgumentException("password mode"); + } + // The next value has a hard limit which cannot be supported if exceeded. + if (minPasswordLength > PASSWORD_LENGTH_MAX) { + throw new IllegalArgumentException("password length"); + } + // This value can be reduced (which actually increases security) if necessary + if (maxPasswordFails > PASSWORD_MAX_FAILS_MAX) { + maxPasswordFails = PASSWORD_MAX_FAILS_MAX; + } + // This value can be reduced (which actually increases security) if necessary + if (maxScreenLockTime > SCREEN_LOCK_TIME_MAX) { + maxScreenLockTime = SCREEN_LOCK_TIME_MAX; + } } - if (passwordMode < PASSWORD_MODE_NONE || passwordMode > PASSWORD_MODE_STRONG) { - throw new IllegalArgumentException("password mode"); - } - // This value can be reduced (which actually increases security) if necessary - if (maxPasswordFails > PASSWORD_MAX_FAILS_MAX) { - maxPasswordFails = PASSWORD_MAX_FAILS_MAX; - } - // This value can be reduced (which actually increases security) if necessary - if (maxScreenLockTime > SCREEN_LOCK_TIME_MAX) { - maxScreenLockTime = SCREEN_LOCK_TIME_MAX; - } - mMinPasswordLength = minPasswordLength; mPasswordMode = passwordMode; mMaxPasswordFails = maxPasswordFails; diff --git a/tests/src/com/android/email/SecurityPolicyTests.java b/tests/src/com/android/email/SecurityPolicyTests.java index 1e25050ca..5924bceba 100644 --- a/tests/src/com/android/email/SecurityPolicyTests.java +++ b/tests/src/com/android/email/SecurityPolicyTests.java @@ -95,7 +95,7 @@ public class SecurityPolicyTests extends ProviderTestCase2 { // 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); + new PolicySet(100, PolicySet.PASSWORD_MODE_SIMPLE, 0, 0, false); fail("Too-long password allowed"); } catch (IllegalArgumentException e) { } @@ -104,12 +104,19 @@ public class SecurityPolicyTests extends ProviderTestCase2 { fail("Illegal password mode allowed"); } catch (IllegalArgumentException e) { } - PolicySet ps = new PolicySet(0, PolicySet.PASSWORD_MODE_NONE, 0, + PolicySet ps = new PolicySet(0, PolicySet.PASSWORD_MODE_SIMPLE, 0, PolicySet.SCREEN_LOCK_TIME_MAX + 1, false); assertEquals(PolicySet.SCREEN_LOCK_TIME_MAX, ps.getMaxScreenLockTime()); - ps = new PolicySet(0, PolicySet.PASSWORD_MODE_NONE, + ps = new PolicySet(0, PolicySet.PASSWORD_MODE_SIMPLE, PolicySet.PASSWORD_MAX_FAILS_MAX + 1, 0, false); assertEquals(PolicySet.PASSWORD_MAX_FAILS_MAX, ps.getMaxPasswordFails()); + // All password related fields should be zero when password mode is NONE + // Illegal values for these fields should be ignored + ps = new PolicySet(999/*length*/, PolicySet.PASSWORD_MODE_NONE, + 999/*fails*/, 9999/*screenlock*/, false); + assertEquals(0, ps.mMinPasswordLength); + assertEquals(0, ps.mMaxScreenLockTime); + assertEquals(0, ps.mMaxPasswordFails); } /** @@ -165,7 +172,7 @@ public class SecurityPolicyTests extends ProviderTestCase2 { // pw length and pw mode - max logic - will *not* change because smaller #s here // fail count and lock timer - min logic - will change because smaller #s here // wipe required - OR logic - will change here because true - PolicySet p5in = new PolicySet(4, PolicySet.PASSWORD_MODE_NONE, 5, 6, true); + PolicySet p5in = new PolicySet(4, PolicySet.PASSWORD_MODE_SIMPLE, 5, 6, true); Account a5 = ProviderTestUtils.setupAccount("sec-5", false, mMockContext); p5in.writeAccount(a5, null, true, mMockContext); PolicySet p5out = sp.computeAggregatePolicy(); @@ -202,9 +209,9 @@ public class SecurityPolicyTests extends ProviderTestCase2 { */ @SmallTest public void testFieldIsolation() { - PolicySet p = new PolicySet(PolicySet.PASSWORD_LENGTH_MAX, 0, 0, 0, false); + PolicySet p = new PolicySet(PolicySet.PASSWORD_LENGTH_MAX, PolicySet.PASSWORD_MODE_SIMPLE, + 0, 0, false); assertEquals(PolicySet.PASSWORD_LENGTH_MAX, p.mMinPasswordLength); - assertEquals(0, p.mPasswordMode); assertEquals(0, p.mMaxPasswordFails); assertEquals(0, p.mMaxScreenLockTime); assertFalse(p.mRequireRemoteWipe); @@ -212,27 +219,27 @@ public class SecurityPolicyTests extends ProviderTestCase2 { p = new PolicySet(0, PolicySet.PASSWORD_MODE_STRONG, 0, 0, false); assertEquals(0, p.mMinPasswordLength); assertEquals(PolicySet.PASSWORD_MODE_STRONG, p.mPasswordMode); + assertEquals(0, p.mMinPasswordLength); assertEquals(0, p.mMaxPasswordFails); assertEquals(0, p.mMaxScreenLockTime); assertFalse(p.mRequireRemoteWipe); - p = new PolicySet(0, 0, PolicySet.PASSWORD_MAX_FAILS_MAX, 0, false); + p = new PolicySet(0, PolicySet.PASSWORD_MODE_SIMPLE, PolicySet.PASSWORD_MAX_FAILS_MAX, 0, + false); assertEquals(0, p.mMinPasswordLength); - assertEquals(0, p.mPasswordMode); assertEquals(PolicySet.PASSWORD_MAX_FAILS_MAX, p.mMaxPasswordFails); assertEquals(0, p.mMaxScreenLockTime); assertFalse(p.mRequireRemoteWipe); - p = new PolicySet(0, 0, 0, PolicySet.SCREEN_LOCK_TIME_MAX, false); + p = new PolicySet(0, PolicySet.PASSWORD_MODE_SIMPLE, 0, PolicySet.SCREEN_LOCK_TIME_MAX, + false); assertEquals(0, p.mMinPasswordLength); - assertEquals(0, p.mPasswordMode); assertEquals(0, p.mMaxPasswordFails); assertEquals(PolicySet.SCREEN_LOCK_TIME_MAX, p.mMaxScreenLockTime); assertFalse(p.mRequireRemoteWipe); - p = new PolicySet(0, 0, 0, 0, true); + p = new PolicySet(0, PolicySet.PASSWORD_MODE_NONE, 0, 0, true); assertEquals(0, p.mMinPasswordLength); - assertEquals(0, p.mPasswordMode); assertEquals(0, p.mMaxPasswordFails); assertEquals(0, p.mMaxScreenLockTime); assertTrue(p.mRequireRemoteWipe);