From 907fa8143aaa55075e6d298e6cb9417fe7f4ca01 Mon Sep 17 00:00:00 2001 From: Hanxiao Liu Date: Fri, 7 Aug 2020 17:29:53 +0800 Subject: [PATCH] Enhance auth manager error handling (#4533) * Enhance auth manager error handling * Fix checkstyle * Send telemetries for authentication * Fix telemetry issue --- .../authmanage/AuthMethodManager.java | 146 +++++++++--------- .../authmanage/models/AuthMethodDetails.java | 5 +- .../telemetry/AppInsightsClient.java | 13 +- .../telemetry/TelemetryConstants.java | 1 + .../telemetrywrapper/CommonUtil.java | 29 +++- .../telemetrywrapper/TelemetryManager.java | 5 + 6 files changed, 116 insertions(+), 83 deletions(-) diff --git a/Utils/azuretools-core/src/com/microsoft/azuretools/authmanage/AuthMethodManager.java b/Utils/azuretools-core/src/com/microsoft/azuretools/authmanage/AuthMethodManager.java index 51c4a84687..43f1e25122 100644 --- a/Utils/azuretools-core/src/com/microsoft/azuretools/authmanage/AuthMethodManager.java +++ b/Utils/azuretools-core/src/com/microsoft/azuretools/authmanage/AuthMethodManager.java @@ -30,37 +30,40 @@ import com.microsoft.azuretools.azurecommons.helpers.Nullable; import com.microsoft.azuretools.sdkmanage.AzureManager; import com.microsoft.azuretools.sdkmanage.ServicePrincipalAzureManager; +import com.microsoft.azuretools.telemetrywrapper.ErrorType; +import com.microsoft.azuretools.telemetrywrapper.EventType; +import com.microsoft.azuretools.telemetrywrapper.EventUtil; import com.microsoft.azuretools.utils.AzureUIRefreshCore; import com.microsoft.azuretools.utils.AzureUIRefreshEvent; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.HashSet; import java.util.Set; import java.util.logging.Logger; import static com.microsoft.azuretools.Constants.FILE_NAME_AUTH_METHOD_DETAILS; +import static com.microsoft.azuretools.telemetry.TelemetryConstants.ACCOUNT; +import static com.microsoft.azuretools.telemetry.TelemetryConstants.RESIGNIN; +import static com.microsoft.azuretools.telemetry.TelemetryConstants.SIGNIN_METHOD; public class AuthMethodManager { private static final Logger LOGGER = Logger.getLogger(AuthMethodManager.class.getName()); - private static final String CANNOT_GET_AZURE_MANAGER = "Cannot get Azure Manager. " - + "Please check if you have already signed in."; - private static final String CANNOT_GET_AZURE_BY_SID = "Cannot get Azure with Subscription ID: %s. " - + "Please check if you have already signed in with this Subscription."; + private static final String CANNOT_GET_AZURE_MANAGER = "Cannot get Azure Manager. " + + "Please check if you have already signed in."; + private static final String CANNOT_GET_AZURE_BY_SID = "Cannot get Azure with Subscription ID: %s. " + + "Please check if you have already signed in with this Subscription."; + private static final String FAILED_TO_GET_AZURE_MANAGER_INSTANCE = "Failed to get an AzureManager instance " + + "for AuthMethodDetails: %s with error %s"; private AuthMethodDetails authMethodDetails; private volatile AzureManager azureManager; - private Set signInEventListeners = new HashSet<>(); - private Set signOutEventListeners = new HashSet<>(); + private final Set signInEventListeners = new HashSet<>(); + private final Set signOutEventListeners = new HashSet<>(); - private AuthMethodManager() { - final AuthMethodDetails savedAuthMethodDetails = loadSettings(); - authMethodDetails = savedAuthMethodDetails.getAuthMethod() == null ? new AuthMethodDetails() : - savedAuthMethodDetails.getAuthMethod().restoreAuth(savedAuthMethodDetails); - } - - private static class LazyHolder { - static final AuthMethodManager INSTANCE = new AuthMethodManager(); + private AuthMethodManager(AuthMethodDetails authMethodDetails) { + this.authMethodDetails = authMethodDetails; } public static AuthMethodManager getInstance() { @@ -86,27 +89,19 @@ public AppPlatformManager getAzureSpringCloudClient(String sid) throws IOExcepti } public void addSignInEventListener(Runnable l) { - if (!signInEventListeners.contains(l)) { - signInEventListeners.add(l); - } + signInEventListeners.add(l); } public void removeSignInEventListener(Runnable l) { - if (signInEventListeners.contains(l)) { - signInEventListeners.remove(l); - } + signInEventListeners.remove(l); } public void addSignOutEventListener(Runnable l) { - if (!signOutEventListeners.contains(l)) { - signOutEventListeners.add(l); - } + signOutEventListeners.add(l); } public void removeSignOutEventListener(Runnable l) { - if (signOutEventListeners.contains(l)) { - signOutEventListeners.remove(l); - } + signOutEventListeners.remove(l); } public void notifySignInEventListener() { @@ -132,29 +127,46 @@ public AzureManager getAzureManager() throws IOException { return getAzureManager(getAuthMethod()); } - private synchronized AzureManager getAzureManager(final AuthMethod authMethod) throws IOException { - AzureManager localAzureManagerRef = azureManager; - - if (localAzureManagerRef == null) { - try { - localAzureManagerRef = authMethod.createAzureManager(getAuthMethodDetails()); - } catch (RuntimeException ex) { - LOGGER.info(String.format( - "Failed to get an AzureManager instance for AuthMethodDetails: %s with error %s", - getAuthMethodDetails(), ex.getMessage())); - - cleanAll(); - } + public void signOut() throws IOException { + cleanAll(); + notifySignOutEventListener(); + } - azureManager = localAzureManagerRef; + public boolean isSignedIn() { + try { + return getAzureManager() != null; + } catch (IOException e) { + return false; } + } - return localAzureManagerRef; + public AuthMethod getAuthMethod() { + return authMethodDetails == null ? null : authMethodDetails.getAuthMethod(); } - public void signOut() throws IOException { + public AuthMethodDetails getAuthMethodDetails() { + return this.authMethodDetails; + } + + public synchronized void setAuthMethodDetails(AuthMethodDetails authMethodDetails) throws IOException { cleanAll(); - notifySignOutEventListener(); + this.authMethodDetails = authMethodDetails; + saveSettings(); + } + + private synchronized AzureManager getAzureManager(final AuthMethod authMethod) throws IOException { + if (authMethod == null) { + return null; + } + if (azureManager == null) { + try { + azureManager = authMethod.createAzureManager(getAuthMethodDetails()); + } catch (RuntimeException ex) { + LOGGER.info(String.format(FAILED_TO_GET_AZURE_MANAGER_INSTANCE, getAuthMethodDetails(), ex.getMessage())); + cleanAll(); + } + } + return azureManager; } private synchronized void cleanAll() throws IOException { @@ -171,30 +183,34 @@ private synchronized void cleanAll() throws IOException { saveSettings(); } - public boolean isSignedIn() { - try { - return getAzureManager() != null; - } catch (IOException e) { - return false; - } - } - - public AuthMethod getAuthMethod() { - return authMethodDetails.getAuthMethod(); + private void saveSettings() throws IOException { + System.out.println("saving authMethodDetails..."); + String sd = JsonHelper.serialize(authMethodDetails); + FileStorage fs = new FileStorage(FILE_NAME_AUTH_METHOD_DETAILS, CommonSettings.getSettingsBaseDir()); + fs.write(sd.getBytes(StandardCharsets.UTF_8)); } - public AuthMethodDetails getAuthMethodDetails() { - return this.authMethodDetails; + private static class LazyHolder { + static final AuthMethodManager INSTANCE = initAuthMethodManagerFromSettings(); } - public synchronized void setAuthMethodDetails(AuthMethodDetails authMethodDetails) throws IOException { - cleanAll(); - this.authMethodDetails = authMethodDetails; - saveSettings(); - //if (isSignedIn()) notifySignInEventListener(); + private static AuthMethodManager initAuthMethodManagerFromSettings() { + return EventUtil.executeWithLog(ACCOUNT, RESIGNIN, operation -> { + try { + final AuthMethodDetails savedAuthMethodDetails = loadSettings(); + final AuthMethodDetails authMethodDetails = savedAuthMethodDetails.getAuthMethod() == null ? + new AuthMethodDetails() : savedAuthMethodDetails.getAuthMethod().restoreAuth(savedAuthMethodDetails); + final String authMethod = authMethodDetails.getAuthMethod() == null ? "Empty" : authMethodDetails.getAuthMethod().name(); + EventUtil.logEvent(EventType.info, operation, Collections.singletonMap(SIGNIN_METHOD, authMethod)); + return new AuthMethodManager(authMethodDetails); + } catch (RuntimeException ignore) { + EventUtil.logError(operation, ErrorType.systemError, ignore, null, null); + return new AuthMethodManager(new AuthMethodDetails()); + } + }); } - private AuthMethodDetails loadSettings() { + private static AuthMethodDetails loadSettings() { System.out.println("loading authMethodDetails..."); try { FileStorage fs = new FileStorage(FILE_NAME_AUTH_METHOD_DETAILS, CommonSettings.getSettingsBaseDir()); @@ -204,18 +220,10 @@ private AuthMethodDetails loadSettings() { System.out.println(FILE_NAME_AUTH_METHOD_DETAILS + " is empty"); return new AuthMethodDetails(); } - return JsonHelper.deserialize(AuthMethodDetails.class, json); } catch (IOException ignored) { System.out.println("Failed to loading authMethodDetails settings. Use defaults."); return new AuthMethodDetails(); } } - - private void saveSettings() throws IOException { - System.out.println("saving authMethodDetails..."); - String sd = JsonHelper.serialize(authMethodDetails); - FileStorage fs = new FileStorage(FILE_NAME_AUTH_METHOD_DETAILS, CommonSettings.getSettingsBaseDir()); - fs.write(sd.getBytes(StandardCharsets.UTF_8)); - } } diff --git a/Utils/azuretools-core/src/com/microsoft/azuretools/authmanage/models/AuthMethodDetails.java b/Utils/azuretools-core/src/com/microsoft/azuretools/authmanage/models/AuthMethodDetails.java index e1065abe83..2e0debb966 100644 --- a/Utils/azuretools-core/src/com/microsoft/azuretools/authmanage/models/AuthMethodDetails.java +++ b/Utils/azuretools-core/src/com/microsoft/azuretools/authmanage/models/AuthMethodDetails.java @@ -22,11 +22,10 @@ package com.microsoft.azuretools.authmanage.models; +import com.microsoft.azuretools.authmanage.AuthMethod; import org.codehaus.jackson.annotate.JsonIgnoreProperties; import org.codehaus.jackson.annotate.JsonProperty; -import com.microsoft.azuretools.authmanage.AuthMethod; - /** * Created by shch on 10/8/2016. */ @@ -47,7 +46,6 @@ public class AuthMethodDetails { // for jackson json public AuthMethodDetails() { - this.authMethod = AuthMethod.AD; } public AuthMethod getAuthMethod() { @@ -74,7 +72,6 @@ public void setCredFilePath(String credFilePath) { this.credFilePath = credFilePath; } - public String getAzureEnv() { return azureEnv; } diff --git a/Utils/azuretools-core/src/com/microsoft/azuretools/telemetry/AppInsightsClient.java b/Utils/azuretools-core/src/com/microsoft/azuretools/telemetry/AppInsightsClient.java index 48fa9678c0..d0267cf6d2 100644 --- a/Utils/azuretools-core/src/com/microsoft/azuretools/telemetry/AppInsightsClient.java +++ b/Utils/azuretools-core/src/com/microsoft/azuretools/telemetry/AppInsightsClient.java @@ -26,6 +26,7 @@ import com.microsoft.azuretools.adauth.StringUtils; import com.microsoft.azuretools.azurecommons.helpers.Nullable; import com.microsoft.azuretools.telemetrywrapper.TelemetryManager; + import java.util.HashMap; import java.util.Iterator; import java.util.Map; @@ -47,7 +48,7 @@ public enum EventType { Azure } - public static String getInstallationId(){ + public static String getInstallationId() { return configuration == null ? null : configuration.installationId(); } @@ -77,7 +78,8 @@ public static void createByType(final EventType eventType, final String objectNa createByType(eventType, objectName, action, properties, false); } - public static void createByType(final EventType eventType, final String objectName, final String action, final Map properties, final boolean force) { + public static void createByType(final EventType eventType, final String objectName, final String action, final Map properties, + final boolean force) { if (!isAppInsightsClientAvailable()) return; @@ -108,7 +110,7 @@ public static void create(String eventName, String version, @Nullable Map myProperties, - Map metrics, boolean force) { + Map metrics, boolean force) { if (isAppInsightsClientAvailable() && configuration.validated()) { String prefValue = configuration.preferenceVal(); if (prefValue == null || prefValue.isEmpty() || prefValue.equalsIgnoreCase("true") || force) { @@ -128,7 +130,7 @@ private static Map buildProperties(String version, Map> iter = properties.entrySet().iterator(); iter.hasNext(); ) { + for (Iterator> iter = properties.entrySet().iterator(); iter.hasNext();) { Map.Entry entry = iter.next(); if (StringUtils.isNullOrEmpty(entry.getKey()) || StringUtils.isNullOrEmpty(entry.getValue())) { iter.remove(); @@ -177,7 +179,7 @@ public static void createFTPEvent(String eventName, String uri, String appName, properties.put("Installation ID", instID); } } - synchronized(TelemetryClientSingleton.class){ + synchronized (TelemetryClientSingleton.class) { telemetry.trackEvent(eventName, properties, null); telemetry.flush(); } @@ -192,6 +194,7 @@ private static void initTelemetryManager() { TelemetryManager.getInstance().setCommonProperties(buildProperties("", new HashMap<>())); TelemetryManager.getInstance().setTelemetryClient(TelemetryClientSingleton.getTelemetry()); TelemetryManager.getInstance().setEventNamePrefix(configuration.eventName()); + TelemetryManager.getInstance().sendCachedTelemetries(); } catch (Exception ignore) { } } diff --git a/Utils/azuretools-core/src/com/microsoft/azuretools/telemetry/TelemetryConstants.java b/Utils/azuretools-core/src/com/microsoft/azuretools/telemetry/TelemetryConstants.java index 7c36e897e7..13f1a80e40 100644 --- a/Utils/azuretools-core/src/com/microsoft/azuretools/telemetry/TelemetryConstants.java +++ b/Utils/azuretools-core/src/com/microsoft/azuretools/telemetry/TelemetryConstants.java @@ -132,6 +132,7 @@ public class TelemetryConstants { public static final String REFRESH_METADATA = "refresh"; public static final String SIGNIN = "signin"; public static final String SIGNOUT = "signout"; + public static final String RESIGNIN = "re-signin"; public static final String SELECT_SUBSCRIPTIONS = "select-subscriptions"; public static final String GET_SUBSCRIPTIONS = "get-subscriptions"; public static final String REPORT_ISSUES = "report-issues"; diff --git a/Utils/azuretools-core/src/com/microsoft/azuretools/telemetrywrapper/CommonUtil.java b/Utils/azuretools-core/src/com/microsoft/azuretools/telemetrywrapper/CommonUtil.java index 8a612c7c4c..5dd2163a50 100644 --- a/Utils/azuretools-core/src/com/microsoft/azuretools/telemetrywrapper/CommonUtil.java +++ b/Utils/azuretools-core/src/com/microsoft/azuretools/telemetrywrapper/CommonUtil.java @@ -24,9 +24,12 @@ import com.microsoft.applicationinsights.TelemetryClient; import com.microsoft.azuretools.adauth.StringUtils; +import org.apache.commons.lang3.tuple.MutableTriple; import org.joda.time.Instant; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; public class CommonUtil { @@ -42,6 +45,7 @@ public class CommonUtil { public static final String SERVICE_NAME = "serviceName"; public static final String TIMESTAMP = "timestamp"; public static TelemetryClient client; + private static List> cachedEvents = new ArrayList<>(); public static Map mergeProperties(Map properties) { Map commonProperties = TelemetryManager.getInstance().getCommonProperties(); @@ -52,20 +56,35 @@ public static Map mergeProperties(Map properties return merged; } - public synchronized static void sendTelemetry(EventType eventType, String serviceName, Map properties, + public static synchronized void sendTelemetry(EventType eventType, String serviceName, Map properties, Map metrics) { Map mutableProps = properties == null ? new HashMap<>() : new HashMap<>(properties); // Tag UTC time as timestamp mutableProps.put(TIMESTAMP, Instant.now().toString()); + if (!StringUtils.isNullOrEmpty(serviceName)) { + mutableProps.put(SERVICE_NAME, serviceName); + } + if (client != null) { + final String eventName = getFullEventName(eventType); + client.trackEvent(eventName, mutableProps, metrics); + client.flush(); + } else { + cacheEvents(eventType, mutableProps, metrics); + } + } + + public static void clearCachedEvents() { if (client != null) { - if (!StringUtils.isNullOrEmpty(serviceName)) { - mutableProps.put(SERVICE_NAME, serviceName); - } - client.trackEvent(getFullEventName(eventType), mutableProps, metrics); + cachedEvents.forEach(triple -> client.trackEvent(getFullEventName(triple.left), triple.middle, triple.right)); client.flush(); + cachedEvents.clear(); } } + private static void cacheEvents(EventType eventType, Map mutableProps, Map metrics) { + cachedEvents.add(MutableTriple.of(eventType, mutableProps, metrics)); + } + private static String getFullEventName(EventType eventType) { return TelemetryManager.getInstance().getEventNamePrefix() + "/" + eventType.name(); } diff --git a/Utils/azuretools-core/src/com/microsoft/azuretools/telemetrywrapper/TelemetryManager.java b/Utils/azuretools-core/src/com/microsoft/azuretools/telemetrywrapper/TelemetryManager.java index 9842cfc21b..e612b72d8a 100644 --- a/Utils/azuretools-core/src/com/microsoft/azuretools/telemetrywrapper/TelemetryManager.java +++ b/Utils/azuretools-core/src/com/microsoft/azuretools/telemetrywrapper/TelemetryManager.java @@ -23,6 +23,7 @@ package com.microsoft.azuretools.telemetrywrapper; import com.microsoft.applicationinsights.TelemetryClient; + import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -55,6 +56,10 @@ public void setEventNamePrefix(String eventNamePrefix) { this.eventNamePrefix = eventNamePrefix; } + public void sendCachedTelemetries() { + CommonUtil.clearCachedEvents(); + } + public Map getCommonProperties() { return commonProperties; }