hi felix, what's the status? can't wait to play around with this feature ;)
thanks, raphael On Fri, Jul 5, 2013 at 8:20 PM, Felix Meschberger <[email protected]> wrote: > Hi Angela, > > Thanks alot for the extensive feedback. Very much appreciated ! > > Am 05.07.2013 um 11:01 schrieb Angela Schreiber: > >> 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 >> > ACK > >> - 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. >> > > ACK > >> >> 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. >> > > ACK > >> >> 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. > > Oops, missed that. Yes, that makes perfect sense. I will add it with values > "enabled" (will write a warn log message) and "disabled" (will throw a > LoginException and log an error message explaining the case) > > For backwards compatibility I would think that initially this will default to > "enabled" with. Later we can change that default. > > WDYT ? > >> >> 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? > > Yes, absolutely. > >> >> >> 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. > > We could split the loginService(Bundle, ...) method to allow plugging a > different implementation for the actual session creation once the user has > been defined. > > The default implementation would be the impersonation based one and an > improved one based on Oak could implenent that. > >> >> >> ServiceUserMapper >> -------------------------------------------------------------------- >> if we agree that userId was better than user name, the javadoc of >> the ServiceUserMapper should be adjusted to consistently use userId. > > Absolutely > >> >> 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). > > Good point. > >> >> >> Mapping >> -------------------------------------------------------------------- >> here again 'userName' should be replaced by userId. > > ACK > >> >> >> ServiceUserMapperImpl >> -------------------------------------------------------------------- >> same here: references to the user's name should be replaced by user Id. >> > > ACK > >> 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. > > The default for the default is user is none, hence if there is no user > defined for a service, the service will by default have no access. But I > agree, there is a risk for lazily define admin as the default user and thus > basically circumvent the mechanism. > > Yet, this would be an administrative, deployment decision as opposed to a > developer decision using loginAdministrative. Hence the system must > explicitly be made vulnerable. > > What do others think ? > >> >> 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! > > Thanks. Your input is really appreciated ! > > Regards > Felix > >> 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 >
