Index: oak-core/src/main/java/org/apache/jackrabbit/oak/security/internal/SecurityProviderRegistration.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-core/src/main/java/org/apache/jackrabbit/oak/security/internal/SecurityProviderRegistration.java (revision 1856820) +++ oak-core/src/main/java/org/apache/jackrabbit/oak/security/internal/SecurityProviderRegistration.java (date 1554210337000) @@ -17,11 +17,15 @@ package org.apache.jackrabbit.oak.security.internal; import java.io.Closeable; +import java.util.Collection; import java.util.Collections; import java.util.Dictionary; import java.util.Hashtable; import java.util.List; import java.util.Map; +import java.util.SortedMap; +import java.util.TreeMap; + import org.apache.jackrabbit.oak.commons.PropertiesUtil; import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard; import org.apache.jackrabbit.oak.plugins.tree.RootProvider; @@ -33,7 +37,6 @@ import org.apache.jackrabbit.oak.security.user.whiteboard.WhiteboardAuthorizableNodeName; import org.apache.jackrabbit.oak.security.user.whiteboard.WhiteboardUserAuthenticationFactory; import org.apache.jackrabbit.oak.spi.security.CompositeConfiguration; -import org.apache.jackrabbit.oak.spi.security.ConfigurationBase; import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters; import org.apache.jackrabbit.oak.spi.security.SecurityConfiguration; import org.apache.jackrabbit.oak.spi.security.SecurityProvider; @@ -61,6 +64,7 @@ import org.jetbrains.annotations.NotNull; import org.osgi.framework.BundleContext; import org.osgi.framework.Constants; +import org.osgi.framework.ServiceReference; import org.osgi.framework.ServiceRegistration; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; @@ -152,7 +156,7 @@ private final List authorizableNodeNames = newCopyOnWriteArrayList(); private final List authorizableActionProviders = newCopyOnWriteArrayList(); private final List restrictionProviders = newCopyOnWriteArrayList(); - private final List userAuthenticationFactories = newCopyOnWriteArrayList(); + private final SortedMap userAuthenticationFactories = Collections.synchronizedSortedMap(new TreeMap<>()); private RootProvider rootProvider; private TreeProvider treeProvider; @@ -403,19 +407,19 @@ cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC ) - public void bindUserAuthenticationFactory(UserAuthenticationFactory userAuthenticationFactory, Map properties) { + public void bindUserAuthenticationFactory(@NotNull ServiceReference serviceReference, @NotNull UserAuthenticationFactory userAuthenticationFactory) { synchronized (this) { - userAuthenticationFactories.add(userAuthenticationFactory); - addCandidate(properties); + userAuthenticationFactories.put(serviceReference, userAuthenticationFactory); + addCandidate(serviceReference); } maybeRegister(); } - public void unbindUserAuthenticationFactory(UserAuthenticationFactory userAuthenticationFactory, Map properties) { + public void unbindUserAuthenticationFactory(@NotNull ServiceReference serviceReference, @NotNull UserAuthenticationFactory userAuthenticationFactory) { synchronized (this) { - userAuthenticationFactories.remove(userAuthenticationFactory); - removeCandidate(properties); + userAuthenticationFactories.remove(serviceReference); + removeCandidate(serviceReference); } maybeUnregister(); @@ -600,7 +604,10 @@ @Override protected List getServices() { - return newArrayList(userAuthenticationFactories); + Collection values = userAuthenticationFactories.values(); + synchronized (userAuthenticationFactories) { + return newArrayList(values); + } } }; @@ -609,6 +616,16 @@ private void addCandidate(Map properties) { String pidOrName = getServicePidOrComponentName(properties); + if (pidOrName == null) { + return; + } + + preconditions.addCandidate(pidOrName); + } + + private void addCandidate(@NotNull ServiceReference serviceReference) { + String pidOrName = getServicePidOrComponentName(serviceReference); + if (pidOrName == null) { return; } @@ -626,6 +643,16 @@ preconditions.removeCandidate(pidOrName); } + private void removeCandidate(@NotNull ServiceReference serviceReference) { + String pidOrName = getServicePidOrComponentName(serviceReference); + + if (pidOrName == null) { + return; + } + + preconditions.removeCandidate(pidOrName); + } + private static String getServicePidOrComponentName(Map properties) { String servicePid = PropertiesUtil.toString(properties.get(Constants.SERVICE_PID), null); if ( servicePid != null ) { @@ -633,4 +660,12 @@ } return PropertiesUtil.toString(properties.get(OAK_SECURITY_NAME), null); } + + private static String getServicePidOrComponentName(@NotNull ServiceReference serviceReference) { + String servicePid = PropertiesUtil.toString(serviceReference.getProperty(Constants.SERVICE_PID), null); + if ( servicePid != null ) { + return servicePid; + } + return PropertiesUtil.toString(serviceReference.getProperty(OAK_SECURITY_NAME), null); + } } Index: oak-core/src/test/java/org/apache/jackrabbit/oak/security/internal/SecurityProviderRegistrationTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- oak-core/src/test/java/org/apache/jackrabbit/oak/security/internal/SecurityProviderRegistrationTest.java (revision 1856820) +++ oak-core/src/test/java/org/apache/jackrabbit/oak/security/internal/SecurityProviderRegistrationTest.java (date 1554211112000) @@ -18,26 +18,34 @@ import java.lang.annotation.Annotation; import java.lang.reflect.Field; +import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.SortedMap; + import com.google.common.base.Predicates; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import org.apache.jackrabbit.oak.AbstractSecurityTest; import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Root; import org.apache.jackrabbit.oak.api.Tree; import org.apache.jackrabbit.oak.plugins.memory.PropertyStates; import org.apache.jackrabbit.oak.plugins.tree.TreeLocation; import org.apache.jackrabbit.oak.security.authorization.AuthorizationConfigurationImpl; import org.apache.jackrabbit.oak.security.authorization.composite.CompositeAuthorizationConfiguration; +import org.apache.jackrabbit.oak.security.authorization.restriction.RestrictionProviderImpl; import org.apache.jackrabbit.oak.security.authorization.restriction.WhiteboardRestrictionProvider; import org.apache.jackrabbit.oak.security.principal.PrincipalConfigurationImpl; +import org.apache.jackrabbit.oak.security.user.UserAuthenticationFactoryImpl; import org.apache.jackrabbit.oak.spi.security.CompositeConfiguration; import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters; import org.apache.jackrabbit.oak.spi.security.Context; import org.apache.jackrabbit.oak.spi.security.RegistrationConstants; import org.apache.jackrabbit.oak.spi.security.SecurityConfiguration; import org.apache.jackrabbit.oak.spi.security.SecurityProvider; +import org.apache.jackrabbit.oak.spi.security.authentication.Authentication; import org.apache.jackrabbit.oak.spi.security.authentication.AuthenticationConfiguration; import org.apache.jackrabbit.oak.spi.security.authentication.token.CompositeTokenConfiguration; import org.apache.jackrabbit.oak.spi.security.authentication.token.TokenConfiguration; @@ -52,18 +60,23 @@ import org.apache.jackrabbit.oak.spi.security.user.action.AuthorizableActionProvider; import org.apache.sling.testing.mock.osgi.junit.OsgiContext; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.junit.Rule; import org.junit.Test; -import org.mockito.Mockito; import org.osgi.framework.Constants; +import org.osgi.framework.ServiceReference; +import org.osgi.service.component.annotations.Component; +import static org.apache.jackrabbit.oak.spi.security.RegistrationConstants.OAK_SECURITY_NAME; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.osgi.framework.Constants.SERVICE_PID; public class SecurityProviderRegistrationTest extends AbstractSecurityTest { @@ -94,7 +107,7 @@ } private static T mockConfiguration(Class cl) { - T sc = Mockito.mock(cl); + T sc = mock(cl); when(sc.getContext()).thenReturn(new ContextImpl()); when(sc.getParameters()).thenReturn(ConfigurationParameters.EMPTY); return sc; @@ -120,7 +133,7 @@ SecurityProvider service = context.getService(SecurityProvider.class); assertNull(service); - registration.bindAuthorizableNodeName(Mockito.mock(AuthorizableNodeName.class), ImmutableMap.of(Constants.SERVICE_PID, "serviceId")); + registration.bindAuthorizableNodeName(mock(AuthorizableNodeName.class), ImmutableMap.of(SERVICE_PID, "serviceId")); service = context.getService(SecurityProvider.class); assertNotNull(service); @@ -133,13 +146,13 @@ SecurityProvider service = context.getService(SecurityProvider.class); assertNull(service); - RestrictionProvider mockRp = Mockito.mock(RestrictionProvider.class); - registration.bindRestrictionProvider(mockRp, ImmutableMap.of(Constants.SERVICE_PID, "serviceA")); + RestrictionProvider mockRp = mock(RestrictionProvider.class); + registration.bindRestrictionProvider(mockRp, ImmutableMap.of(SERVICE_PID, "serviceA")); service = context.getService(SecurityProvider.class); assertNull(service); - registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(Constants.SERVICE_PID, "serviceB")); + registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(SERVICE_PID, "serviceB")); service = context.getService(SecurityProvider.class); assertNotNull(service); } @@ -169,7 +182,7 @@ public void testModified() { registration.activate(context.bundleContext(), configWithRequiredServiceIds("rpId", "authorizationId")); - registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(Constants.SERVICE_PID, "authorizationId")); + registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(SERVICE_PID, "authorizationId")); assertNull(context.getService(SecurityProvider.class)); @@ -188,9 +201,9 @@ public void testModifiedPreconditionStillSatisfied() { registration.activate(context.bundleContext(), configWithRequiredServiceIds("rpId", "authorizationId")); - RestrictionProvider mockRp = Mockito.mock(RestrictionProvider.class); - registration.bindRestrictionProvider(mockRp, ImmutableMap.of(Constants.SERVICE_PID, "rpId")); - registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(Constants.SERVICE_PID, "authorizationId")); + RestrictionProvider mockRp = mock(RestrictionProvider.class); + registration.bindRestrictionProvider(mockRp, ImmutableMap.of(SERVICE_PID, "rpId")); + registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(SERVICE_PID, "authorizationId")); SecurityProvider service = context.getService(SecurityProvider.class); assertNotNull(service); @@ -204,8 +217,8 @@ @Test public void testDeactivate() throws Exception { registration.activate(context.bundleContext(), configWithRequiredServiceIds("nodeName")); - AuthorizableNodeName mock = Mockito.mock(AuthorizableNodeName.class); - registration.bindAuthorizableNodeName(mock, ImmutableMap.of(Constants.SERVICE_PID, "nodeName")); + AuthorizableNodeName mock = mock(AuthorizableNodeName.class); + registration.bindAuthorizableNodeName(mock, ImmutableMap.of(SERVICE_PID, "nodeName")); SecurityProvider service = context.getService(SecurityProvider.class); assertNotNull(service); @@ -219,8 +232,9 @@ @Test public void testDeactivateWithoutPreconditions() throws Exception { registration.activate(context.bundleContext(), configWithRequiredServiceIds()); - UserAuthenticationFactory mock = Mockito.mock(UserAuthenticationFactory.class); - registration.bindUserAuthenticationFactory(mock, ImmutableMap.of(Constants.SERVICE_PID, "nodeName")); + UserAuthenticationFactory mock = mock(UserAuthenticationFactory.class); + ServiceReference serviceReference = when(mock(ServiceReference.class).getProperty(OAK_SECURITY_NAME)).thenReturn("my.custom.uaf").getMock(); + registration.bindUserAuthenticationFactory(serviceReference, mock); assertNotNull(context.getService(SecurityProvider.class)); @@ -239,8 +253,8 @@ assertFalse(((Preconditions) f.get(registration)).areSatisfied()); - AuthorizableNodeName mock = Mockito.mock(AuthorizableNodeName.class); - registration.bindAuthorizableNodeName(mock, ImmutableMap.of(Constants.SERVICE_PID, "nodeName")); + AuthorizableNodeName mock = mock(AuthorizableNodeName.class); + registration.bindAuthorizableNodeName(mock, ImmutableMap.of(SERVICE_PID, "nodeName")); assertTrue(((Preconditions) f.get(registration)).areSatisfied()); @@ -257,7 +271,7 @@ f.setAccessible(true); TokenConfiguration tc = mockConfiguration(TokenConfiguration.class); - registration.bindTokenConfiguration(tc, ImmutableMap.of(Constants.SERVICE_PID, "otherServiceId")); + registration.bindTokenConfiguration(tc, ImmutableMap.of(SERVICE_PID, "otherServiceId")); Preconditions preconditions = (Preconditions) f.get(registration); assertFalse(preconditions.areSatisfied()); @@ -269,13 +283,13 @@ public void testBindOptionalCandidateAfterRegistration() { registration.activate(context.bundleContext(), configWithRequiredServiceIds("serviceId")); - registration.bindTokenConfiguration(mockConfiguration(TokenConfiguration.class), ImmutableMap.of(Constants.SERVICE_PID, "serviceId")); + registration.bindTokenConfiguration(mockConfiguration(TokenConfiguration.class), ImmutableMap.of(SERVICE_PID, "serviceId")); SecurityProvider service = context.getService(SecurityProvider.class); assertNotNull(service); // binding another (optional configuration) must not result in re-registration of the service - registration.bindPrincipalConfiguration(mockConfiguration(PrincipalConfiguration.class), ImmutableMap.of(Constants.SERVICE_PID, "optionalService")); + registration.bindPrincipalConfiguration(mockConfiguration(PrincipalConfiguration.class), ImmutableMap.of(SERVICE_PID, "optionalService")); SecurityProvider service2 = context.getService(SecurityProvider.class); assertSame(service, service2); @@ -289,7 +303,7 @@ f.setAccessible(true); TokenConfiguration tc = mockConfiguration(TokenConfiguration.class); - registration.bindTokenConfiguration(tc, ImmutableMap.of(Constants.SERVICE_PID, "serviceId")); + registration.bindTokenConfiguration(tc, ImmutableMap.of(SERVICE_PID, "serviceId")); Preconditions preconditions = (Preconditions) f.get(registration); assertTrue(preconditions.areSatisfied()); @@ -303,13 +317,13 @@ registration.bindUserConfiguration(mockConfiguration(UserConfiguration.class)); - AuthorizableActionProvider ap = Mockito.mock(AuthorizableActionProvider.class); - registration.bindAuthorizableActionProvider(ap, ImmutableMap.of(Constants.SERVICE_PID, "actionProvider")); + AuthorizableActionProvider ap = mock(AuthorizableActionProvider.class); + registration.bindAuthorizableActionProvider(ap, ImmutableMap.of(SERVICE_PID, "actionProvider")); SecurityProvider service = context.getService(SecurityProvider.class); assertNotNull(service); - registration.unbindAuthorizableActionProvider(ap, ImmutableMap.of(Constants.SERVICE_PID, "actionProvider")); + registration.unbindAuthorizableActionProvider(ap, ImmutableMap.of(SERVICE_PID, "actionProvider")); service = context.getService(SecurityProvider.class); assertNull(service); } @@ -321,9 +335,9 @@ Field f = registration.getClass().getDeclaredField("preconditions"); f.setAccessible(true); - AuthorizableNodeName mock = Mockito.mock(AuthorizableNodeName.class); - registration.bindAuthorizableNodeName(mock, ImmutableMap.of(Constants.SERVICE_PID, "nodeName")); - registration.unbindAuthorizableNodeName(mock, ImmutableMap.of(Constants.SERVICE_PID, "nodeName")); + AuthorizableNodeName mock = mock(AuthorizableNodeName.class); + registration.bindAuthorizableNodeName(mock, ImmutableMap.of(SERVICE_PID, "nodeName")); + registration.unbindAuthorizableNodeName(mock, ImmutableMap.of(SERVICE_PID, "nodeName")); Preconditions preconditions = (Preconditions) f.get(registration); assertFalse(preconditions.areSatisfied()); @@ -333,19 +347,20 @@ public void testUnbindOptionalCandidateAfterRegistration() { registration.activate(context.bundleContext(), configWithRequiredServiceIds("serviceId")); - UserAuthenticationFactory uaf = Mockito.mock(UserAuthenticationFactory.class); - Map properties = ImmutableMap.of(Constants.SERVICE_PID, "notMandatory"); - registration.bindUserAuthenticationFactory(uaf, properties); + UserAuthenticationFactory uaf = mock(UserAuthenticationFactory.class); + ServiceReference serviceReference = when(mock(ServiceReference.class).getProperty(SERVICE_PID)).thenReturn("notMandatory").getMock(); + + registration.bindUserAuthenticationFactory(serviceReference, uaf); assertNull(context.getService(SecurityProvider.class)); - registration.bindAuthorizableActionProvider(Mockito.mock(AuthorizableActionProvider.class), ImmutableMap.of(Constants.SERVICE_PID, "serviceId")); + registration.bindAuthorizableActionProvider(mock(AuthorizableActionProvider.class), ImmutableMap.of(SERVICE_PID, "serviceId")); SecurityProvider service = context.getService(SecurityProvider.class); assertNotNull(service); // unbinding an optional configuration must not result in unrregistration of the service - registration.unbindUserAuthenticationFactory(uaf, properties); + registration.unbindUserAuthenticationFactory(serviceReference, uaf); SecurityProvider service2 = context.getService(SecurityProvider.class); assertSame(service, service2); @@ -550,8 +565,8 @@ public void testBindRestrictionProviderWithoutAuthorizationConfig() { registration.activate(context.bundleContext(), configWithRequiredServiceIds("serviceId")); - RestrictionProvider mockRp = Mockito.mock(RestrictionProvider.class); - registration.bindRestrictionProvider(mockRp, ImmutableMap.of(Constants.SERVICE_PID, "serviceId")); + RestrictionProvider mockRp = mock(RestrictionProvider.class); + registration.bindRestrictionProvider(mockRp, ImmutableMap.of(SERVICE_PID, "serviceId")); SecurityProvider service = context.getService(SecurityProvider.class); assertNotNull(service); @@ -568,9 +583,9 @@ public void testBindRestrictionProviderWithAuthorizationConfig() { registration.activate(context.bundleContext(), configWithRequiredServiceIds("rpId", "authorizationId")); - RestrictionProvider mockRp = Mockito.mock(RestrictionProvider.class); - registration.bindRestrictionProvider(mockRp, ImmutableMap.of(Constants.SERVICE_PID, "rpId")); - registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(Constants.SERVICE_PID, "authorizationId")); + RestrictionProvider mockRp = mock(RestrictionProvider.class); + registration.bindRestrictionProvider(mockRp, ImmutableMap.of(SERVICE_PID, "rpId")); + registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(SERVICE_PID, "authorizationId")); SecurityProvider service = context.getService(SecurityProvider.class); RestrictionProvider rp = service.getConfiguration(AuthorizationConfiguration.class).getRestrictionProvider(); @@ -584,7 +599,7 @@ SecurityProvider service = context.getService(SecurityProvider.class); assertNull(service); - registration.bindAuthorizableNodeName(Mockito.mock(AuthorizableNodeName.class), ImmutableMap.of(RegistrationConstants.OAK_SECURITY_NAME, "serviceId")); + registration.bindAuthorizableNodeName(mock(AuthorizableNodeName.class), ImmutableMap.of(RegistrationConstants.OAK_SECURITY_NAME, "serviceId")); service = context.getService(SecurityProvider.class); assertNotNull(service); @@ -594,8 +609,8 @@ public void testActivateWithMixedServicePiAnddOakServiceName() { registration.activate(context.bundleContext(), configWithRequiredServiceIds("rpId", "authorizationId")); - RestrictionProvider mockRp = Mockito.mock(RestrictionProvider.class); - registration.bindRestrictionProvider(mockRp, ImmutableMap.of(Constants.SERVICE_PID, "rpId")); + RestrictionProvider mockRp = mock(RestrictionProvider.class); + registration.bindRestrictionProvider(mockRp, ImmutableMap.of(SERVICE_PID, "rpId")); registration.bindAuthorizationConfiguration(new AuthorizationConfigurationImpl(), ImmutableMap.of(RegistrationConstants.OAK_SECURITY_NAME, "authorizationId")); SecurityProvider service = context.getService(SecurityProvider.class); @@ -603,6 +618,27 @@ assertTrue(rp instanceof WhiteboardRestrictionProvider); } + @Test + public void testMultipleUserAuthenticationFactoriesRespectsRanking() throws Exception { + context.registerService(SecurityProviderRegistration.class, registration, ImmutableMap.of("requiredServicePids", new String[] {"uaf1", "uaf2", "uaf3"})); + + UserAuthenticationFactory uaf1 = new UserAuthenticationFactoryImpl(); + UserAuthenticationFactory uaf2 = new UserAuthenticationFactoryImpl(); + UserAuthenticationFactory uaf3 = new UserAuthenticationFactoryImpl(); + + context.registerInjectActivateService(uaf1, ImmutableMap.of(RegistrationConstants.OAK_SECURITY_NAME, "uaf1", Constants.SERVICE_RANKING, 50)); + context.registerInjectActivateService(uaf2, ImmutableMap.of(RegistrationConstants.OAK_SECURITY_NAME, "uaf2")); + context.registerInjectActivateService(uaf3, ImmutableMap.of(RegistrationConstants.OAK_SECURITY_NAME, "uaf3", Constants.SERVICE_RANKING, 1)); + + Field f = registration.getClass().getDeclaredField("userAuthenticationFactories"); + f.setAccessible(true); + + SortedMap m = (SortedMap) f.get(registration); + assertEquals(3, m.size()); + Collection c = m.values(); + assertTrue(Iterables.elementsEqual(ImmutableList.of(uaf2, uaf3, uaf1), c)); + } + private static class ContextImpl implements Context { @Override