From 043e31b53d13fdbb4215e3d21671ef70350dee31 Mon Sep 17 00:00:00 2001 From: Daniel Reynaud Date: Wed, 3 Jun 2020 17:53:55 -0700 Subject: [PATCH] chore(logs): be more consistent with exception logging (#1237) --- .../security/anonymous/AnonymousConfig.groovy | 2 +- .../DefaultProviderLookupService.groovy | 4 +-- .../security/iap/IapAuthenticationFilter.java | 4 +-- .../SpinnakerUserInfoTokenServices.groovy | 3 +- .../GithubProviderTokenServices.groovy | 28 ++++++++----------- .../gate/security/saml/SamlSsoConfig.groovy | 3 +- .../gate/controllers/AuthController.groovy | 4 +-- .../gate/controllers/ManagedController.java | 2 +- ...509AuthenticationUserDetailsService.groovy | 3 +- 9 files changed, 24 insertions(+), 29 deletions(-) diff --git a/gate-core/src/main/groovy/com/netflix/spinnaker/gate/security/anonymous/AnonymousConfig.groovy b/gate-core/src/main/groovy/com/netflix/spinnaker/gate/security/anonymous/AnonymousConfig.groovy index 006b6fe4fe..3e073e32ac 100644 --- a/gate-core/src/main/groovy/com/netflix/spinnaker/gate/security/anonymous/AnonymousConfig.groovy +++ b/gate-core/src/main/groovy/com/netflix/spinnaker/gate/security/anonymous/AnonymousConfig.groovy @@ -85,7 +85,7 @@ class AnonymousConfig extends WebSecurityConfigurerAdapter { anonymousAllowedAccounts.removeAll(toRemove) anonymousAllowedAccounts.addAll(toAdd) } catch (Exception e) { - log.warn(ExceptionUtils.getStackTrace(e)) + log.warn("Error while updating anonymous accounts", e) } } } diff --git a/gate-core/src/main/groovy/com/netflix/spinnaker/gate/services/DefaultProviderLookupService.groovy b/gate-core/src/main/groovy/com/netflix/spinnaker/gate/services/DefaultProviderLookupService.groovy index de87577d7b..c37bd2200b 100644 --- a/gate-core/src/main/groovy/com/netflix/spinnaker/gate/services/DefaultProviderLookupService.groovy +++ b/gate-core/src/main/groovy/com/netflix/spinnaker/gate/services/DefaultProviderLookupService.groovy @@ -82,8 +82,8 @@ class DefaultProviderLookupService implements ProviderLookupService, AccountLook } } accountsCache.set(accounts) - } catch (Exception e) { - log.error("Unable to refresh account details cache, reason: ${e.message}") +z } catch (Exception e) { + log.error("Unable to refresh account details cache", e) } } diff --git a/gate-iap/src/main/java/com/netflix/spinnaker/gate/security/iap/IapAuthenticationFilter.java b/gate-iap/src/main/java/com/netflix/spinnaker/gate/security/iap/IapAuthenticationFilter.java index 8df3910bf5..8aa587b9a7 100644 --- a/gate-iap/src/main/java/com/netflix/spinnaker/gate/security/iap/IapAuthenticationFilter.java +++ b/gate-iap/src/main/java/com/netflix/spinnaker/gate/security/iap/IapAuthenticationFilter.java @@ -122,9 +122,7 @@ protected void doFilterInternal( session.setAttribute(SIGNATURE_ATTRIBUTE, jwt.getSignature()); } catch (Exception e) { - if (log.isDebugEnabled()) { - log.info("Could not verify JWT Token for request: " + request.getPathInfo(), e); - } + log.error("Could not verify JWT Token for request {}", request.getPathInfo(), e); } chain.doFilter(request, response); } diff --git a/gate-oauth2/src/main/groovy/com/netflix/spinnaker/gate/security/oauth2/SpinnakerUserInfoTokenServices.groovy b/gate-oauth2/src/main/groovy/com/netflix/spinnaker/gate/security/oauth2/SpinnakerUserInfoTokenServices.groovy index bf3786b6b7..fbbabd15d5 100644 --- a/gate-oauth2/src/main/groovy/com/netflix/spinnaker/gate/security/oauth2/SpinnakerUserInfoTokenServices.groovy +++ b/gate-oauth2/src/main/groovy/com/netflix/spinnaker/gate/security/oauth2/SpinnakerUserInfoTokenServices.groovy @@ -132,7 +132,8 @@ class SpinnakerUserInfoTokenServices implements ResourceServerTokenServices { username, roles.size(), roles, - fiatClientConfigurationProperties.legacyFallback + fiatClientConfigurationProperties.legacyFallback, + e ) id = id.withTag("success", false).withTag("fallback", fiatClientConfigurationProperties.legacyFallback) diff --git a/gate-oauth2/src/main/groovy/com/netflix/spinnaker/gate/security/oauth2/provider/GithubProviderTokenServices.groovy b/gate-oauth2/src/main/groovy/com/netflix/spinnaker/gate/security/oauth2/provider/GithubProviderTokenServices.groovy index e662000616..0aa44e252b 100644 --- a/gate-oauth2/src/main/groovy/com/netflix/spinnaker/gate/security/oauth2/provider/GithubProviderTokenServices.groovy +++ b/gate-oauth2/src/main/groovy/com/netflix/spinnaker/gate/security/oauth2/provider/GithubProviderTokenServices.groovy @@ -58,40 +58,34 @@ class GithubProviderTokenServices implements SpinnakerProviderTokenServices { boolean checkOrganization(String accessToken, String organizationsUrl, String organization) { try { - log.debug("Getting user organizations from: " + organizationsUrl) - OAuth2RestOperations restTemplate = this.restTemplate; + log.debug("Getting user organizations from URL {}", organizationsUrl) + OAuth2RestOperations restTemplate = this.restTemplate if (restTemplate == null) { - BaseOAuth2ProtectedResourceDetails resource = new BaseOAuth2ProtectedResourceDetails(); + BaseOAuth2ProtectedResourceDetails resource = new BaseOAuth2ProtectedResourceDetails() resource.setClientId(sso.clientId) restTemplate = new OAuth2RestTemplate(resource) } - OAuth2AccessToken existingToken = restTemplate.getOAuth2ClientContext() - .getAccessToken() + OAuth2AccessToken existingToken = restTemplate.getOAuth2ClientContext().getAccessToken() if (existingToken == null || !accessToken.equals(existingToken.getValue())) { - DefaultOAuth2AccessToken token = new DefaultOAuth2AccessToken( - accessToken) + DefaultOAuth2AccessToken token = new DefaultOAuth2AccessToken(accessToken) token.setTokenType(this.tokenType) restTemplate.getOAuth2ClientContext().setAccessToken(token) } - List> organizations = restTemplate - .getForEntity(organizationsUrl, List.class).getBody() + List> organizations = restTemplate.getForEntity(organizationsUrl, List.class).getBody() return githubOrganizationMember(organization, organizations) } - catch (Exception ex) { - log.warn("Could not fetch user organizations: " + ex.getClass() + ", " - + ex.getMessage()) - return Collections.singletonMap("error", - "Could not fetch user organizations") + catch (Exception e) { + log.warn("Could not fetch user organizations", e) + return Collections.singletonMap("error", "Could not fetch user organizations") } } boolean hasAllProviderRequirements(String token, Map details) { boolean hasRequirements = true if (requirements.organization != null && details.containsKey("organizations_url")) { - boolean orgMatch = checkOrganization(token, details['organizations_url'], - requirements.organization) + boolean orgMatch = checkOrganization(token, details['organizations_url'], requirements.organization) if (!orgMatch) { - log.debug("User does not include required organization: " + requirements.organization) + log.debug("User does not include required organization {}", requirements.organization) hasRequirements = false } } diff --git a/gate-saml/src/main/groovy/com/netflix/spinnaker/gate/security/saml/SamlSsoConfig.groovy b/gate-saml/src/main/groovy/com/netflix/spinnaker/gate/security/saml/SamlSsoConfig.groovy index 7a2f70c76e..4b94557ca0 100644 --- a/gate-saml/src/main/groovy/com/netflix/spinnaker/gate/security/saml/SamlSsoConfig.groovy +++ b/gate-saml/src/main/groovy/com/netflix/spinnaker/gate/security/saml/SamlSsoConfig.groovy @@ -241,7 +241,8 @@ class SamlSsoConfig extends WebSecurityConfigurerAdapter { username, roles.size(), roles, - fiatClientConfigurationProperties.legacyFallback + fiatClientConfigurationProperties.legacyFallback, + e ) id = id.withTag("success", false).withTag("fallback", fiatClientConfigurationProperties.legacyFallback) diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/AuthController.groovy b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/AuthController.groovy index b1d829f522..b7209c2ae9 100644 --- a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/AuthController.groovy +++ b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/AuthController.groovy @@ -129,8 +129,8 @@ class AuthController { URL toURL try { toURL = new URL(to) - } catch (MalformedURLException malEx) { - log.warn "Malformed redirect URL: $to\n${ExceptionUtils.getStackTrace(malEx)}" + } catch (MalformedURLException e) { + log.warn("Malformed redirect URL {}", to, e) return false } diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/ManagedController.java b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/ManagedController.java index f6284f486f..fd20e78f13 100644 --- a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/ManagedController.java +++ b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/ManagedController.java @@ -163,7 +163,7 @@ ResponseEntity validateManifest(@RequestBody DeliveryConfig manifest) { return ResponseEntity.badRequest() .body(objectMapper.readValue(e.getResponse().getBody().in(), Map.class)); } catch (Exception ex) { - log.error("Error parsing error response from keel: {}", ex.getMessage(), ex); + log.error("Error parsing error response from keel", ex); return ResponseEntity.badRequest().body(Collections.emptyMap()); } } else { diff --git a/gate-x509/src/main/groovy/com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsService.groovy b/gate-x509/src/main/groovy/com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsService.groovy index 107125ffe5..86e72d3c36 100644 --- a/gate-x509/src/main/groovy/com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsService.groovy +++ b/gate-x509/src/main/groovy/com/netflix/spinnaker/gate/security/x509/X509AuthenticationUserDetailsService.groovy @@ -191,7 +191,8 @@ class X509AuthenticationUserDetailsService implements AuthenticationUserDetailsS email, roles.size(), roles, - fiatClientConfigurationProperties.legacyFallback + fiatClientConfigurationProperties.legacyFallback, + e ) id = id.withTag("success", false).withTag("fallback", fiatClientConfigurationProperties.legacyFallback)