From 224948fd7e98d96681748ce83018719e1e17515d Mon Sep 17 00:00:00 2001 From: Weng Su Date: Mon, 9 Nov 2020 06:33:59 +0800 Subject: [PATCH] Fix the bug of losing Wi-Fi certificate when editing - When the saved certificate is loaded into the UI, the EAP method spinner will trigger a redundant item selection event to refresh the certificate to the default value. - Filter out redundant item selection event of the EAP method spinner. - Analysis report: https://docs.google.com/document/d/1uzyO0NQsT0PVT-ZKbWtDTt4KRXvm3L994MmZTXZ4d5Y/edit?usp=sharing Bug: 161569880 Test: make RunSettingsRoboTests ROBOTEST_FILTER=WifiConfigController2Test Merged-In: I947fb7668ffa7e9ed8c150fe14e6ae9d7a67393c Change-Id: I00b73b4bfc078aca37396aab4c802a3de64318cf --- .../settings/wifi/WifiConfigController2.java | 8 +- .../wifi/WifiConfigController2Test.java | 86 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/src/com/android/settings/wifi/WifiConfigController2.java b/src/com/android/settings/wifi/WifiConfigController2.java index 7ae6061c4df..a177f1ac2fc 100644 --- a/src/com/android/settings/wifi/WifiConfigController2.java +++ b/src/com/android/settings/wifi/WifiConfigController2.java @@ -164,6 +164,7 @@ public class WifiConfigController2 implements TextWatcher, private ScrollView mDialogContainer; private Spinner mSecuritySpinner; @VisibleForTesting Spinner mEapMethodSpinner; + private int mLastShownEapMethod; @VisibleForTesting Spinner mEapSimSpinner; // For EAP-SIM, EAP-AKA and EAP-AKA-PRIME. private Spinner mEapCaCertSpinner; private Spinner mEapOcspSpinner; @@ -1057,6 +1058,7 @@ public class WifiConfigController2 implements TextWatcher, final int eapMethod = enterpriseConfig.getEapMethod(); final int phase2Method = enterpriseConfig.getPhase2Method(); mEapMethodSpinner.setSelection(eapMethod); + mLastShownEapMethod = eapMethod; showEapFieldsByMethod(eapMethod); switch (eapMethod) { case Eap.PEAP: @@ -1627,7 +1629,11 @@ public class WifiConfigController2 implements TextWatcher, mSsidScanButton.setVisibility(View.GONE); } } else if (parent == mEapMethodSpinner) { - showSecurityFields(/* refreshEapMethods */ false, /* refreshCertificates */ true); + final int selectedItemPosition = mEapMethodSpinner.getSelectedItemPosition(); + if (mLastShownEapMethod != selectedItemPosition) { + mLastShownEapMethod = selectedItemPosition; + showSecurityFields(/* refreshEapMethods */ false, /* refreshCertificates */ true); + } } else if (parent == mEapCaCertSpinner) { showSecurityFields(/* refreshEapMethods */ false, /* refreshCertificates */ false); } else if (parent == mPhase2Spinner diff --git a/tests/robotests/src/com/android/settings/wifi/WifiConfigController2Test.java b/tests/robotests/src/com/android/settings/wifi/WifiConfigController2Test.java index 779044c77f5..869658264c2 100644 --- a/tests/robotests/src/com/android/settings/wifi/WifiConfigController2Test.java +++ b/tests/robotests/src/com/android/settings/wifi/WifiConfigController2Test.java @@ -18,7 +18,9 @@ package com.android.settings.wifi; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.robolectric.Shadows.shadowOf; @@ -32,6 +34,7 @@ import android.net.wifi.WifiEnterpriseConfig.Eap; import android.net.wifi.WifiEnterpriseConfig.Phase2; import android.net.wifi.WifiManager; import android.os.ServiceSpecificException; +import android.security.Credentials; import android.security.KeyStore; import android.telephony.SubscriptionInfo; import android.telephony.SubscriptionManager; @@ -78,6 +81,10 @@ public class WifiConfigController2Test { private KeyStore mKeyStore; private View mView; private Spinner mHiddenSettingsSpinner; + private Spinner mEapCaCertSpinner; + private Spinner mEapUserCertSpinner; + private String mUseSystemCertsString; + private String mDoNotProvideEapUserCertString; private ShadowSubscriptionManager mShadowSubscriptionManager; public WifiConfigController2 mController; @@ -98,6 +105,9 @@ public class WifiConfigController2Test { private static final String NUMBER_AND_CHARACTER_KEY = "123456abcd"; private static final String PARTIAL_NUMBER_AND_CHARACTER_KEY = "123456abc?"; private static final int DHCP = 0; + // Saved certificates + private static final String SAVED_CA_CERT = "saved CA cert"; + private static final String SAVED_USER_CERT = "saved user cert"; @Before public void setUp() { @@ -108,6 +118,11 @@ public class WifiConfigController2Test { mView = LayoutInflater.from(mContext).inflate(R.layout.wifi_dialog, null); final Spinner ipSettingsSpinner = mView.findViewById(R.id.ip_settings); mHiddenSettingsSpinner = mView.findViewById(R.id.hidden_settings); + mEapCaCertSpinner = mView.findViewById(R.id.ca_cert); + mEapUserCertSpinner = mView.findViewById(R.id.user_cert); + mUseSystemCertsString = mContext.getString(R.string.wifi_use_system_certs); + mDoNotProvideEapUserCertString = + mContext.getString(R.string.wifi_do_not_provide_eap_user_cert); ipSettingsSpinner.setSelection(DHCP); mShadowSubscriptionManager = shadowOf(mContext.getSystemService(SubscriptionManager.class)); @@ -835,4 +850,75 @@ public class WifiConfigController2Test { final WifiConfiguration wifiConfiguration = mController.getConfig(); assertThat(wifiConfiguration.carrierId).isEqualTo(carrierId); } + + @Test + public void loadCaCertificateValue_shouldPersistentAsDefault() { + setUpModifyingSavedCertificateConfigController(null, null); + + assertThat(mEapCaCertSpinner.getSelectedItem()).isEqualTo(mUseSystemCertsString); + } + + @Test + public void loadSavedCaCertificateValue_shouldBeCorrectValue() { + setUpModifyingSavedCertificateConfigController(SAVED_CA_CERT, null); + + assertThat(mEapCaCertSpinner.getSelectedItem()).isEqualTo(SAVED_CA_CERT); + } + + @Test + public void loadUserCertificateValue_shouldPersistentAsDefault() { + setUpModifyingSavedCertificateConfigController(null, null); + + assertThat(mEapUserCertSpinner.getSelectedItem()).isEqualTo(mDoNotProvideEapUserCertString); + } + + @Test + public void loadSavedUserCertificateValue_shouldBeCorrectValue() { + setUpModifyingSavedCertificateConfigController(null, SAVED_USER_CERT); + + assertThat(mEapUserCertSpinner.getSelectedItem()).isEqualTo(SAVED_USER_CERT); + } + + private void setUpModifyingSavedCertificateConfigController(String savedCaCertificate, + String savedUserCertificate) { + final WifiConfiguration mockWifiConfig = mock(WifiConfiguration.class); + final WifiEnterpriseConfig mockWifiEnterpriseConfig = mock(WifiEnterpriseConfig.class); + mockWifiConfig.enterpriseConfig = mockWifiEnterpriseConfig; + when(mWifiEntry.isSaved()).thenReturn(true); + when(mWifiEntry.getSecurity()).thenReturn(WifiEntry.SECURITY_EAP); + when(mWifiEntry.getWifiConfiguration()).thenReturn(mockWifiConfig); + when(mockWifiConfig.getIpConfiguration()).thenReturn(mock(IpConfiguration.class)); + when(mockWifiEnterpriseConfig.getEapMethod()).thenReturn(Eap.TLS); + if (savedCaCertificate != null) { + String[] savedCaCertificates = new String[]{savedCaCertificate}; + when(mockWifiEnterpriseConfig.getCaCertificateAliases()) + .thenReturn(savedCaCertificates); + when(mKeyStore.list(eq(Credentials.CA_CERTIFICATE), anyInt())) + .thenReturn(savedCaCertificates); + } + if (savedUserCertificate != null) { + String[] savedUserCertificates = new String[]{savedUserCertificate}; + when(mockWifiEnterpriseConfig.getClientCertificateAlias()) + .thenReturn(savedUserCertificate); + when(mKeyStore.list(eq(Credentials.USER_PRIVATE_KEY), anyInt())) + .thenReturn(savedUserCertificates); + } + + mController = new TestWifiConfigController2(mConfigUiBase, mView, mWifiEntry, + WifiConfigUiBase2.MODE_MODIFY); + + // Because Robolectric has a different behavior from normal flow. + // + // Normal flow: + // showSecurityFields start -> mEapMethodSpinner.setSelection + // -> showSecurityFields end -> mController.onItemSelected + // + // Robolectric flow: + // showSecurityFields start -> mEapMethodSpinner.setSelection + // -> mController.onItemSelected -> showSecurityFields end + // + // We need to add a redundant mEapMethodSpinner.setSelection here to verify whether the + // certificates are covered by mController.onItemSelected after showSecurityFields end. + mController.mEapMethodSpinner.setSelection(Eap.TLS); + } }