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
