jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Francesco Mari (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (OAK-3201) Use static references in SecurityProviderImpl for composite services
Date Thu, 17 Sep 2015 07:25:45 GMT

    [ https://issues.apache.org/jira/browse/OAK-3201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14791710#comment-14791710
] 

Francesco Mari commented on OAK-3201:
-------------------------------------

bq. Setting immediate to true - That should not be required per se as DS can figure out that
there is a requirement of services of given type and those components would be then activated.
So some component would have dependency on SecurityProvider which in turn expresses its dependency
on various providers and thus DS would initialize them. So you can omit that change.

I will revert those changes before committing (or if I post another patch for review, of course).

bq. SecurityProviderRegistrationTest - Various stub class can possibly be removed with Mocks.

I hoped to use Groovy's MockFor, but it doesn't work. I will user Mockito instead.

bq. Preconditions - areSatisfied - You can also use Guava Sets.difference there

I will use Guava for that, but I will not propose a new patch for review because of this change.

bq. createCompositePrincipalConfiguration, createCompositeTokenConfiguration - Given that
by the time SecurityProvider is being constructed all the dependencies are met you can just
as well populate the actual composites.

This may work the first time, but it doesn't work if another, non-required PrincipalConfiguration
or TokenConfiguration is subsequently bound to SecurityProviderRegistration. Moreover, an
unregistration of the SecurityProvider followed by a registration creates a new instance of
SecurityProvider that has to be bound to the configurations (again).

bq. createCompositeRestrictionProvider, createCompositeAuthorizableActionProvider - Here also
we just well initialize the composites with required instances. This would prevent unnecessary
garbage creation of a new list for every invocation.

Similar problem of the point above. New, non-required instances of RestrictionProvider or
AuthorizableActionProvider can be bound at any time after the registration of the SecurityProvider.

bq. getRequiredServicePids - It would be better to also pass a default value there and not
rely on metatype providing default values.

I already provide a default value, which is the empty array. If you think that the default
value should be the PIDs of services that I think should be required, I don't feel comfortable
with that. If the list of required PIDs change over time, I don't want to recompile oak-core
because the default value was hardcoded in SecurityProviderRegistration.

> Use static references in SecurityProviderImpl for composite services
> --------------------------------------------------------------------
>
>                 Key: OAK-3201
>                 URL: https://issues.apache.org/jira/browse/OAK-3201
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: core
>            Reporter: Francesco Mari
>            Assignee: Francesco Mari
>             Fix For: 1.3.7
>
>         Attachments: OAK-3201-01.patch, OAK-3201-02.patch, OAK-3201-03.patch, OAK-3201-04.patch,
OAK-3201-05.patch, OAK-3201-06.patch, OAK-3201-07.patch, OAK-3201-08.patch, mbean-test.log
>
>
> {{SecurityProviderImpl}} has dynamic references to many other services, like {{RestrictionProvider}},
that represent the configuration of this component.
> Being these services dynamic, the OSGi runtime has no clear dependency relationship between
the {{SecurityProviderImpl}} and the required services. Thus, it may happen that an instance
of {{SecurityProviderImpl}} is published before the services it requires are started, creating
a window where the {{SecurityProviderimpl}} is operating differently from the way it's configured.
> I suggest to turn the dynamic references in {{SecurityProviderImpl}} to static ones to
improve the consistency of the implementation.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message