From a71abad2c70c8ea995483655baba9c15979ee64e Mon Sep 17 00:00:00 2001 From: Alfusainey Jallow Date: Wed, 20 Apr 2016 12:33:27 +0000 Subject: [PATCH] trivial improvements based on findbugs reports (NPEs in particular) --- .../authentication/external/DefaultSyncHandlerTest.java | 5 +++++ .../authentication/external/ExternalLoginModuleTest.java | 3 +++ .../external/basic/DefaultSyncContextTest.java | 6 ++++-- .../external/impl/ExternalLoginModuleFactoryTest.java | 1 + .../security/authorization/cug/impl/CugPolicyImplTest.java | 2 +- .../apache/jackrabbit/oak/blob/cloud/aws/s3/S3Backend.java | 4 +++- .../oak/security/authorization/AuthorizationContext.java | 2 +- .../authorization/accesscontrol/AccessControlValidator.java | 12 ++++++++++-- .../composite/CompositeAuthorizationConfiguration.java | 2 +- .../java/org/apache/jackrabbit/oak/remote/RemoteValue.java | 2 +- .../jackrabbit/oak/upgrade/cli/parser/MigrationOptions.java | 11 ++++++----- 11 files changed, 36 insertions(+), 14 deletions(-) diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/DefaultSyncHandlerTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/DefaultSyncHandlerTest.java index 488aa2e..6284b90 100644 --- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/DefaultSyncHandlerTest.java +++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/DefaultSyncHandlerTest.java @@ -76,6 +76,7 @@ public class DefaultSyncHandlerTest extends ExternalLoginModuleTestBase { public void testFindMissingIdentity() throws Exception { UserManager userManager = getUserManager(root); SyncHandler mgr = syncManager.getSyncHandler("default"); + assertNotNull(mgr); SyncedIdentity id = mgr.findIdentity(userManager, "foobar"); assertNull("unknown authorizable should not exist", id); } @@ -84,6 +85,7 @@ public class DefaultSyncHandlerTest extends ExternalLoginModuleTestBase { public void testFindLocalIdentity() throws Exception { UserManager userManager = getUserManager(root); SyncHandler mgr = syncManager.getSyncHandler("default"); + assertNotNull(mgr); SyncedIdentity id = mgr.findIdentity(userManager, "admin"); assertNotNull("known authorizable should exist", id); assertNull("local user should not have external ref", id.getExternalIdRef()); @@ -96,6 +98,7 @@ public class DefaultSyncHandlerTest extends ExternalLoginModuleTestBase { UserManager userManager = getUserManager(root); SyncHandler mgr = syncManager.getSyncHandler("default"); + assertNotNull(mgr); SyncedIdentity id = mgr.findIdentity(userManager, userId); assertNotNull("known authorizable should exist", id); assertEquals("external user should have correct external ref.idp", idp.getName(), id.getExternalIdRef().getProviderName()); @@ -109,6 +112,7 @@ public class DefaultSyncHandlerTest extends ExternalLoginModuleTestBase { UserManager userManager = getUserManager(root); SyncHandler mgr = syncManager.getSyncHandler("default"); + assertNotNull(mgr); SyncedIdentity id = mgr.findIdentity(userManager, userId); assertNotNull("known authorizable should exist", id); @@ -131,6 +135,7 @@ public class DefaultSyncHandlerTest extends ExternalLoginModuleTestBase { root.commit(); SyncHandler mgr = syncManager.getSyncHandler("default"); + assertNotNull(mgr); SyncedIdentity id = mgr.findIdentity(userManager, userId); assertNotNull("known authorizable should exist", id); diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/ExternalLoginModuleTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/ExternalLoginModuleTest.java index d0ab693..127df7f 100644 --- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/ExternalLoginModuleTest.java +++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/ExternalLoginModuleTest.java @@ -96,6 +96,7 @@ public class ExternalLoginModuleTest extends ExternalLoginModuleTestBase { Authorizable a = userManager.getAuthorizable(userId); assertNotNull(a); ExternalUser user = idp.getUser(userId); + assertNotNull(user); for (String prop : user.getProperties().keySet()) { assertTrue(a.hasProperty(prop)); } @@ -122,6 +123,7 @@ public class ExternalLoginModuleTest extends ExternalLoginModuleTestBase { Authorizable a = userManager.getAuthorizable(userId); assertNotNull(a); ExternalUser user = idp.getUser(userId); + assertNotNull(user); for (String prop : user.getProperties().keySet()) { assertTrue(a.hasProperty(prop)); } @@ -181,6 +183,7 @@ public class ExternalLoginModuleTest extends ExternalLoginModuleTestBase { // create user upfront in order to test update mode UserManager userManager = getUserManager(root); ExternalUser externalUser = idp.getUser(userId); + assertNotNull(externalUser); Authorizable user = userManager.createUser(externalUser.getId(), null); user.setProperty("rep:externalId", new ValueFactoryImpl(root, NamePathMapper.DEFAULT).createValue(externalUser.getExternalId().getString())); root.commit(); diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContextTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContextTest.java index 31cfed1..0412d0c 100644 --- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContextTest.java +++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/basic/DefaultSyncContextTest.java @@ -41,6 +41,7 @@ import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalId import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentityRef; import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalUser; import org.apache.jackrabbit.oak.spi.security.authentication.external.SyncResult; +import org.apache.jackrabbit.oak.spi.security.authentication.external.SyncedIdentity; import org.apache.jackrabbit.oak.spi.security.authentication.external.TestIdentityProvider; import org.junit.After; import org.junit.Before; @@ -264,8 +265,9 @@ public class DefaultSyncContextTest extends AbstractSecurityTest { SyncResult result = syncCtx.sync(idp.listUsers().next()); assertEquals(SyncResult.Status.ADD, result.getStatus()); - - Authorizable a = getUserManager(root).getAuthorizable(result.getIdentity().getId()); + SyncedIdentity si = result.getIdentity(); + assertNotNull(si); + Authorizable a = getUserManager(root).getAuthorizable(si.getId()); assertTrue(gr.isDeclaredMember(a)); } diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModuleFactoryTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModuleFactoryTest.java index c6da20d..b2b06b7 100644 --- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModuleFactoryTest.java +++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModuleFactoryTest.java @@ -166,6 +166,7 @@ public class ExternalLoginModuleFactoryTest extends AbstractSecurityTest { Authorizable a = userManager.getAuthorizable(userId); assertNotNull(a); ExternalUser user = idp.getUser(userId); + assertNotNull(user); for (String prop : user.getProperties().keySet()) { assertTrue(a.hasProperty(prop)); } diff --git a/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java b/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java index ae81703..5c5d0ca 100644 --- a/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java +++ b/oak-authorization-cug/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/cug/impl/CugPolicyImplTest.java @@ -216,6 +216,6 @@ public class CugPolicyImplTest extends AbstractSecurityTest { @Test(expected = IllegalArgumentException.class) public void testInvalidImportBehavior() { - CugPolicy cug = new CugPolicyImpl(path, NamePathMapper.DEFAULT, principalManager, -1, principals); + new CugPolicyImpl(path, NamePathMapper.DEFAULT, principalManager, -1, principals); } } \ No newline at end of file diff --git a/oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/aws/s3/S3Backend.java b/oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/aws/s3/S3Backend.java index 6a11188..81368ec 100644 --- a/oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/aws/s3/S3Backend.java +++ b/oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/aws/s3/S3Backend.java @@ -340,7 +340,9 @@ public class S3Backend implements SharedS3Backend { } }); } catch (Exception e) { - callback.onAbort(new AsyncTouchResult(identifier)); + if (callback != null) { + callback.onAbort(new AsyncTouchResult(identifier)); + } throw new DataStoreException("Cannot touch the record " + identifier.toString(), e); } finally { diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/AuthorizationContext.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/AuthorizationContext.java index 2ee863e..47c6f32 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/AuthorizationContext.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/AuthorizationContext.java @@ -101,7 +101,7 @@ final class AuthorizationContext implements Context, AccessControlConstants, Per return false; } - private static boolean isNtName(@Nonnull String name) { + private static boolean isNtName(String name) { for (String n : NT_NAMES) { if (n.equals(name)) { return true; diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java index f79e60b..a620832 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java @@ -299,8 +299,16 @@ class AccessControlValidator extends DefaultValidator implements AccessControlCo private final Set restrictions; private Entry(String path, Tree aceTree) { - principalName = aceTree.getProperty(REP_PRINCIPAL_NAME).getValue(Type.STRING); - privilegeBits = privilegeBitsProvider.getBits(aceTree.getProperty(REP_PRIVILEGES).getValue(Type.NAMES)); + PropertyState ps = aceTree.getProperty(REP_PRINCIPAL_NAME); + principalName = (ps != null) ? ps.getValue(Type.STRING) : null; + Iterable privilegeNames; + ps = aceTree.getProperty(REP_PRIVILEGES); + if (ps == null) { + privilegeNames = Collections.emptyList(); + } else { + privilegeNames = ps.getValue(Type.NAMES); + } + privilegeBits = privilegeBitsProvider.getBits(privilegeNames); restrictions = restrictionProvider.readRestrictions(path, aceTree); } diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/composite/CompositeAuthorizationConfiguration.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/composite/CompositeAuthorizationConfiguration.java index ec815e4..f04de0c 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/composite/CompositeAuthorizationConfiguration.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/composite/CompositeAuthorizationConfiguration.java @@ -115,7 +115,7 @@ public class CompositeAuthorizationConfiguration extends CompositeConfiguration< default: List rps = new ArrayList(configurations.size()); for (AuthorizationConfiguration c : configurations) { - if (RestrictionProvider.EMPTY != c) { + if (RestrictionProvider.EMPTY != c.getRestrictionProvider()) { rps.add(c.getRestrictionProvider()); } } diff --git a/oak-remote/src/main/java/org/apache/jackrabbit/oak/remote/RemoteValue.java b/oak-remote/src/main/java/org/apache/jackrabbit/oak/remote/RemoteValue.java index 321408d..14abb18 100644 --- a/oak-remote/src/main/java/org/apache/jackrabbit/oak/remote/RemoteValue.java +++ b/oak-remote/src/main/java/org/apache/jackrabbit/oak/remote/RemoteValue.java @@ -1024,7 +1024,7 @@ public class RemoteValue { * boolean, false otherwise. */ public Boolean asBoolean() { - return null; + return false; } /** diff --git a/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/cli/parser/MigrationOptions.java b/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/cli/parser/MigrationOptions.java index 51c55db..08a45f0 100644 --- a/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/cli/parser/MigrationOptions.java +++ b/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/cli/parser/MigrationOptions.java @@ -28,7 +28,7 @@ public class MigrationOptions { private static final Logger log = LoggerFactory.getLogger(MigrationOptions.class); - private static final DateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd"); + private final DateFormat dateFormat; private final boolean copyBinariesByReference; @@ -51,6 +51,7 @@ public class MigrationOptions { private final boolean earlyShutdown; public MigrationOptions(MigrationCliArguments args) { + dateFormat = new SimpleDateFormat("yyyy-MM-dd"); this.copyBinariesByReference = !args.hasOption(OptionParserFactory.COPY_BINARIES); this.mmap = args.hasOption(OptionParserFactory.MMAP); if (args.hasOption(OptionParserFactory.CACHE_SIZE)) { @@ -133,13 +134,13 @@ public class MigrationOptions { if (copyVersions == null) { log.info("copyVersions parameter set to false"); } else { - log.info("copyVersions parameter set to {}", DATE_FORMAT.format(copyVersions.getTime())); + log.info("copyVersions parameter set to {}", dateFormat.format(copyVersions.getTime())); } if (copyOrphanedVersions == null) { log.info("copyOrphanedVersions parameter set to false"); } else { - log.info("copyOrphanedVersions parameter set to {}", DATE_FORMAT.format(copyOrphanedVersions.getTime())); + log.info("copyOrphanedVersions parameter set to {}", dateFormat.format(copyOrphanedVersions.getTime())); } if (includePaths != null) { @@ -170,7 +171,7 @@ public class MigrationOptions { } } - private static Calendar parseVersionCopyArgument(String string) { + private Calendar parseVersionCopyArgument(String string) { final Calendar calendar; if (Boolean.parseBoolean(string)) { @@ -179,7 +180,7 @@ public class MigrationOptions { } else if (string != null && string.matches("^\\d{4}-\\d{2}-\\d{2}$")) { calendar = Calendar.getInstance(); try { - calendar.setTime(DATE_FORMAT.parse(string)); + calendar.setTime(dateFormat.parse(string)); } catch (ParseException e) { return null; } -- 1.9.1