[
https://issues.apache.org/jira/browse/SLING-6285?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15692743#comment-15692743
]
Julian Sedding edited comment on SLING-6285 at 11/24/16 1:03 PM:
-----------------------------------------------------------------
[~bdelacretaz], your observation about
{{AbstractSlingRepositoryManager.isAllowLoginAdministrativeForBundle
-Overridden-}} is correct, it is not required. However, unfortunately this
interface change has is already released in version 2.4.2. So removing it
would require a major version bump IIUC, which I wanted to avoid. We can
deprecate it of course and remove or make it private later on.
Having the possibility to override {{isAllowLoginAdministrativeForBundle
-Overridden-}} allows implementations extending from the abstract base classes
to implement a different strategy for whitelisting. So far this was only
helpful for unit testing, see {{MockSlingRepositoryManager}}.
I chose to use reflection, because that allowed me to only bind the
{{LoginAdminWhitelist}} if the subclass inherits
{{isAllowLoginAdministrativeForBundle -Overridden-}}. Note also that the
reflection calls happen exactly once during {{activate}}.
We can introduce another method to indicate that we indeed want to use the base
whitelist implementation, however, this is error prone, because implementations
have to turn two screws in sync: they need to indicate whether or not they
override {{isAllowLoginAdministrativeForBundle -Overridden-}}.
Maybe the best thing is to deprecate {{isAllowLoginAdministrativeForBundle
-Overridden-}} now, which allows us to remove the reflection based code once we
remove the method.
edit: this comment was originally incorrectly referring to
{{isAllowLoginAdministrativeForBundleOverridden}} instead of
{{isAllowLoginAdministrativeForBundle}}, I edited to strike through the
{{-Overridden-}} part.
was (Author: jsedding):
[~bdelacretaz], your observation about
{{AbstractSlingRepositoryManager.isAllowLoginAdministrativeForBundleOverridden}}
is correct, it is not required. However, unfortunately this interface change
has is already released in version 2.4.2. So removing it would require a major
version bump IIUC, which I wanted to avoid. We can deprecate it of course and
remove or make it private later on.
Having the possibility to override
{{isAllowLoginAdministrativeForBundleOverridden}} allows implementations
extending from the abstract base classes to implement a different strategy for
whitelisting. So far this was only helpful for unit testing, see
{{MockSlingRepositoryManager}}.
I chose to use reflection, because that allowed me to only bind the
{{LoginAdminWhitelist}} if the subclass inherits
{{isAllowLoginAdministrativeForBundleOverridden}}. Note also that the
reflection calls happen exactly once during {{activate}}.
We can introduce another method to indicate that we indeed want to use the base
whitelist implementation, however, this is error prone, because implementations
have to turn two screws in sync: they need to indicate whether or not they
override {{isAllowLoginAdministrativeForBundleOverridden}}.
Maybe the best thing is to deprecate
{{isAllowLoginAdministrativeForBundleOverridden}} now, which allows us to
remove the reflection based code once we remove the method.
> Implement LoginAdminWhitelist in JCR Base
> -----------------------------------------
>
> Key: SLING-6285
> URL: https://issues.apache.org/jira/browse/SLING-6285
> Project: Sling
> Issue Type: Improvement
> Components: JCR
> Affects Versions: JCR Base 2.4.2
> Reporter: Julian Sedding
> Assignee: Julian Sedding
> Fix For: JCR Oak Server 1.1.4, JCR Base 3.0.0
>
> Attachments: SLING-6285-jsedding.patch
>
>
> JCR Base should provide a default implementation
> {{AbstractSlingRepositoryManager#allowLoginAdministrativeForBundle(Bundle)}}
> with a configurable {{LoginAdminWhitelist}}. Implementation that want a
> different logic can then still choose to overwrite the method.
> cc [~cziegeler], [~bdelacretaz]
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
