[java-identity-provider] branch main updated: IDP-2069 Null Handling
Rod Widdowson
rdw at steadingsoftware.com
Mon Feb 6 15:32:26 UTC 2023
This is an automated email from the git hooks/post-receive script.
rdw pushed a commit to branch main
in repository java-identity-provider.
View the commit online:
http://git.shibboleth.net/view/?p=java-identity-provider.git;a=commit;h=b5a255383943966852423b4bc6ac5d9eaefb0f6f
The following commit(s) were added to refs/heads/main by this push:
new b5a255383 IDP-2069 Null Handling
b5a255383 is described below
commit b5a255383943966852423b4bc6ac5d9eaefb0f6f
Author: Rod Widdowson <rdw at steadingsoftware.com>
AuthorDate: Mon Feb 6 10:12:21 2023 +0000
IDP-2069 Null Handling
https://shibboleth.atlassian.net/browse/IDP-2069
By way of an experiment make ValidateExternalAuthentication and
DoStorageOperation.
Some drive by clean up of the usual suspects (add assertions and
use of null safe methods)
---
.../shibboleth/idp/plugin/AbstractIdPPlugin.java | 16 ++---
.../idp/plugin/PropertyDrivenIdPPlugin.java | 4 +-
.../idp/admin/impl/DoStorageOperation.java | 69 +++++++++++++++-------
.../authn/principal/PrincipalServiceManager.java | 2 -
.../authn/impl/ValidateExternalAuthentication.java | 54 +++++++++--------
.../idp/authn/spnego/impl/GSSContextAcceptor.java | 3 +-
.../spnego/impl/SPNEGOAuthnControllerTest.java | 2 +-
7 files changed, 90 insertions(+), 60 deletions(-)
diff --git a/idp-admin-api/src/main/java/net/shibboleth/idp/plugin/AbstractIdPPlugin.java b/idp-admin-api/src/main/java/net/shibboleth/idp/plugin/AbstractIdPPlugin.java
index f292d6249..676d9cc64 100644
--- a/idp-admin-api/src/main/java/net/shibboleth/idp/plugin/AbstractIdPPlugin.java
+++ b/idp-admin-api/src/main/java/net/shibboleth/idp/plugin/AbstractIdPPlugin.java
@@ -17,7 +17,6 @@
package net.shibboleth.idp.plugin;
-import java.util.Collections;
import java.util.Set;
import javax.annotation.Nonnegative;
@@ -28,6 +27,7 @@ import net.shibboleth.shared.annotation.constraint.NonnullElements;
import net.shibboleth.shared.annotation.constraint.NotEmpty;
import net.shibboleth.shared.annotation.constraint.NotLive;
import net.shibboleth.shared.annotation.constraint.Unmodifiable;
+import net.shibboleth.shared.collection.CollectionSupport;
/**
* A base class implementing {@link IdPPlugin} that defaults common settings.
@@ -44,13 +44,15 @@ public abstract class AbstractIdPPlugin implements IdPPlugin {
/** Constructor. */
public AbstractIdPPlugin() {
- enableModules = Collections.emptySet();
- disableModules = Collections.emptySet();
+ enableModules = CollectionSupport.emptySet();
+ disableModules = CollectionSupport.emptySet();
}
/** {@inheritDoc} */
@Nonnull @NotEmpty public String getPluginId() {
- return getClass().getPackageName();
+ final String result = getClass().getPackageName();
+ assert result != null;
+ return result;
}
/** {@inheritDoc} */
@@ -60,7 +62,7 @@ public abstract class AbstractIdPPlugin implements IdPPlugin {
/** {@inheritDoc} */
@Nonnull @NonnullElements @Unmodifiable @NotLive public Set<String> getRequiredModules() {
- return Collections.emptySet();
+ return CollectionSupport.emptySet();
}
/** {@inheritDoc} */
@@ -79,7 +81,7 @@ public abstract class AbstractIdPPlugin implements IdPPlugin {
* @param modules modules to enable
*/
protected void setEnableOnInstall(@Nonnull @NonnullElements final Set<IdPModule> modules) {
- enableModules = Set.copyOf(modules);
+ enableModules = CollectionSupport.copyToSet(modules);
}
/** {@inheritDoc} */
@@ -93,7 +95,7 @@ public abstract class AbstractIdPPlugin implements IdPPlugin {
* @param modules modules to disable
*/
protected void setDisableOnRemoval(@Nonnull @NonnullElements final Set<IdPModule> modules) {
- disableModules = Set.copyOf(modules);
+ disableModules = CollectionSupport.copyToSet(modules);
}
/** {@inheritDoc} */
diff --git a/idp-admin-api/src/main/java/net/shibboleth/idp/plugin/PropertyDrivenIdPPlugin.java b/idp-admin-api/src/main/java/net/shibboleth/idp/plugin/PropertyDrivenIdPPlugin.java
index e15e8be77..e0d232ad4 100644
--- a/idp-admin-api/src/main/java/net/shibboleth/idp/plugin/PropertyDrivenIdPPlugin.java
+++ b/idp-admin-api/src/main/java/net/shibboleth/idp/plugin/PropertyDrivenIdPPlugin.java
@@ -167,9 +167,9 @@ public abstract class PropertyDrivenIdPPlugin extends AbstractIdPPlugin {
urls.addAll(getDefaultUpdateURLs());
- updateURLs = List.copyOf(urls);
+ updateURLs = CollectionSupport.copyToList(urls);
- requiredModules = Set.copyOf(StringSupport.normalizeStringCollection(
+ requiredModules = CollectionSupport.copyToSet(StringSupport.normalizeStringCollection(
StringSupport.stringToList(pluginProperties.getProperty(PLUGIN_REQ_MODULES_PROPERTY, ""), ",")));
log.debug("Plugin {} loaded", getPluginId());
diff --git a/idp-admin-impl/src/main/java/net/shibboleth/idp/admin/impl/DoStorageOperation.java b/idp-admin-impl/src/main/java/net/shibboleth/idp/admin/impl/DoStorageOperation.java
index d7e1e515a..dcf8e3bf2 100644
--- a/idp-admin-impl/src/main/java/net/shibboleth/idp/admin/impl/DoStorageOperation.java
+++ b/idp-admin-impl/src/main/java/net/shibboleth/idp/admin/impl/DoStorageOperation.java
@@ -119,11 +119,25 @@ public class DoStorageOperation extends AbstractProfileAction {
throw new ComponentInitializationException("ObjectMapper cannot be null");
}
}
+
+ /** Null safe key getter.
+ * @return Returns the key.
+ */
+ @Nonnull private String getKeyInExecute() {
+ return Constraint.isNotNull(key, "null key not detected in preExecute");
+ }
+
+ /** Null safe context getter.
+ * @return Returns the context.
+ */
+ @Nonnull private String getContextInExecute() {
+ return Constraint.isNotNull(context, "null context not detected in preExecute");
+ }
// Checkstyle: CyclomaticComplexity OFF
/** {@inheritDoc} */
@Override
- protected boolean doPreExecute(final ProfileRequestContext profileRequestContext) {
+ protected boolean doPreExecute(@Nonnull final ProfileRequestContext profileRequestContext) {
if (!super.doPreExecute(profileRequestContext)) {
return false;
@@ -179,11 +193,11 @@ public class DoStorageOperation extends AbstractProfileAction {
// Checkstyle: CyclomaticComplexity ON
/** {@inheritDoc} */
- @Override protected void doExecute(final ProfileRequestContext profileRequestContext) {
+ @Override protected void doExecute(@Nonnull final ProfileRequestContext profileRequestContext) {
try {
- final HttpServletRequest request = getHttpServletRequest();
- final HttpServletResponse response = getHttpServletResponse();
+ @Nonnull final HttpServletRequest request = Constraint.isNotNull(getHttpServletRequest(), "No Servlet request present");
+ @Nonnull final HttpServletResponse response = Constraint.isNotNull(getHttpServletResponse(), "No Servlet response present");
response.setContentType("application/json");
response.setHeader("Cache-Control", "must-revalidate,no-cache,no-store");
@@ -233,17 +247,19 @@ public class DoStorageOperation extends AbstractProfileAction {
private void doRead() throws IOException {
final StorageRecord<?> record;
try {
- record = storageService.read(context, key);
+ @Nonnull final StorageService storageServ = Constraint.isNotNull(storageService, "Null storge service not detected in preExecute");
+ @Nonnull final HttpServletResponse response = Constraint.isNotNull(getHttpServletResponse(), "No Servlet response present");
+ record = storageServ.read(getContextInExecute(), getKeyInExecute());
if (record != null) {
- getHttpServletResponse().setStatus(HttpServletResponse.SC_OK);
+ response.setStatus(HttpServletResponse.SC_OK);
final JsonFactory jsonFactory = new JsonFactory();
try (final JsonGenerator g = jsonFactory.createGenerator(
- getHttpServletResponse().getOutputStream()).useDefaultPrettyPrinter()) {
+ response.getOutputStream()).useDefaultPrettyPrinter()) {
g.setCodec(objectMapper);
g.writeStartObject();
g.writeObjectFieldStart("data");
g.writeStringField("type", "records");
- g.writeStringField("id", storageService.getId() + '/' + context +'/' + key);
+ g.writeStringField("id", storageServ.getId() + '/' + context +'/' + key);
g.writeObjectFieldStart("attributes");
g.writeStringField("value", record.getValue());
g.writeNumberField("version", record.getVersion());
@@ -268,8 +284,12 @@ public class DoStorageOperation extends AbstractProfileAction {
* @throws IOException if an error is raised
*/
private void doCreate() throws IOException {
+ @Nonnull final StorageService storageServ = Constraint.isNotNull(storageService, "Null storge service not detected in preExecute");
+ @Nonnull final HttpServletResponse response = Constraint.isNotNull(getHttpServletResponse(), "No Servlet response present");
+ @Nonnull final HttpServletRequest request = Constraint.isNotNull(getHttpServletRequest(), "No Servlet request present");
+
final JsonFactory jsonFactory = new JsonFactory();
- final JsonParser parser = jsonFactory.createParser(getHttpServletRequest().getInputStream());
+ final JsonParser parser = jsonFactory.createParser(request.getInputStream());
if (parser.nextToken() != JsonToken.START_OBJECT) {
throw new IOException("Expected data to start with an Object");
@@ -292,8 +312,8 @@ public class DoStorageOperation extends AbstractProfileAction {
throw new IOException("Input missing 'val' field");
}
- if (storageService.create(context, key, value, exp)) {
- getHttpServletResponse().setStatus(HttpServletResponse.SC_CREATED);
+ if (storageServ.create(getContextInExecute(), getKeyInExecute(), value, exp)) {
+ response.setStatus(HttpServletResponse.SC_CREATED);
} else {
sendError(HttpServletResponse.SC_CONFLICT, "Duplicate Record",
"Context and key matched an existing record.");
@@ -308,7 +328,11 @@ public class DoStorageOperation extends AbstractProfileAction {
*/
private void doUpdate() throws IOException {
final JsonFactory jsonFactory = new JsonFactory();
- final JsonParser parser = jsonFactory.createParser(getHttpServletRequest().getInputStream());
+ @Nonnull final StorageService storageServ = Constraint.isNotNull(storageService, "Null storge service not detected in preExecute");
+ @Nonnull final HttpServletResponse response = Constraint.isNotNull(getHttpServletResponse(), "No Servlet response present");
+ @Nonnull final HttpServletRequest request = Constraint.isNotNull(getHttpServletRequest(), "No Servlet request present");
+
+ final JsonParser parser = jsonFactory.createParser(request .getInputStream());
if (parser.nextToken() != JsonToken.START_OBJECT) {
throw new IOException("Expected data to start with an Object");
@@ -336,9 +360,9 @@ public class DoStorageOperation extends AbstractProfileAction {
if (version != null) {
try {
- version = storageService.updateWithVersion(version, context, key, value, exp);
+ version = storageServ.updateWithVersion(version, getContextInExecute(), getKeyInExecute(), value, exp);
if (version != null) {
- getHttpServletResponse().setStatus(HttpServletResponse.SC_OK);
+ response.setStatus(HttpServletResponse.SC_OK);
} else {
sendError(HttpServletResponse.SC_NOT_FOUND, "Not Found", "Record to update was absent.");
}
@@ -346,10 +370,10 @@ public class DoStorageOperation extends AbstractProfileAction {
sendError(HttpServletResponse.SC_CONFLICT, "Version Mismatch", "Record version did not match.");
}
} else {
- if (storageService.update(context, key, value, exp)) {
- getHttpServletResponse().setStatus(HttpServletResponse.SC_OK);
- } else if (storageService.create(context, key, value, exp)) {
- getHttpServletResponse().setStatus(HttpServletResponse.SC_CREATED);
+ if (storageServ.update(getContextInExecute(), getKeyInExecute(), value, exp)) {
+ response.setStatus(HttpServletResponse.SC_OK);
+ } else if (storageServ.create(getContextInExecute(), getKeyInExecute(), value, exp)) {
+ response.setStatus(HttpServletResponse.SC_CREATED);
} else {
sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Internal Server Error",
"Record to update was absent and create attempt failed.");
@@ -365,8 +389,11 @@ public class DoStorageOperation extends AbstractProfileAction {
*/
private void doDelete() throws IOException {
try {
- if (storageService.delete(context, key)) {
- getHttpServletResponse().setStatus(HttpServletResponse.SC_NO_CONTENT);
+ @Nonnull final StorageService storageServ = Constraint.isNotNull(storageService, "Null storge service not detected in preExecute");
+ @Nonnull final HttpServletResponse response = Constraint.isNotNull(getHttpServletResponse(), "No Servlet response present");
+
+ if (storageServ.delete(getContextInExecute(), getKeyInExecute())) {
+ response.setStatus(HttpServletResponse.SC_NO_CONTENT);
} else {
sendError(HttpServletResponse.SC_NOT_FOUND,
"Record Not Found", "The specified record was not present or has expired.");
@@ -388,7 +415,7 @@ public class DoStorageOperation extends AbstractProfileAction {
private void sendError(final int status, @Nonnull @NotEmpty final String title,
@Nonnull @NotEmpty final String detail) throws IOException {
- final HttpServletResponse response = getHttpServletResponse();
+ @Nonnull final HttpServletResponse response = Constraint.isNotNull(getHttpServletResponse(), "No Servlet response present");
response.setContentType("application/json");
response.setHeader("Cache-Control", "must-revalidate,no-cache,no-store");
response.setStatus(status);
diff --git a/idp-authn-api/src/main/java/net/shibboleth/idp/authn/principal/PrincipalServiceManager.java b/idp-authn-api/src/main/java/net/shibboleth/idp/authn/principal/PrincipalServiceManager.java
index aef4a17c8..1c3e2bf82 100644
--- a/idp-authn-api/src/main/java/net/shibboleth/idp/authn/principal/PrincipalServiceManager.java
+++ b/idp-authn-api/src/main/java/net/shibboleth/idp/authn/principal/PrincipalServiceManager.java
@@ -20,14 +20,12 @@ package net.shibboleth.idp.authn.principal;
import java.security.Principal;
import java.util.Collection;
import java.util.HashMap;
-import java.util.List;
import java.util.Map;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.slf4j.Logger;
-
import org.springframework.beans.factory.annotation.Autowired;
import net.shibboleth.shared.annotation.ParameterName;
diff --git a/idp-authn-impl/src/main/java/net/shibboleth/idp/authn/impl/ValidateExternalAuthentication.java b/idp-authn-impl/src/main/java/net/shibboleth/idp/authn/impl/ValidateExternalAuthentication.java
index 341ea4ba9..e7626dac8 100644
--- a/idp-authn-impl/src/main/java/net/shibboleth/idp/authn/impl/ValidateExternalAuthentication.java
+++ b/idp-authn-impl/src/main/java/net/shibboleth/idp/authn/impl/ValidateExternalAuthentication.java
@@ -50,6 +50,7 @@ import net.shibboleth.idp.authn.principal.ProxyAuthenticationPrincipal;
import net.shibboleth.idp.authn.principal.UsernamePrincipal;
import net.shibboleth.idp.profile.IdPAuditFields;
import net.shibboleth.shared.annotation.constraint.NotEmpty;
+import net.shibboleth.shared.logic.Constraint;
import net.shibboleth.shared.primitive.LoggerFactory;
import net.shibboleth.shared.service.ReloadableService;
import net.shibboleth.shared.service.ServiceException;
@@ -163,33 +164,33 @@ public class ValidateExternalAuthentication extends AbstractAuditingValidationAc
protected void doExecute(@Nonnull final ProfileRequestContext profileRequestContext,
@Nonnull final AuthenticationContext authenticationContext) {
- if (extContext.getAuthnException() != null) {
- log.info("{} External authentication produced exception", getLogPrefix(), extContext.getAuthnException());
- handleError(profileRequestContext, authenticationContext, extContext.getAuthnException(),
+ @Nonnull ExternalAuthenticationContext localExtContext = Constraint.isNotNull(extContext, "external Authn Context cannot be null");
+ if (localExtContext.getAuthnException() != null) {
+ log.info("{} External authentication produced exception", getLogPrefix(), localExtContext.getAuthnException());
+ handleError(profileRequestContext, authenticationContext, localExtContext.getAuthnException(),
AuthnEventIds.AUTHN_EXCEPTION);
recordFailure(profileRequestContext);
return;
- } else if (extContext.getAuthnError() != null) {
+ } else if (localExtContext.getAuthnError() != null) {
log.info("{} External authentication produced error message: {}", getLogPrefix(),
- extContext.getAuthnError());
- handleError(profileRequestContext, authenticationContext, extContext.getAuthnError(),
+ localExtContext.getAuthnError());
+ handleError(profileRequestContext, authenticationContext, localExtContext.getAuthnError(),
AuthnEventIds.AUTHN_EXCEPTION);
recordFailure(profileRequestContext);
return;
}
-
- if (extContext.getSubject() != null) {
+ if (localExtContext.getSubject() != null) {
log.info("{} External authentication succeeded for Subject", getLogPrefix());
- } else if (extContext.getPrincipal() != null) {
+ } else if (localExtContext.getPrincipal() != null) {
log.info("{} External authentication succeeded for Principal: {}", getLogPrefix(),
- extContext.getPrincipal());
- extContext.setSubject(new Subject(false, Collections.singleton(extContext.getPrincipal()),
+ localExtContext.getPrincipal());
+ localExtContext.setSubject(new Subject(false, Collections.singleton(localExtContext.getPrincipal()),
Collections.emptySet(), Collections.emptySet()));
- } else if (extContext.getPrincipalName() != null) {
+ } else if (localExtContext.getPrincipalName() != null) {
log.info("{} External authentication succeeded for user: {}", getLogPrefix(),
- extContext.getPrincipalName());
- extContext.setSubject(new Subject(false,
- Collections.singleton(new UsernamePrincipal(extContext.getPrincipalName())),
+ localExtContext.getPrincipalName());
+ localExtContext.setSubject(new Subject(false,
+ Collections.singleton(new UsernamePrincipal(localExtContext.getPrincipalName())),
Collections.emptySet(), Collections.emptySet()));
} else {
log.info("{} External authentication failed, no user identity or error information returned",
@@ -199,7 +200,7 @@ public class ValidateExternalAuthentication extends AbstractAuditingValidationAc
return;
}
- if (!checkUsername(extContext.getSubject())) {
+ if (!checkUsername(localExtContext.getSubject())) {
handleError(profileRequestContext, authenticationContext, AuthnEventIds.INVALID_CREDENTIALS,
AuthnEventIds.INVALID_CREDENTIALS);
recordFailure(profileRequestContext);
@@ -208,13 +209,13 @@ public class ValidateExternalAuthentication extends AbstractAuditingValidationAc
recordSuccess(profileRequestContext);
- if (!extContext.getAuthenticatingAuthorities().isEmpty()) {
+ if (!localExtContext.getAuthenticatingAuthorities().isEmpty()) {
final ProxyAuthenticationPrincipal proxied =
- new ProxyAuthenticationPrincipal(extContext.getAuthenticatingAuthorities());
- extContext.getSubject().getPrincipals().add(proxied);
+ new ProxyAuthenticationPrincipal(localExtContext.getAuthenticatingAuthorities());
+ localExtContext.getSubject().getPrincipals().add(proxied);
}
- if (extContext.doNotCache()) {
+ if (localExtContext.doNotCache()) {
log.debug("{} Disabling caching of authentication result", getLogPrefix());
authenticationContext.setResultCacheable(false);
}
@@ -224,10 +225,10 @@ public class ValidateExternalAuthentication extends AbstractAuditingValidationAc
buildAuthenticationResult(profileRequestContext, authenticationContext);
if (authenticationContext.getAuthenticationResult() != null) {
- if (extContext.getAuthnInstant() != null) {
- authenticationContext.getAuthenticationResult().setAuthenticationInstant(extContext.getAuthnInstant());
+ if (localExtContext.getAuthnInstant() != null) {
+ authenticationContext.getAuthenticationResult().setAuthenticationInstant(localExtContext.getAuthnInstant());
}
- if (extContext.isPreviousResult()) {
+ if (localExtContext.isPreviousResult()) {
authenticationContext.getAuthenticationResult().setPreviousResult(true);
}
}
@@ -239,16 +240,17 @@ public class ValidateExternalAuthentication extends AbstractAuditingValidationAc
@Nonnull protected Subject populateSubject(@Nonnull final Subject subject) {
// Override supplied Subject with our own, after transferring over any custom Principals
// and adding any filtered inbound attributes.
- extContext.getSubject().getPrincipals().addAll(subject.getPrincipals());
+ @Nonnull ExternalAuthenticationContext localExtContext = Constraint.isNotNull(extContext, "external Authn Context cannot be null");
+ localExtContext.getSubject().getPrincipals().addAll(subject.getPrincipals());
if (attributeContext != null && !attributeContext.getIdPAttributes().isEmpty()) {
log.debug("{} Adding filtered inbound attributes to Subject", getLogPrefix());
- extContext.getSubject().getPrincipals().addAll(
+ localExtContext.getSubject().getPrincipals().addAll(
attributeContext.getIdPAttributes().values().stream().map(
(IdPAttribute a) -> new IdPAttributePrincipal(a)).collect(Collectors.toList()));
}
- return extContext.getSubject();
+ return localExtContext.getSubject();
}
/**
diff --git a/idp-authn-impl/src/main/java/net/shibboleth/idp/authn/spnego/impl/GSSContextAcceptor.java b/idp-authn-impl/src/main/java/net/shibboleth/idp/authn/spnego/impl/GSSContextAcceptor.java
index 47b4f5dc3..405f422e7 100644
--- a/idp-authn-impl/src/main/java/net/shibboleth/idp/authn/spnego/impl/GSSContextAcceptor.java
+++ b/idp-authn-impl/src/main/java/net/shibboleth/idp/authn/spnego/impl/GSSContextAcceptor.java
@@ -32,6 +32,7 @@ import org.ietf.jgss.GSSManager;
import org.ietf.jgss.Oid;
import org.slf4j.Logger;
+import net.shibboleth.shared.logic.Constraint;
import net.shibboleth.shared.primitive.LoggerFactory;
/**
* Helper class that manages context establishment for the SPNEGO GSS-API mechanism.
@@ -192,7 +193,7 @@ public class GSSContextAcceptor {
}
}
- throw preserved;
+ throw Constraint.isNotNull(preserved, "No realms presented to GssContextAccpetor");
}
/**
diff --git a/idp-authn-impl/src/test/java/net/shibboleth/idp/authn/spnego/impl/SPNEGOAuthnControllerTest.java b/idp-authn-impl/src/test/java/net/shibboleth/idp/authn/spnego/impl/SPNEGOAuthnControllerTest.java
index f52157f9f..058d1ea98 100644
--- a/idp-authn-impl/src/test/java/net/shibboleth/idp/authn/spnego/impl/SPNEGOAuthnControllerTest.java
+++ b/idp-authn-impl/src/test/java/net/shibboleth/idp/authn/spnego/impl/SPNEGOAuthnControllerTest.java
@@ -255,7 +255,7 @@ public class SPNEGOAuthnControllerTest {
final AuthenticationContext authnContext =
((BaseContext) req.getConversationScope().get(ProfileRequestContext.BINDING_KEY)).getSubcontext(AuthenticationContext.class);
final SPNEGOContext spnegoContext = authnContext != null ? authnContext.getSubcontext(SPNEGOContext.class) : null;
- Assert.assertNotNull(spnegoContext);
+ assert spnegoContext != null;
Assert.assertEquals(spnegoContext.getContextAcceptor(), mockGSSContextAcceptor);
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list