hi felix
i had a closer look at the patch in SLING-2944. here some
additional comments:
SlingRepository.java:
--------------------------------------------------------------------
- lacks whitespace in the javadoc of #loginService:
+ * Returns a sessionto the given workspace with privileges
+ * Returns a session to the given workspace with privileges
- IMO the param description of @param serviceInfo is not self
explaining. i would suggest that you either quickly describe what
the serviceInfo is or link to another location for further
information.
JcrResourceProviderFactory:
--------------------------------------------------------------------
- typo in the name of the repository constant
+ private static final String REPOSITORY_REFERNENCE_NAME =
"repository";
+ private static final String REPOSITORY_REFERENCE_NAME =
"repository";
=> and fixing accordingly in the @Reference annotation for
the repositoryReference field and in the active method.
Regarding deprecation of loginAdministrative et al
--------------------------------------------------------------------
on the wiki page you state that there will be a configuration flag
to disable those methods. is that still outstanding or did i miss it?
IMO that would be really cool to have as we definitely want to
get rid of the admin-logins.
related to this: i got the impression that the deprecated methods are
still used in AbstractSlingRepository and JcrResourceProviderFactory.
wouldn't it be better to replace them with an internal call such
that the API method can really be disabled?
Regarding implementation of #loginService
--------------------------------------------------------------------
this is currently achieved by loginAdministrative followed by an
impersonation. as of oak where we plan to have pluggable and
osgi aware principal mgt exposed on the OAK layer. this could
potentially be used to simplify the call and replace it by a
single pre-authenticated login where the subject for the service
user is created upfront by retrieve the principal set for the
specified service userId.
ServiceUserMapper
--------------------------------------------------------------------
if we agree that userId was better than user name, the javadoc of
the ServiceUserMapper should be adjusted to consistently use userId.
what i am missing in the documentation is a short note about what
a service user is and if/how that service users should/could be
different from regular users. since that service user mapper goes
along with a modification in jackrabbit that allows to create user
accounts without password it might we worth mentioning somewhere
in the documentation that service users are not necessarily intended
to perform a regular SlingRepository#login (which in combination with
jackrabbit and oak can be achieved by creating those special users
without password).
Mapping
--------------------------------------------------------------------
here again 'userName' should be replaced by userId.
ServiceUserMapperImpl
--------------------------------------------------------------------
same here: references to the user's name should be replaced by user Id.
regarding the configuration property PROP_DEFAULT_USER:
while is see the benefit of having it, there is IMHO just
one reasonable default value, which is again the id of the
administrator.
so, i am a bit undecided if it is really a good thing... if it stays
i would suggest to add some more information indicating the security
impact of the default value.
apart from those things i am convinced that the service user concept
is really great leap forward in making sling secure by default.
thanks again, i really appreciate this effort!
angela
On 7/4/13 11:01 AM, Felix Meschberger wrote:
Hi all
It has been noted that our SlingRepository.loginAdministrative and
ResourceResolverFactory.getAdministrativeResourceResolver solve a problem but
are suboptimal in that they provide administrative privileges. To solve this
dilemma I have created the Service Authentication concept [1] and implemented a
prototype [2].
I am now ready to reintegrate this prototype into the main code base tracked by
SLING-2944 [3]. The proposed changes to the code base are attached as a patch
(of existing code) and a package of the new Service User Mapper bundle.
I would like you to review that code before I go ahead integrating it into the
code base sometime next week. Thank you very much.
Regards
Felix
[1] https://cwiki.apache.org/confluence/display/SLING/Service+Authentication
[2]
http://svn.apache.org/repos/asf/sling/whiteboard/fmeschbe/deprecate_login_administrative
[3] https://issues.apache.org/jira/browse/SLING-2944
