jsedding commented on code in PR #1371:
URL: https://github.com/apache/jackrabbit-oak/pull/1371#discussion_r1604478097
##########
oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/ExternalLoginModuleFactory.java:
##########
@@ -45,100 +37,123 @@
import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard;
import org.osgi.framework.BundleContext;
import org.osgi.service.component.ComponentContext;
+import org.osgi.service.component.annotations.Component;
+import org.osgi.service.component.annotations.Activate;
+import org.osgi.service.component.annotations.Deactivate;
+import org.osgi.service.component.annotations.ConfigurationPolicy;
+import org.osgi.service.component.annotations.Reference;
+import org.osgi.service.component.annotations.ReferencePolicy;
+import org.osgi.service.component.annotations.ReferenceCardinality;
+import org.osgi.service.metatype.annotations.Designate;
+import org.osgi.service.metatype.annotations.ObjectClassDefinition;
+import org.osgi.service.metatype.annotations.AttributeDefinition;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static
org.apache.jackrabbit.oak.spi.security.authentication.external.basic.AutoMembershipConfig.PARAM_SYNC_HANDLER_NAME;
+
/**
* Implements a LoginModuleFactory that creates {@link ExternalLoginModule}s
and allows to configure login modules
* via OSGi config.
*/
@Component(
- label = "Apache Jackrabbit Oak External Login Module",
- metatype = true,
- policy = ConfigurationPolicy.REQUIRE,
- configurationFactory = true
+ configurationPolicy = ConfigurationPolicy.REQUIRE,
+ service = {
+ LoginModuleFactory.class,
+ SyncHandlerMapping.class
+ },
+ property = {
Review Comment:
These are default service properties for the case when no configuration is
present. I agree (and have discussed with @mbaedke) that they are not strictly
necessary here, because `ConfigurationPolicy.REQUIRE` enforces the presence of
a configuration, which then contributes these values to the service properties.
I added them to eliminate them from the diff (they were there before the
change), partially because I once got bitten by a similar seemingly innocuous
change. If you prefer, I think we can safely remove them here.
##########
oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalPrincipalConfiguration.java:
##########
@@ -75,34 +74,59 @@
* @see <a href="https://issues.apache.org/jira/browse/OAK-4101";>OAK-4101</a>
*/
@Component(
- metatype = true,
- label = "Apache Jackrabbit Oak External PrincipalConfiguration",
- immediate = true
+ immediate = true,
+ service = {
+ PrincipalConfiguration.class,
+ SecurityConfiguration.class
+ },
+ property = {
Review Comment:
In this case we don't have `ConfigurationPolicy.REQUIRED`. That means the
component is started without configuration. Without having these default
service properties, they would be missing from a service without configuration.
AFAIK the property `oak.security.name` is used by consumers of this service to
verify the presence of various implementations. Thus, in the absence of this
property and the service running without explicit configuration, there is a
high chance for a regression. I recommend keeping these properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
