OAK-6756: Convert oak-auth-external to OSGi R7 annotations

[mbaedke]

done.

[jsedding]

With the last commit, the diff in OSGi metadata is as follows

org.apache.jackrabbit.oak.spi.security.authentication.external.impl.DefaultSyncHandler
    MetaType
        Attributes
            - namespace = http://www.osgi.org/xmlns/metatype/v1.0.0 (String)
            + namespace = http://www.osgi.org/xmlns/metatype/v1.2.0 (String)
        Designates
            org.apache.jackrabbit.oak.spi.security.authentication.external.impl.DefaultSyncHandler
                - pid = org.apache.jackrabbit.oak.spi.security.authentication.external.impl.DefaultSyncHandler (String)
        ObjectClassDefinitions
            org.apache.jackrabbit.oak.spi.security.authentication.external.impl.DefaultSyncHandler
                - description = Description for org.apache.jackrabbit.oak.spi.security.authentication.external.impl.DefaultSyncHandler (String)
                + description =  (String)

org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalLoginModuleFactory
    Declarative Services
        References
            syncManager
                - bind = bindSyncManager (String)
                - unbind = unbindSyncManager (String)
                + field-collection = service (String)
            idpManager
                - bind = bindIdpManager (String)
                - unbind = unbindIdpManager (String)
                + field-collection = service (String)
    MetaType
        Attributes
            - namespace = http://www.osgi.org/xmlns/metatype/v1.0.0 (String)
            + namespace = http://www.osgi.org/xmlns/metatype/v1.2.0 (String)
        Designates
            org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalLoginModuleFactory
                - pid = org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalLoginModuleFactory (String)
        ObjectClassDefinitions
            org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalLoginModuleFactory
                - description = Description for org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalLoginModuleFactory (String)
                + description =  (String)

org.apache.jackrabbit.oak.spi.security.authentication.external.impl.principal.ExternalPrincipalConfiguration
    MetaType
        Attributes
            - namespace = http://www.osgi.org/xmlns/metatype/v1.0.0 (String)
            + namespace = http://www.osgi.org/xmlns/metatype/v1.2.0 (String)
        ObjectClassDefinitions
            org.apache.jackrabbit.oak.spi.security.authentication.external.impl.principal.ExternalPrincipalConfiguration
                - description = Description for org.apache.jackrabbit.oak.spi.security.authentication.external.impl.principal.ExternalPrincipalConfiguration
(String)
                + description =  (String)

[anchela]

@jsedding , thanks.... but honestly i am not sure that the new diff is actually good or still needs some work.

[anchela]

@mbaedke , the changes looks reasonable to me and i just had 2 questions about the duplication of certain properties.
however, i have found in the past the this type of conversion is prone to subtle changes that are hard to spot but with huge impact (regressions). did you test the changes at hand with an AEMaaCS deployment that comes with external-authentication out of the box?

[anchela]

@mbaedke , these 3 properties are also present as AttributeDefinitions in the configuration below. what is the impact of having them listed here as well?

[jsedding]

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.

[anchela]

same as above.

[jsedding]

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.

[jsedding]

[...] i am not sure that the new diff is actually good or still needs some work.

@anchela At the risk of explaining things you already know, here I go. Most remaining diffs relate to the metatype, which is informational and used predominantly for the rendering of configuration UIs. The most risky diff in the metatype is IMHO the removal of the Designates > pid field. This seems to be a recurring pattern in this sort of refactoring, and I assume that it defaults to the value of the line above (not sure what it's called).

The only change in the declarative services XML is the change of two field from method injection to field injection. I did this, because I suspect that the previous implementation was not 100% correct. I think that during an "update" of a bound service, if the reference is dynamic (which it isn't here), the "bind" method is called before the "unbind" method. I.e. in "unbind" you need to check if the value in the field and the value to be unbound are the same object before you set it to null. Since the bind/unbind methods in question had no other logic than setting the field, I thought it safer to switch them to field injection.

TL;DR - With all that said, I am confident that this PR is good to be merged. 🙂

[mbaedke]

@anchela I only tested with a local QS build. I don't even know if I can easily access testing platforms for AEMaaCS deployments as an external dev.

[mbaedke]

@jsedding Re removal of the pid field: AFAIU the Felix SCM plugin just creates this explicitly with the default value. I have never seen that it causes problems to drop that.

[jsedding]

I read the spec on the subject of the pid in the Designate element and found that the pid attribute is ignored if a factoryPid attribute is present. Both, the DefaultSyncHandler and the ExternalLoginModuleFactory have the factoryPid attribute specified. Thus the new version is arguably more correct than the old version that includes the ignored pid attribute.

The getPids() method returns an array of PIDs that were specified in the pid attribute of the Object elements. The getFactoryPids() method returns an array of the factoryPid attributes. For factories, the related pid attribute is ignored because all instances of a factory must share the same meta type. (emphasis is mine)

Source: https://docs.osgi.org/specification/osgi.cmpn/8.0.0/service.metatype.html#i1492258

[mbaedke]

@jsedding , that actually makes sense.
@anchela, do you have specific worries about the diff? To me it looks perfectly fine.

[anchela]

@jsedding , i didn't know. so thanks for all the explanation that helps
@mbaedke , thanks for the additional info....

so, i think we are good.... and sorry for not being responsive. was busy elsewhere and didn't have too much time to help here.

[anchela]

see previous comment. lgtm

[jsedding]

LGTM as well!