From 177d796efa09478d42641e42a2ecfe91399983de Mon Sep 17 00:00:00 2001 From: Iain McGinniss Date: Mon, 29 Feb 2016 13:47:59 -0800 Subject: [PATCH] Codacy errorprone report warning fixes - Tighten up scoping / Annotate @VisibleForTesting - Use Preconditions static imports consistently --- .../appauth/AuthorizationException.java | 20 ++++-- .../openid/appauth/AuthorizationRequest.java | 19 ++--- .../AuthorizationServiceDiscovery.java | 71 ++++++++++++++++++- .../openid/appauth/BrowserPackageHelper.java | 4 +- library/java/net/openid/appauth/JsonUtil.java | 24 +++---- library/java/net/openid/appauth/Logger.java | 2 +- .../java/net/openid/appauth/TokenRequest.java | 2 +- .../net/openid/appauth/TokenResponse.java | 4 +- 8 files changed, 114 insertions(+), 32 deletions(-) diff --git a/library/java/net/openid/appauth/AuthorizationException.java b/library/java/net/openid/appauth/AuthorizationException.java index 4d3b2d9f..2d5d8730 100644 --- a/library/java/net/openid/appauth/AuthorizationException.java +++ b/library/java/net/openid/appauth/AuthorizationException.java @@ -365,22 +365,27 @@ public static AuthorizationException byString(String error) { } } - static AuthorizationException generalEx(int code, @Nullable String errorDescription) { + private static AuthorizationException generalEx(int code, @Nullable String errorDescription) { return new AuthorizationException( TYPE_GENERAL_ERROR, code, null, errorDescription, null, null); } - static AuthorizationException authEx(int code, @Nullable String error) { + private static AuthorizationException authEx(int code, @Nullable String error) { return new AuthorizationException( TYPE_OAUTH_AUTHORIZATION_ERROR, code, error, null, null, null); } - static AuthorizationException tokenEx(int code, @Nullable String error) { + private static AuthorizationException tokenEx(int code, @Nullable String error) { return new AuthorizationException( TYPE_OAUTH_TOKEN_ERROR, code, error, null, null, null); } - static AuthorizationException fromTemplate( + /** + * Creates an exception based on one of the existing values defined in + * {@link GeneralErrors}, {@link AuthorizationRequestErrors} or {@link TokenRequestErrors}, + * providing a root cause. + */ + public static AuthorizationException fromTemplate( @NonNull AuthorizationException ex, @Nullable Throwable rootCause) { return new AuthorizationException( @@ -392,7 +397,12 @@ static AuthorizationException fromTemplate( rootCause); } - static AuthorizationException fromOAuthTemplate( + /** + * Creates an exception based on one of the existing values defined in + * {@link AuthorizationRequestErrors} or {@link TokenRequestErrors}, adding information + * retrieved from OAuth error response. + */ + public static AuthorizationException fromOAuthTemplate( @NonNull AuthorizationException ex, @Nullable String errorOverride, @Nullable String errorDescriptionOverride, diff --git a/library/java/net/openid/appauth/AuthorizationRequest.java b/library/java/net/openid/appauth/AuthorizationRequest.java index 8e8857f5..92ec8d86 100644 --- a/library/java/net/openid/appauth/AuthorizationRequest.java +++ b/library/java/net/openid/appauth/AuthorizationRequest.java @@ -15,7 +15,10 @@ package net.openid.appauth; import static net.openid.appauth.Preconditions.checkArgument; +import static net.openid.appauth.Preconditions.checkMapEntryFullyDefined; +import static net.openid.appauth.Preconditions.checkNotEmpty; import static net.openid.appauth.Preconditions.checkNotNull; +import static net.openid.appauth.Preconditions.checkNullOrNotEmpty; import android.net.Uri; import android.support.annotation.NonNull; @@ -357,7 +360,7 @@ public Builder( */ public Builder setAuthorizationServiceConfiguration( @NonNull AuthorizationServiceConfiguration configuration) { - mConfiguration = Preconditions.checkNotNull(configuration, + mConfiguration = checkNotNull(configuration, "configuration cannot be null"); return this; } @@ -475,7 +478,7 @@ public Builder setScopes(@Nullable Iterable scopes) { @NonNull public Builder setState(@Nullable String state) { if (state != null) { - Preconditions.checkArgument(!TextUtils.isEmpty(state), + checkArgument(!TextUtils.isEmpty(state), "state cannot be empty if defined"); } mState = state; @@ -523,15 +526,15 @@ public Builder setCodeVerifier( @Nullable String codeVerifierChallengeMethod) { if (codeVerifier != null) { CodeVerifierUtil.checkCodeVerifier(codeVerifier); - Preconditions.checkNotEmpty(codeVerifierChallenge, + checkNotEmpty(codeVerifierChallenge, "code verifier challenge cannot be null or empty if verifier is set"); - Preconditions.checkNotEmpty(codeVerifierChallengeMethod, + checkNotEmpty(codeVerifierChallengeMethod, "code verifier challenge method cannot be null or empty if verifier " + "is set"); } else { - Preconditions.checkArgument(codeVerifierChallenge == null, + checkArgument(codeVerifierChallenge == null, "code verifier challenge must be null if verifier is null"); - Preconditions.checkArgument(codeVerifierChallengeMethod == null, + checkArgument(codeVerifierChallengeMethod == null, "code verifier challenge method must be null if verifier is null"); } @@ -552,7 +555,7 @@ public Builder setCodeVerifier( */ @NonNull public Builder setResponseMode(@Nullable String responseMode) { - Preconditions.checkNullOrNotEmpty(responseMode, "responseMode must not be empty"); + checkNullOrNotEmpty(responseMode, "responseMode must not be empty"); mResponseMode = responseMode; return this; } @@ -570,7 +573,7 @@ public Builder setAdditionalParameters(@NonNull Map additionalPa checkNotNull(additionalParameters); mAdditionalParameters = new HashMap<>(); for (Entry entry : additionalParameters.entrySet()) { - Preconditions.checkMapEntryFullyDefined(entry, + checkMapEntryFullyDefined(entry, "additional parameters must have non-null keys and non-null values"); // TODO: check that the key name does not conflict with any "core" field names. mAdditionalParameters.put(entry.getKey(), entry.getValue()); diff --git a/library/java/net/openid/appauth/AuthorizationServiceDiscovery.java b/library/java/net/openid/appauth/AuthorizationServiceDiscovery.java index 58661baf..40919ecb 100644 --- a/library/java/net/openid/appauth/AuthorizationServiceDiscovery.java +++ b/library/java/net/openid/appauth/AuthorizationServiceDiscovery.java @@ -19,6 +19,7 @@ import android.net.Uri; import android.support.annotation.NonNull; import android.support.annotation.Nullable; +import android.support.annotation.VisibleForTesting; import net.openid.appauth.JsonUtil.BooleanField; import net.openid.appauth.JsonUtil.Field; @@ -41,66 +42,132 @@ */ public class AuthorizationServiceDiscovery { + @VisibleForTesting static final StringField ISSUER = str("issuer"); + + @VisibleForTesting static final UriField AUTHORIZATION_ENDPOINT = uri("authorization_endpoint"); + + @VisibleForTesting static final UriField TOKEN_ENDPOINT = uri("token_endpoint"); + + @VisibleForTesting static final UriField USERINFO_ENDPOINT = uri("userinfo_endpoint"); + + @VisibleForTesting static final UriField JWKS_URI = uri("jwks_uri"); + + @VisibleForTesting static final UriField REGISTRATION_ENDPOINT = uri("registration_endpoint"); + + @VisibleForTesting static final StringListField SCOPES_SUPPORTED = strList("scopes_supported"); + + @VisibleForTesting static final StringListField RESPONSE_TYPES_SUPPORTED = strList("response_types_supported"); + + @VisibleForTesting static final StringListField RESPONSE_MODES_SUPPORTED = strList("response_modes_supported"); + + @VisibleForTesting static final StringListField GRANT_TYPES_SUPPORTED = strList("grant_types_supported", Arrays.asList("authorization_code", "implicit")); + + @VisibleForTesting static final StringListField ACR_VALUES_SUPPORTED = strList("acr_values_supported"); + + @VisibleForTesting static final StringListField SUBJECT_TYPES_SUPPORTED = strList("subject_types_supported"); + @VisibleForTesting static final StringListField ID_TOKEN_SIGNING_ALG_VALUES_SUPPORTED = strList("id_token_signing_alg_values_supported"); + + @VisibleForTesting static final StringListField ID_TOKEN_ENCRYPTION_ALG_VALUES_SUPPORTED = strList("id_token_encryption_enc_values_supported"); + + @VisibleForTesting static final StringListField ID_TOKEN_ENCRYPTION_ENC_VALUES_SUPPORTED = strList("id_token_encryption_enc_values_supported"); + @VisibleForTesting static final StringListField USERINFO_SIGNING_ALG_VALUES_SUPPORTED = strList("userinfo_signing_alg_values_supported"); + + @VisibleForTesting static final StringListField USERINFO_ENCRYPTION_ALG_VALUES_SUPPORTED = strList("userinfo_encryption_alg_values_supported"); + + @VisibleForTesting static final StringListField USERINFO_ENCRYPTION_ENC_VALUES_SUPPORTED = strList("userinfo_encryption_enc_values_supported"); + @VisibleForTesting static final StringListField REQUEST_OBJECT_SIGNING_ALG_VALUES_SUPPORTED = strList("request_object_signing_alg_values_supported"); + + @VisibleForTesting static final StringListField REQUEST_OBJECT_ENCRYPTION_ALG_VALUES_SUPPORTED = strList("request_object_encryption_alg_values_supported"); + + @VisibleForTesting static final StringListField REQUEST_OBJECT_ENCRYPTION_ENC_VALUES_SUPPORTED = strList("request_object_encryption_enc_values_supported"); + @VisibleForTesting static final StringListField TOKEN_ENDPOINT_AUTH_METHODS_SUPPORTED = strList("token_endpoint_auth_methods_supported", Collections.singletonList("client_secret_basic")); + @VisibleForTesting static final StringListField TOKEN_ENDPOINT_AUTH_SIGNING_ALG_VALUES_SUPPORTED = strList("token_endpoint_auth_signing_alg_values_supported"); + @VisibleForTesting static final StringListField DISPLAY_VALUES_SUPPORTED = strList("display_values_supported"); + + @VisibleForTesting static final StringListField CLAIM_TYPES_SUPPORTED = strList("claim_types_supported", Collections.singletonList("normal")); + + @VisibleForTesting static final StringListField CLAIMS_SUPPORTED = strList("claims_supported"); + + @VisibleForTesting static final UriField SERVICE_DOCUMENTATION = uri("service_documentation"); + + @VisibleForTesting static final StringListField CLAIMS_LOCALES_SUPPORTED = strList("claims_locales_supported"); + + @VisibleForTesting static final StringListField UI_LOCALES_SUPPORTED = strList("ui_locales_supported"); + + @VisibleForTesting static final BooleanField CLAIMS_PARAMETER_SUPPORTED = bool("claims_parameter_supported", false); + + @VisibleForTesting static final BooleanField REQUEST_PARAMETER_SUPPORTED = bool("request_parameter_supported", false); + + @VisibleForTesting static final BooleanField REQUEST_URI_PARAMETER_SUPPORTED = bool("request_uri_parameter_supported", true); + + @VisibleForTesting static final BooleanField REQUIRE_REQUEST_URI_REGISTRATION = bool("require_request_uri_registration", false); + + @VisibleForTesting static final UriField OP_POLICY_URI = uri("op_policy_uri"); + + @VisibleForTesting static final UriField OP_TOS_URI = uri("op_tos_uri"); + /** + * The fields which are marked as mandatory in the OpenID discovery spec. + */ private static final List MANDATORY_METADATA = Arrays.asList( ISSUER.key, AUTHORIZATION_ENDPOINT.key, @@ -155,7 +222,7 @@ public String getMissingField() { * for the retrieval of a non-standard metadata value. Convenience methods are defined on this * class for all standard metadata values. */ - T get(Field field) { + private T get(Field field) { return JsonUtil.get(docJson, field); } @@ -164,7 +231,7 @@ T get(Field field) { * for the retrieval of a non-standard metadata value. Convenience methods are defined on this * class for all standard metadata values. */ - List get(JsonUtil.ListField field) { + private List get(JsonUtil.ListField field) { return JsonUtil.get(docJson, field); } diff --git a/library/java/net/openid/appauth/BrowserPackageHelper.java b/library/java/net/openid/appauth/BrowserPackageHelper.java index f9f8cf72..170d3b4b 100644 --- a/library/java/net/openid/appauth/BrowserPackageHelper.java +++ b/library/java/net/openid/appauth/BrowserPackageHelper.java @@ -35,8 +35,8 @@ */ class BrowserPackageHelper { - static final String SCHEME_HTTP = "http"; - static final String SCHEME_HTTPS = "https"; + private static final String SCHEME_HTTP = "http"; + private static final String SCHEME_HTTPS = "https"; /** * The service we expect to find on a web browser that indicates it supports custom tabs. diff --git a/library/java/net/openid/appauth/JsonUtil.java b/library/java/net/openid/appauth/JsonUtil.java index 1e6b8bdb..106976de 100644 --- a/library/java/net/openid/appauth/JsonUtil.java +++ b/library/java/net/openid/appauth/JsonUtil.java @@ -49,7 +49,7 @@ public static void put( try { json.put(field, value); } catch (JSONException ex) { - throw new RuntimeException("JSONException thrown in violation of contract, ex"); + throw new IllegalStateException("JSONException thrown in violation of contract, ex"); } } @@ -63,7 +63,7 @@ public static void put( try { json.put(field, value); } catch (JSONException ex) { - throw new RuntimeException("JSONException thrown in violation of contract", ex); + throw new IllegalStateException("JSONException thrown in violation of contract", ex); } } @@ -77,7 +77,7 @@ public static void put( try { json.put(field, value); } catch (JSONException ex) { - throw new RuntimeException("JSONException thrown in violation of contract", ex); + throw new IllegalStateException("JSONException thrown in violation of contract", ex); } } @@ -91,7 +91,7 @@ public static void put( try { json.put(field, value); } catch (JSONException ex) { - throw new RuntimeException("JSONException thrown in violation of contract", ex); + throw new IllegalStateException("JSONException thrown in violation of contract", ex); } } @@ -107,7 +107,7 @@ public static void putIfNotNull( try { json.put(field, value); } catch (JSONException ex) { - throw new RuntimeException("JSONException thrown in violation of contract", ex); + throw new IllegalStateException("JSONException thrown in violation of contract", ex); } } @@ -123,7 +123,7 @@ public static void putIfNotNull( try { json.put(field, value.toString()); } catch (JSONException ex) { - throw new RuntimeException("JSONException thrown in violation of contract", ex); + throw new IllegalStateException("JSONException thrown in violation of contract", ex); } } @@ -139,7 +139,7 @@ public static void putIfNotNull( try { json.put(field, value); } catch (JSONException ex) { - throw new RuntimeException("JSONException thrown in violation of contract", ex); + throw new IllegalStateException("JSONException thrown in violation of contract", ex); } } @@ -155,7 +155,7 @@ public static void putIfNotNull( try { json.put(field, value); } catch (JSONException ex) { - throw new RuntimeException("JSONException thrown in violation of contract", ex); + throw new IllegalStateException("JSONException thrown in violation of contract", ex); } } @@ -316,8 +316,8 @@ public static T get(JSONObject json, Field field) { return field.convert(json.getString(field.key)); } catch (JSONException e) { // all appropriate steps are taken above to avoid a JSONException. If it is still - // thrown, indicating an implementation change, throw a RuntimeException - throw new RuntimeException("unexpected JSONException", e); + // thrown, indicating an implementation change, throw an exception + throw new IllegalStateException("unexpected JSONException", e); } } @@ -339,8 +339,8 @@ public static List get(JSONObject json, ListField field) { return values; } catch (JSONException e) { // all appropriate steps are taken above to avoid a JSONException. If it is still - // thrown, indicating an implementation change, throw a RuntimeException - throw new RuntimeException("unexpected JSONException", e); + // thrown, indicating an implementation change, throw an excpetion + throw new IllegalStateException("unexpected JSONException", e); } } diff --git a/library/java/net/openid/appauth/Logger.java b/library/java/net/openid/appauth/Logger.java index 1fc57290..47ba6a3c 100644 --- a/library/java/net/openid/appauth/Logger.java +++ b/library/java/net/openid/appauth/Logger.java @@ -138,7 +138,7 @@ interface LogWrapper { * Default {@link LogWrapper} implementation, using {@link android.util.Log} static methods. */ private static final class AndroidLogWrapper implements LogWrapper { - static final AndroidLogWrapper INSTANCE = new AndroidLogWrapper(); + private static final AndroidLogWrapper INSTANCE = new AndroidLogWrapper(); private AndroidLogWrapper() {} diff --git a/library/java/net/openid/appauth/TokenRequest.java b/library/java/net/openid/appauth/TokenRequest.java index 0d5ca221..d791743f 100644 --- a/library/java/net/openid/appauth/TokenRequest.java +++ b/library/java/net/openid/appauth/TokenRequest.java @@ -228,7 +228,7 @@ public static final class Builder { private Uri mRedirectUri; @Nullable - String mScope; + private String mScope; @Nullable private String mAuthorizationCode; diff --git a/library/java/net/openid/appauth/TokenResponse.java b/library/java/net/openid/appauth/TokenResponse.java index 9f58ddf9..f33c2592 100644 --- a/library/java/net/openid/appauth/TokenResponse.java +++ b/library/java/net/openid/appauth/TokenResponse.java @@ -221,7 +221,9 @@ public Builder(@NonNull TokenRequest request) { // JSONException should only be thrown if a get() call is made for a key which // cannot be found. As all calls are guarded by has() calls, this exception should // therefore not be thrown. - throw new RuntimeException("JSONException thrown in violation of contract", ex); + throw new IllegalStateException( + "JSONException thrown in violation of contract", + ex); } }