You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by en...@apache.org on 2021/05/22 19:08:07 UTC
[sling-org-apache-sling-auth-form] branch master updated:
SLING-10411 resolve code quality warnings and issues (#2)
This is an automated email from the ASF dual-hosted git repository.
enorman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-auth-form.git
The following commit(s) were added to refs/heads/master by this push:
new 73d2210 SLING-10411 resolve code quality warnings and issues (#2)
73d2210 is described below
commit 73d221095c8877dd7b29397879c0a307b04fec00
Author: Eric Norman <er...@gmail.com>
AuthorDate: Sat May 22 12:08:02 2021 -0700
SLING-10411 resolve code quality warnings and issues (#2)
as reported by sonar
Upgrade to sling-bundle-parent version 41
Improve JaCoCo code coverage setup
---
pom.xml | 8 +-
.../auth/form/impl/AuthenticationFormServlet.java | 2 +-
.../auth/form/impl/FormAuthenticationHandler.java | 40 +++-----
.../form/impl/FormAuthenticationHandlerConfig.java | 26 ++---
.../auth/form/impl/FormLoginModulePlugin.java | 20 ++--
.../apache/sling/auth/form/impl/TokenStore.java | 114 ++++++++-------------
.../sling/auth/form/impl/jaas/FormCredentials.java | 1 +
.../sling/auth/form/impl/jaas/FormLoginModule.java | 9 +-
.../sling/auth/form/impl/jaas/JaasHelper.java | 2 +-
.../org/apache/sling/auth/form/FormReasonTest.java | 6 +-
.../form/impl/FormAuthenticationHandlerTest.java | 6 +-
.../sling/auth/form/impl/TokenStoreTest.java | 52 ++++++----
.../sling/auth/form/it/AuthFormTestSupport.java | 18 +++-
13 files changed, 144 insertions(+), 160 deletions(-)
diff --git a/pom.xml b/pom.xml
index d5bf9b5..745ade1 100644
--- a/pom.xml
+++ b/pom.xml
@@ -15,7 +15,7 @@
<parent>
<groupId>org.apache.sling</groupId>
<artifactId>sling-bundle-parent</artifactId>
- <version>38</version>
+ <version>41</version>
<relativePath />
</parent>
@@ -89,6 +89,10 @@
<dependencies>
<dependency>
+ <groupId>org.jetbrains</groupId>
+ <artifactId>annotations</artifactId>
+ </dependency>
+ <dependency>
<groupId>org.osgi</groupId>
<artifactId>org.osgi.service.component.annotations</artifactId>
</dependency>
@@ -180,6 +184,8 @@
<dependency>
<groupId>org.jmock</groupId>
<artifactId>jmock-junit4</artifactId>
+ <version>2.8.2</version>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
diff --git a/src/main/java/org/apache/sling/auth/form/impl/AuthenticationFormServlet.java b/src/main/java/org/apache/sling/auth/form/impl/AuthenticationFormServlet.java
index 595d71e..1ccef7c 100644
--- a/src/main/java/org/apache/sling/auth/form/impl/AuthenticationFormServlet.java
+++ b/src/main/java/org/apache/sling/auth/form/impl/AuthenticationFormServlet.java
@@ -35,7 +35,7 @@ import org.osgi.service.component.annotations.Component;
"service.description=Default Login Form for Form Based Authentication" })
public class AuthenticationFormServlet extends AbstractAuthenticationFormServlet {
- public static final String SERVLET_PATH = "/system/sling/form/login";
+ public static final String SERVLET_PATH = "/system/sling/form/login"; // NOSONAR
public static final String AUTH_REQUIREMENTS = "-" + SERVLET_PATH;
private static final long serialVersionUID = -1497963620502763188L;
diff --git a/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandler.java b/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandler.java
index 785a876..bbfc8df 100644
--- a/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandler.java
+++ b/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandler.java
@@ -21,6 +21,7 @@ package org.apache.sling.auth.form.impl;
import java.io.File;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
+import java.nio.charset.StandardCharsets;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.util.HashMap;
@@ -166,7 +167,7 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
* The resource resolver factory used to resolve the login form as a resource
*/
@Reference(policy = ReferencePolicy.DYNAMIC, cardinality = ReferenceCardinality.OPTIONAL)
- private volatile ResourceResolverFactory resourceResolverFactory;
+ private volatile ResourceResolverFactory resourceResolverFactory; // NOSONAR
/**
* If true the login form will be presented when the token expires.
@@ -248,13 +249,11 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
Resource loginFormResource = resourceResolver.resolve(loginForm);
Servlet loginFormServlet = loginFormResource.adaptTo(Servlet.class);
if (loginFormServlet != null) {
- try {
loginFormServlet.service(request, response);
return true;
+ }
} catch (ServletException e) {
log.error("Failed to include the form: " + loginForm, e);
- }
- }
} catch (LoginException e) {
log.error(
"Unable to get a resource resolver to include for the login resource. Will redirect instead.");
@@ -265,7 +264,7 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
}
}
- HashMap<String, String> params = new HashMap<String, String>();
+ HashMap<String, String> params = new HashMap<>();
params.put(Authenticator.LOGIN_RESOURCE, resource);
// append indication of previous login failure
@@ -445,13 +444,7 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
try {
authData = null;
authData = tokenStore.encode(expires, authInfo.getUser());
- } catch (InvalidKeyException e) {
- log.error(e.getMessage(), e);
- } catch (IllegalStateException e) {
- log.error(e.getMessage(), e);
- } catch (UnsupportedEncodingException e) {
- log.error(e.getMessage(), e);
- } catch (NoSuchAlgorithmException e) {
+ } catch (InvalidKeyException | IllegalStateException | NoSuchAlgorithmException e) {
log.error(e.getMessage(), e);
}
@@ -569,7 +562,7 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
*/
@Activate
protected void activate(FormAuthenticationHandlerConfig config, ComponentContext componentContext)
- throws InvalidKeyException, NoSuchAlgorithmException, IllegalStateException, UnsupportedEncodingException {
+ throws InvalidKeyException, NoSuchAlgorithmException, IllegalStateException {
this.jaasHelper = new JaasHelper(this, componentContext.getBundleContext(), config);
this.loginForm = config.form_login_form();
@@ -582,8 +575,8 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
defaultCookieDomain = null;
}
- final String authStorage = config.form_auth_storage();
- if (FormAuthenticationHandlerConfig.AUTH_STORAGE_SESSION_ATTRIBUTE.equals(authStorage)) {
+ final String formAuthStorage = config.form_auth_storage();
+ if (FormAuthenticationHandlerConfig.AUTH_STORAGE_SESSION_ATTRIBUTE.equals(formAuthStorage)) {
this.authStorage = new SessionStorage(authName);
log.info("Using HTTP Session store with attribute name {}", authName);
} else {
@@ -685,7 +678,7 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
String getUserId(final String authData) {
if (authData != null) {
String[] parts = TokenStore.split(authData);
- if (parts != null) {
+ if (parts.length == 3) {
return parts[2];
}
}
@@ -705,7 +698,7 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
updateCookie = true;
} else {
String[] parts = TokenStore.split(authData);
- if (parts != null && parts.length == 3) {
+ if (parts.length == 3) {
long cookieTime = Long.parseLong(parts[1].substring(1));
if (System.currentTimeMillis() + (sessionTimeout / 2) > cookieTime) {
updateCookie = true;
@@ -764,11 +757,7 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
// from it and reverse the base64 encoding
String value = cookie.getValue();
if (value.length() > 0) {
- try {
- return new String(Base64.decodeBase64(value), "UTF-8");
- } catch (UnsupportedEncodingException e1) {
- throw new RuntimeException(e1);
- }
+ return new String(Base64.decodeBase64(value), StandardCharsets.UTF_8);
}
}
}
@@ -781,12 +770,7 @@ public class FormAuthenticationHandler extends DefaultAuthenticationFeedbackHand
public void set(HttpServletRequest request, HttpServletResponse response, String authData,
AuthenticationInfo info) {
// base64 encode to handle any special characters
- String cookieValue;
- try {
- cookieValue = Base64.encodeBase64URLSafeString(authData.getBytes("UTF-8"));
- } catch (UnsupportedEncodingException e1) {
- throw new RuntimeException(e1);
- }
+ String cookieValue = Base64.encodeBase64URLSafeString(authData.getBytes(StandardCharsets.UTF_8));
// send the cookie to the response
String cookieDomain = (String) info.get(COOKIE_DOMAIN);
diff --git a/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandlerConfig.java b/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandlerConfig.java
index 010c6ca..67bd748 100644
--- a/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandlerConfig.java
+++ b/src/main/java/org/apache/sling/auth/form/impl/FormAuthenticationHandlerConfig.java
@@ -41,49 +41,49 @@ public @interface FormAuthenticationHandlerConfig {
public static final String DEFAULT_FORM_TOKEN_FILE = "cookie-tokens.bin";
@AttributeDefinition(name = "%authName.name", description = "%authName.description")
- String form_auth_name() default DEFAULT_AUTH_CREDENTIALS_NAME;
+ String form_auth_name() default DEFAULT_AUTH_CREDENTIALS_NAME; // NOSONAR
@AttributeDefinition(options = { @Option(label = "Cookie", value = DEFAULT_AUTH_FORM_STORAGE),
@Option(label = "Session Attribute", value = AUTH_STORAGE_SESSION_ATTRIBUTE) }, name = "%authStorage.name", description = "%authStorage.description")
- String form_auth_storage() default DEFAULT_AUTH_FORM_STORAGE;
+ String form_auth_storage() default DEFAULT_AUTH_FORM_STORAGE; // NOSONAR
@AttributeDefinition(type = AttributeType.INTEGER, name = "%authTimeout.name", description = "%authTimeout.description")
- int form_auth_timeout() default DEFAULT_AUTH_TIMEOUT;
+ int form_auth_timeout() default DEFAULT_AUTH_TIMEOUT; // NOSONAR
@AttributeDefinition(name = "%credentialsName.name", description = "%credentialsName.description")
- String form_credentials_name() default DEFAULT_AUTH_CREDENTIALS_NAME;
+ String form_credentials_name() default DEFAULT_AUTH_CREDENTIALS_NAME; // NOSONAR
@AttributeDefinition(name = "%defaultCookieDomain.name", description = "%defaultCookieDomain.description")
- String form_default_cookie_domain() default "";
+ String form_default_cookie_domain() default ""; // NOSONAR
@AttributeDefinition(name = "%loginForm.name", description = "%loginForm.description")
- String form_login_form() default AuthenticationFormServlet.SERVLET_PATH;
+ String form_login_form() default AuthenticationFormServlet.SERVLET_PATH; // NOSONAR
@AttributeDefinition(name = "%onexpireLogin.name", description = "%onexpireLogin.description")
- boolean form_onexpire_login() default false;
+ boolean form_onexpire_login() default false; // NOSONAR
@AttributeDefinition(type = AttributeType.BOOLEAN, name = "%tokenFileFastseed.name", description = "%tokenFileFastseed.description")
- boolean form_token_fastseed() default false;
+ boolean form_token_fastseed() default false; // NOSONAR
@AttributeDefinition(name = "%tokenFile.name", description = "%tokenFile.description")
- String form_token_file() default DEFAULT_FORM_TOKEN_FILE;
+ String form_token_file() default DEFAULT_FORM_TOKEN_FILE; // NOSONAR
@AttributeDefinition(options = { @Option(label = "Optional", value = "optional"),
@Option(label = "Required", value = "required"), @Option(label = "Requisite", value = "requisite"),
@Option(label = "Sufficient", value = DEEFAULT_JAAS_CONTROL_FLAG) }, name = "%jaasControlFlag.name", description = "%jaasControlFlag.description")
- String jaas_controlFlag() default DEEFAULT_JAAS_CONTROL_FLAG;
+ String jaas_controlFlag() default DEEFAULT_JAAS_CONTROL_FLAG; // NOSONAR
@AttributeDefinition(type = AttributeType.INTEGER, name = "%jaasRanking.name", description = "%jaasRanking.description")
- int jaas_ranking() default DEFAULT_JAAS_RANKING;
+ int jaas_ranking() default DEFAULT_JAAS_RANKING; // NOSONAR
@AttributeDefinition(name = "%jaasRealm.name", description = "%jaasRealm.description")
- String jaas_realmName() default DEFAULT_JAAS_REALM_NAME;
+ String jaas_realmName() default DEFAULT_JAAS_REALM_NAME; // NOSONAR
@AttributeDefinition(cardinality = Integer.MAX_VALUE, name = "%path.name", description = "%path.description")
String[] path() default { "/" };
@AttributeDefinition(type = AttributeType.INTEGER, name = "%service.ranking.name", description = "%service.ranking.description")
- int service_ranking() default 0;
+ int service_ranking() default 0; // NOSONAR
@AttributeDefinition(type = AttributeType.BOOLEAN, name = "%useInclude.name", description = "%useInclude.description")
boolean useInclude() default false;
diff --git a/src/main/java/org/apache/sling/auth/form/impl/FormLoginModulePlugin.java b/src/main/java/org/apache/sling/auth/form/impl/FormLoginModulePlugin.java
index 786750b..fc802ed 100644
--- a/src/main/java/org/apache/sling/auth/form/impl/FormLoginModulePlugin.java
+++ b/src/main/java/org/apache/sling/auth/form/impl/FormLoginModulePlugin.java
@@ -66,18 +66,18 @@ final class FormLoginModulePlugin implements LoginModulePlugin {
* the {@link FormAuthenticationHandler} to unregister the service
* on shutdown.
*/
- static ServiceRegistration register(
+ static ServiceRegistration<LoginModulePlugin> register(
final FormAuthenticationHandler authHandler,
final BundleContext bundleContext) {
FormLoginModulePlugin plugin = new FormLoginModulePlugin(authHandler);
- Hashtable<String, Object> properties = new Hashtable<String, Object>();
+ Hashtable<String, Object> properties = new Hashtable<>(); // NOSONAR
properties.put(Constants.SERVICE_DESCRIPTION,
"LoginModulePlugin Support for FormAuthenticationHandler");
properties.put(Constants.SERVICE_VENDOR,
bundleContext.getBundle().getHeaders().get(Constants.BUNDLE_VENDOR));
- return bundleContext.registerService(LoginModulePlugin.class.getName(),
+ return bundleContext.registerService(LoginModulePlugin.class,
plugin, properties);
}
@@ -107,9 +107,9 @@ final class FormLoginModulePlugin implements LoginModulePlugin {
/**
* This implementation does nothing.
*/
- @SuppressWarnings("unchecked")
public void doInit(CallbackHandler callbackHandler, Session session,
- Map options) {
+ @SuppressWarnings("rawtypes") Map options) {
+ // no-op
}
/**
@@ -123,8 +123,8 @@ final class FormLoginModulePlugin implements LoginModulePlugin {
/**
* This implementation does nothing.
*/
- @SuppressWarnings("unchecked")
- public void addPrincipals(@SuppressWarnings("unused") Set principals) {
+ public void addPrincipals(@SuppressWarnings({ "unused", "rawtypes" }) Set principals) {
+ throw new UnsupportedOperationException();
}
/**
@@ -136,11 +136,7 @@ final class FormLoginModulePlugin implements LoginModulePlugin {
*/
public AuthenticationPlugin getAuthentication(Principal principal,
Credentials creds) {
- return new AuthenticationPlugin() {
- public boolean authenticate(Credentials credentials) {
- return authHandler.isValid(credentials);
- }
- };
+ return authHandler::isValid;
}
/**
diff --git a/src/main/java/org/apache/sling/auth/form/impl/TokenStore.java b/src/main/java/org/apache/sling/auth/form/impl/TokenStore.java
index 8ff67fd..4e9ff42 100644
--- a/src/main/java/org/apache/sling/auth/form/impl/TokenStore.java
+++ b/src/main/java/org/apache/sling/auth/form/impl/TokenStore.java
@@ -24,15 +24,18 @@ import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
+import java.nio.charset.StandardCharsets;
import java.security.InvalidKeyException;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
+import java.util.concurrent.atomic.AtomicReferenceArray;
import javax.crypto.Mac;
import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;
import org.apache.commons.lang3.StringUtils;
+import org.jetbrains.annotations.NotNull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -42,6 +45,7 @@ import org.slf4j.LoggerFactory;
* validate and persist secure tokens.
*/
class TokenStore {
+ private static final String[] EMPTY_ARRAY = new String[0];
/**
* Array of hex characters used by {@link #byteToHex(byte[])} to convert a
@@ -88,7 +92,7 @@ class TokenStore {
/**
* A ring of tokens used to encrypt.
*/
- private volatile SecretKey[] currentTokens;
+ private AtomicReferenceArray<SecretKey> currentTokens;
/**
* A secure random used for generating new tokens.
@@ -111,8 +115,7 @@ class TokenStore {
*/
TokenStore(final File tokenFile, final long sessionTimeout,
final boolean fastSeed) throws NoSuchAlgorithmException,
- InvalidKeyException, IllegalStateException,
- UnsupportedEncodingException {
+ InvalidKeyException, IllegalStateException {
if (tokenFile == null) {
throw new NullPointerException("tokenfile");
@@ -142,7 +145,7 @@ class TokenStore {
final SecretKey secretKey = new SecretKeySpec(b, HMAC_SHA1);
final Mac m = Mac.getInstance(HMAC_SHA1);
m.init(secretKey);
- m.update(UTF_8.getBytes(UTF_8));
+ m.update(UTF_8.getBytes(StandardCharsets.UTF_8));
m.doFinal();
}
@@ -156,23 +159,22 @@ class TokenStore {
* @throws InvalidKeyException
*/
String encode(final long expires, final String userId)
- throws IllegalStateException, UnsupportedEncodingException,
+ throws IllegalStateException,
NoSuchAlgorithmException, InvalidKeyException {
int token = getActiveToken();
- SecretKey key = currentTokens[token];
+ SecretKey key = currentTokens.get(token);
return encode(expires, userId, token, key);
}
private String encode(final long expires, final String userId,
final int token, final SecretKey key) throws IllegalStateException,
- UnsupportedEncodingException, NoSuchAlgorithmException,
- InvalidKeyException {
+ NoSuchAlgorithmException, InvalidKeyException {
String cookiePayload = String.valueOf(token) + String.valueOf(expires)
+ "@" + userId;
Mac m = Mac.getInstance(HMAC_SHA1);
m.init(key);
- m.update(cookiePayload.getBytes(UTF_8));
+ m.update(cookiePayload.getBytes(StandardCharsets.UTF_8));
String cookieValue = byteToHex(m.doFinal());
return cookieValue + "@" + cookiePayload;
}
@@ -187,12 +189,12 @@ class TokenStore {
* <code>null</code> or if the string does not contain (at least)
* three '@' separated parts.
*/
- static String[] split(final String authData) {
+ static @NotNull String[] split(final String authData) {
String[] parts = StringUtils.split(authData, "@", 3);
if (parts != null && parts.length == 3) {
return parts;
}
- return null;
+ return EMPTY_ARRAY;
}
/**
@@ -211,17 +213,17 @@ class TokenStore {
*/
boolean isValid(String value) {
String[] parts = split(value);
- if (parts != null) {
+ if (parts.length == 3) {
// single digit token number
int tokenNumber = parts[1].charAt(0) - '0';
- if (tokenNumber >= 0 && tokenNumber < currentTokens.length) {
+ if (tokenNumber >= 0 && tokenNumber < currentTokens.length()) {
long cookieTime = Long.parseLong(parts[1].substring(1));
if (System.currentTimeMillis() < cookieTime) {
try {
- SecretKey secretKey = currentTokens[tokenNumber];
+ SecretKey secretKey = currentTokens.get(tokenNumber);
if ( secretKey == null ) {
log.error("AuthNCookie value '{}' points to an unknown token number", value);
return false;
@@ -229,15 +231,8 @@ class TokenStore {
String hmac = encode(cookieTime, parts[2], tokenNumber,
secretKey);
return value.equals(hmac);
- } catch (ArrayIndexOutOfBoundsException e) {
- log.error(e.getMessage(), e);
- } catch (InvalidKeyException e) {
- log.error(e.getMessage(), e);
- } catch (IllegalStateException e) {
- log.error(e.getMessage(), e);
- } catch (UnsupportedEncodingException e) {
- log.error(e.getMessage(), e);
- } catch (NoSuchAlgorithmException e) {
+ } catch (ArrayIndexOutOfBoundsException | InvalidKeyException | IllegalStateException |
+ NoSuchAlgorithmException e) {
log.error(e.getMessage(), e);
}
@@ -250,7 +245,7 @@ class TokenStore {
} else {
log.error(
- "AuthNCookie value '{}' is invalid: refers to an invalid token number",
+ "AuthNCookie value '{}' is invalid: refers to an invalid token number {}",
value, tokenNumber);
}
@@ -269,20 +264,20 @@ class TokenStore {
*/
private synchronized int getActiveToken() {
if (System.currentTimeMillis() > nextUpdate
- || currentTokens[currentToken] == null) {
+ || currentTokens.get(currentToken) == null) {
// cycle so that during a typical ttl the tokens get completely
// refreshed.
nextUpdate = System.currentTimeMillis() + ttl
- / (currentTokens.length - 1);
+ / (currentTokens.length() - 1);
byte[] b = new byte[20];
random.nextBytes(b);
SecretKey newToken = new SecretKeySpec(b, HMAC_SHA1);
int nextToken = currentToken + 1;
- if (nextToken == currentTokens.length) {
+ if (nextToken == currentTokens.length()) {
nextToken = 0;
}
- currentTokens[nextToken] = newToken;
+ currentTokens.set(nextToken, newToken);
currentToken = nextToken;
saveTokens();
}
@@ -293,41 +288,29 @@ class TokenStore {
* Stores the current set of tokens to the token file
*/
private void saveTokens() {
- FileOutputStream fout = null;
- DataOutputStream keyOutputStream = null;
- try {
File parent = tokenFile.getAbsoluteFile().getParentFile();
log.info("Token File {} parent {} ", tokenFile, parent);
if (!parent.exists()) {
parent.mkdirs();
}
- fout = new FileOutputStream(tmpTokenFile);
- keyOutputStream = new DataOutputStream(fout);
+ try (DataOutputStream keyOutputStream = new DataOutputStream(new FileOutputStream(tmpTokenFile))) {
keyOutputStream.writeInt(currentToken);
keyOutputStream.writeLong(nextUpdate);
- for (int i = 0; i < currentTokens.length; i++) {
- if (currentTokens[i] == null) {
+ for (int i = 0; i < currentTokens.length(); i++) {
+ if (currentTokens.get(i) == null) {
keyOutputStream.writeInt(0);
} else {
keyOutputStream.writeInt(1);
- byte[] b = currentTokens[i].getEncoded();
+ byte[] b = currentTokens.get(i).getEncoded();
keyOutputStream.writeInt(b.length);
keyOutputStream.write(b);
}
}
- keyOutputStream.close();
- tmpTokenFile.renameTo(tokenFile);
} catch (IOException e) {
- log.error("Failed to save cookie keys " + e.getMessage());
- } finally {
- try {
- keyOutputStream.close();
- } catch (Exception e) {
- }
- try {
- fout.close();
- } catch (Exception e) {
+ log.error("Failed to save cookie keys {}", e.getMessage());
}
+ if (!tmpTokenFile.renameTo(tokenFile)) {
+ log.error("Failed to rename the temporary token file");
}
}
@@ -339,23 +322,24 @@ class TokenStore {
*/
private void loadTokens() {
if (tokenFile.isFile() && tokenFile.canRead()) {
- FileInputStream fin = null;
- DataInputStream keyInputStream = null;
- try {
- fin = new FileInputStream(tokenFile);
- keyInputStream = new DataInputStream(fin);
+ try (DataInputStream keyInputStream = new DataInputStream(new FileInputStream(tokenFile))) {
int newCurrentToken = keyInputStream.readInt();
long newNextUpdate = keyInputStream.readLong();
- SecretKey[] newKeys = new SecretKey[TOKEN_BUFFER_SIZE];
- for (int i = 0; i < newKeys.length; i++) {
+ AtomicReferenceArray<SecretKey> newKeys = new AtomicReferenceArray<>(TOKEN_BUFFER_SIZE);
+ for (int i = 0; i < newKeys.length(); i++) {
int isNull = keyInputStream.readInt();
if (isNull == 1) {
int l = keyInputStream.readInt();
byte[] b = new byte[l];
- keyInputStream.read(b);
- newKeys[i] = new SecretKeySpec(b, HMAC_SHA1);
+ int offset = 0;
+ int bytesRead = -1;
+ do {
+ bytesRead = keyInputStream.read(b, offset, b.length - offset);
+ offset += bytesRead;
+ } while (bytesRead != -1 && offset < b.length);
+ newKeys.set(i, new SecretKeySpec(b, HMAC_SHA1));
} else {
- newKeys[i] = null;
+ newKeys.set(i, null);
}
}
@@ -366,27 +350,15 @@ class TokenStore {
} catch (IOException e) {
- log.error("Failed to load cookie keys " + e.getMessage());
+ log.error("Failed to load cookie keys {}", e.getMessage());
- } finally {
- if (keyInputStream != null) {
- try {
- keyInputStream.close();
- } catch (IOException e) {
- }
- } else if (fin != null) {
- try {
- fin.close();
- } catch (IOException e) {
- }
- }
}
}
// if there was a failure to read the current tokens, create new ones
if (currentTokens == null) {
- currentTokens = new SecretKey[TOKEN_BUFFER_SIZE];
+ currentTokens = new AtomicReferenceArray<>(TOKEN_BUFFER_SIZE);
nextUpdate = System.currentTimeMillis();
currentToken = 0;
}
diff --git a/src/main/java/org/apache/sling/auth/form/impl/jaas/FormCredentials.java b/src/main/java/org/apache/sling/auth/form/impl/jaas/FormCredentials.java
index 6f127fc..45aa7aa 100644
--- a/src/main/java/org/apache/sling/auth/form/impl/jaas/FormCredentials.java
+++ b/src/main/java/org/apache/sling/auth/form/impl/jaas/FormCredentials.java
@@ -22,6 +22,7 @@ package org.apache.sling.auth.form.impl.jaas;
import javax.jcr.Credentials;
public class FormCredentials implements Credentials {
+ private static final long serialVersionUID = -1813012094053361438L;
private final String userId;
private final String authData;
diff --git a/src/main/java/org/apache/sling/auth/form/impl/jaas/FormLoginModule.java b/src/main/java/org/apache/sling/auth/form/impl/jaas/FormLoginModule.java
index e98ddf1..686f0c4 100644
--- a/src/main/java/org/apache/sling/auth/form/impl/jaas/FormLoginModule.java
+++ b/src/main/java/org/apache/sling/auth/form/impl/jaas/FormLoginModule.java
@@ -38,6 +38,7 @@ final class FormLoginModule extends AbstractLoginModule {
/**
* The set of supported credentials. required by the {@link org.apache.jackrabbit.oak.spi.security.authentication.AbstractLoginModule}
*/
+ @SuppressWarnings("rawtypes")
private static final Set<Class> SUPPORTED_CREDENTIALS = Collections.<Class>singleton(FormCredentials.class);
private static final char[] EMPTY_PWD = new char[0];
@@ -46,6 +47,7 @@ final class FormLoginModule extends AbstractLoginModule {
*/
private String userId;
+ @SuppressWarnings("rawtypes")
@Override
protected Set<Class> getSupportedCredentials() {
return SUPPORTED_CREDENTIALS;
@@ -63,6 +65,7 @@ final class FormLoginModule extends AbstractLoginModule {
@SuppressWarnings("unchecked")
public boolean login() throws LoginException {
+ boolean succeeded = false;
Credentials credentials = getCredentials();
if (credentials instanceof FormCredentials) {
FormCredentials cred = (FormCredentials) credentials;
@@ -70,10 +73,8 @@ final class FormLoginModule extends AbstractLoginModule {
if (!authHandler.isValid(cred)){
log.debug("Invalid credentials");
- return false;
- }
- if (userId == null) {
+ } else if (userId == null) {
log.debug("Could not extract userId/credentials");
} else {
// we just set the login name and rely on the following login modules to populate the subject
@@ -83,7 +84,7 @@ final class FormLoginModule extends AbstractLoginModule {
log.debug("login succeeded with trusted user: {}", userId);
}
}
- return false;
+ return succeeded;
}
public boolean commit() throws LoginException {
diff --git a/src/main/java/org/apache/sling/auth/form/impl/jaas/JaasHelper.java b/src/main/java/org/apache/sling/auth/form/impl/jaas/JaasHelper.java
index 9b619c1..01acd09 100644
--- a/src/main/java/org/apache/sling/auth/form/impl/jaas/JaasHelper.java
+++ b/src/main/java/org/apache/sling/auth/form/impl/jaas/JaasHelper.java
@@ -82,7 +82,7 @@ public class JaasHelper {
private ServiceRegistration<?> registerLoginModuleFactory(BundleContext ctx, FormAuthenticationHandlerConfig config) {
ServiceRegistration<?> reg = null;
try {
- Dictionary<String, Object> props = new Hashtable<String,Object>();
+ Dictionary<String, Object> props = new Hashtable<>(); // NOSONARs
final String desc = "LoginModule Support for FormAuthenticationHandler";
props.put(Constants.SERVICE_DESCRIPTION, desc);
props.put(Constants.SERVICE_VENDOR, ctx.getBundle().getHeaders().get(Constants.BUNDLE_VENDOR));
diff --git a/src/test/java/org/apache/sling/auth/form/FormReasonTest.java b/src/test/java/org/apache/sling/auth/form/FormReasonTest.java
index d756179..cfe9c9f 100644
--- a/src/test/java/org/apache/sling/auth/form/FormReasonTest.java
+++ b/src/test/java/org/apache/sling/auth/form/FormReasonTest.java
@@ -25,17 +25,17 @@ import org.junit.Test;
public class FormReasonTest {
- @Test public void test_TIMEOUT() {
+ @Test public void testTimeout() {
assertEquals(FormReason.TIMEOUT,
FormReason.valueOf(FormReason.TIMEOUT.name()));
}
- @Test public void test_INVALID_CREDENTIALS() {
+ @Test public void testInvalidCredentials() {
assertEquals(FormReason.INVALID_CREDENTIALS,
FormReason.valueOf(FormReason.INVALID_CREDENTIALS.name()));
}
- @Test public void test_INVALID() {
+ @Test public void testInvalid() {
try {
FormReason.valueOf("INVALID");
fail("unexpected result getting value of an invalid constant");
diff --git a/src/test/java/org/apache/sling/auth/form/impl/FormAuthenticationHandlerTest.java b/src/test/java/org/apache/sling/auth/form/impl/FormAuthenticationHandlerTest.java
index 5858d85..161c460 100644
--- a/src/test/java/org/apache/sling/auth/form/impl/FormAuthenticationHandlerTest.java
+++ b/src/test/java/org/apache/sling/auth/form/impl/FormAuthenticationHandlerTest.java
@@ -55,7 +55,7 @@ import org.powermock.modules.junit4.PowerMockRunner;
@PowerMockIgnore("jdk.internal.reflect.*")
public class FormAuthenticationHandlerTest {
- @Test public void test_getTokenFile() {
+ @Test public void testGetTokenFile() {
final File root = new File("bundle999").getAbsoluteFile();
final SlingHomeAction slingHome = new SlingHomeAction();
slingHome.setSlingHome(new File("sling").getAbsolutePath());
@@ -107,7 +107,7 @@ public class FormAuthenticationHandlerTest {
assertEquals(absFile, absFile0);
}
- @Test public void test_getUserid() {
+ @Test public void testGetUserid() {
final FormAuthenticationHandler handler = new FormAuthenticationHandler();
assertEquals(null, handler.getUserId(null));
assertEquals(null, handler.getUserId(""));
@@ -147,7 +147,7 @@ public class FormAuthenticationHandlerTest {
// Mocks the HttpServletRequest and HttpServletResponse object
expect(request.getMethod()).andReturn("POST");
expect(request.getRequestURI()).andReturn("http://blah/blah/j_security_check");
- String contextPath = "/blah";
+ String contextPath = "/blah"; // NOSONAR
expect(request.getContextPath()).andReturn(contextPath).anyTimes();
expect(response.isCommitted()).andReturn(false);
diff --git a/src/test/java/org/apache/sling/auth/form/impl/TokenStoreTest.java b/src/test/java/org/apache/sling/auth/form/impl/TokenStoreTest.java
index 89526d4..fad6038 100644
--- a/src/test/java/org/apache/sling/auth/form/impl/TokenStoreTest.java
+++ b/src/test/java/org/apache/sling/auth/form/impl/TokenStoreTest.java
@@ -2,12 +2,18 @@ package org.apache.sling.auth.form.impl;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;
import java.io.File;
+import java.io.IOException;
import java.math.BigInteger;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.time.Duration;
import java.util.UUID;
+import org.awaitility.Awaitility;
import org.junit.Before;
import org.junit.Test;
@@ -33,10 +39,10 @@ import org.junit.Test;
/* TokenStore tests, incomplete for now */
public class TokenStoreTest {
private TokenStore store;
- private static final long sessionTimeoutMsec = 60 * 1000L;
- private static final long defaultExpirationTimeMsec = System.currentTimeMillis() + sessionTimeoutMsec / 2;
- private static final boolean defaultFastSeed = false;
- private static final String userId = "user_" + UUID.randomUUID();
+ private static final long SESSION_TIMEOUT_MSEC = 60 * 1000L;
+ private static final long DEFAULT_EXPIRATION_TIME_MSEC = System.currentTimeMillis() + SESSION_TIMEOUT_MSEC / 2;
+ private static final boolean DEFAULT_FAST_SEED = false;
+ private static final String USER_ID = "user_" + UUID.randomUUID();
private String encodedToken;
private File tokenFile;
private int additionalFileIndex;
@@ -46,19 +52,19 @@ import org.junit.Test;
}
@Before
- public void setup() throws Exception {
+ public void setup() throws IOException, InvalidKeyException, NoSuchAlgorithmException, IllegalStateException {
tokenFile = File.createTempFile(getClass().getName(), "tokenstore");
- store = new TokenStore(tokenFile, sessionTimeoutMsec, defaultFastSeed);
- encodedToken = store.encode(defaultExpirationTimeMsec, userId);
+ store = new TokenStore(tokenFile, SESSION_TIMEOUT_MSEC, DEFAULT_FAST_SEED);
+ encodedToken = store.encode(DEFAULT_EXPIRATION_TIME_MSEC, USER_ID);
}
@Test
- public void validTokenTest() throws Exception {
+ public void validTokenTest() {
assertTrue(store.isValid(encodedToken));
}
@Test
- public void invalidTokensTest() throws Exception {
+ public void invalidTokensTest() {
final String [] invalid = {
"1@21@3",
"nothing",
@@ -70,28 +76,32 @@ import org.junit.Test;
}
@Test
- public void expiredTokenTest() throws Exception {
- final String expired = store.encode(1, userId);
- Thread.sleep(50);
+ public void expiredTokenTest() throws InvalidKeyException, IllegalStateException, NoSuchAlgorithmException {
+ final String expired = store.encode(1, USER_ID);
+ Awaitility.await("expiredToken")
+ .atMost(Duration.ofSeconds(5))
+ .pollDelay(Duration.ofMillis(50))
+ .pollInterval(Duration.ofMillis(50))
+ .until(() -> !store.isValid(expired));
assertFalse(store.isValid(expired));
}
@Test
- public void loadTokenFileTest() throws Exception {
- final TokenStore newStore = new TokenStore(tokenFile, sessionTimeoutMsec, defaultFastSeed);
+ public void loadTokenFileTest() throws InvalidKeyException, NoSuchAlgorithmException, IllegalStateException {
+ final TokenStore newStore = new TokenStore(tokenFile, SESSION_TIMEOUT_MSEC, DEFAULT_FAST_SEED);
assertTrue(newStore.isValid(encodedToken));
- final TokenStore emptyStore = new TokenStore(additionalTokenFile(), sessionTimeoutMsec, defaultFastSeed);
+ final TokenStore emptyStore = new TokenStore(additionalTokenFile(), SESSION_TIMEOUT_MSEC, DEFAULT_FAST_SEED);
assertFalse(emptyStore.isValid(encodedToken));
}
@Test
- public void encodingPartsTest() throws Exception {
+ public void encodingPartsTest() throws InvalidKeyException, NoSuchAlgorithmException, IllegalStateException {
// Test with both a normal and "fast seed" store
final TokenStore [] testStores = {
- new TokenStore(additionalTokenFile(), sessionTimeoutMsec, true),
- new TokenStore(additionalTokenFile(), sessionTimeoutMsec, false)
+ new TokenStore(additionalTokenFile(), SESSION_TIMEOUT_MSEC, true),
+ new TokenStore(additionalTokenFile(), SESSION_TIMEOUT_MSEC, false)
};
for(TokenStore testStore : testStores) {
@@ -101,7 +111,7 @@ import org.junit.Test;
final String [] parts = TokenStore.split(testStore.encode(123, uniqueUserId));
// First a unique large hex number
- assertFalse(parts[0].equals(lastHexNumber));
+ assertNotEquals(parts[0], lastHexNumber);
lastHexNumber = parts[0];
new BigInteger(lastHexNumber, 16);
assertTrue(lastHexNumber.length() > 20);
@@ -116,7 +126,7 @@ import org.junit.Test;
}
@Test(expected = NullPointerException.class)
- public void nullTokenFileTest() throws Exception {
- new TokenStore(null, sessionTimeoutMsec, defaultFastSeed);
+ public void nullTokenFileTest() throws InvalidKeyException, NoSuchAlgorithmException, IllegalStateException {
+ new TokenStore(null, SESSION_TIMEOUT_MSEC, DEFAULT_FAST_SEED);
}
}
diff --git a/src/test/java/org/apache/sling/auth/form/it/AuthFormTestSupport.java b/src/test/java/org/apache/sling/auth/form/it/AuthFormTestSupport.java
index a41b7c2..519d61d 100644
--- a/src/test/java/org/apache/sling/auth/form/it/AuthFormTestSupport.java
+++ b/src/test/java/org/apache/sling/auth/form/it/AuthFormTestSupport.java
@@ -28,6 +28,7 @@ import static org.ops4j.pax.exam.CoreOptions.mavenBundle;
import static org.ops4j.pax.exam.CoreOptions.options;
import static org.ops4j.pax.exam.CoreOptions.streamBundle;
import static org.ops4j.pax.exam.CoreOptions.vmOption;
+import static org.ops4j.pax.exam.CoreOptions.when;
import static org.ops4j.pax.exam.cm.ConfigurationAdminOptions.factoryConfiguration;
import static org.ops4j.pax.tinybundles.core.TinyBundles.withBnd;
@@ -73,10 +74,23 @@ public abstract class AuthFormTestSupport extends TestSupport {
@Configuration
public Option[] configuration() throws IOException {
+ final String vmOpt = System.getProperty("pax.vm.options");
+ VMOption vmOption = null;
+ if (vmOpt != null && !vmOpt.isEmpty()) {
+ vmOption = new VMOption(vmOpt);
+ }
+
+ final String jacocoOpt = System.getProperty("jacoco.command");
+ VMOption jacocoCommand = null;
+ if (jacocoOpt != null && !jacocoOpt.isEmpty()) {
+ jacocoCommand = new VMOption(jacocoOpt);
+ }
+
return options(
composite(
super.baseConfiguration(),
- vmOption(System.getProperty("pax.vm.options")),
+ when(vmOption != null).useOptions(vmOption),
+ when(jacocoCommand != null).useOptions(jacocoCommand),
optionalRemoteDebug(),
slingQuickstart(),
testBundle("bundle.filename"),
@@ -97,7 +111,7 @@ public abstract class AuthFormTestSupport extends TestSupport {
);
}
- protected Option[] additionalOptions() throws IOException {
+ protected Option[] additionalOptions() throws IOException { // NOSONAR
return new Option[]{};
}