Hi Felix, I just did wonder if this really can replace loginAdministrative(). Although I'd really like to get rid of the adminSession due to various (mostly securitybased) reasons, I don't see all usecases covered by this solution. The one I have explicitly in mind is the impersonation to users. Just think of asynchronous processing of content by a service _in the name and under the restrictions of a user_ Nowadays you'd get an adminsession and then impersonate it since the adminsession is the only session implicitly allowed to impersonate any user. Beside the fact that impersonation is a problematic feature in sense of tracibility (not being identified as impersonated session in the logs) this seems to be the only way to fullfill such a task.
Cheers Dominik 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 > >
