On Wed, Apr 24, 2013 at 3:38 PM, Thomas Delafosse < thomas.delafosse(a)xwiki.com&gt; wrote: IMO, you should use an existing API that will be deprecated as soon as we have a real security authentication module. However, I not think com.xpn.xwiki.plugin.rightsmanager.RightsManagerPluginApi to be the right place, I would see it more in com.xpn.xwiki.user.api.XWikiUser, with the advantage that reaching it will require PR (preventing brute force attack). In the new authentication module, the abstraction should be really improved, allowing to change the password outside of the XWiki as well, if the authentication backend support such feature. The notion of password will need to be abstracted as well, since there is more then just password for authentication. So, this will surely be another story, and it is not foreseeable now.

Hi, Thanks for the piece of advice. Since I need access to the context and would like to be able to put some warning in the logs if an error occurs while checking the password, I think I would put the method in com.xpn.xwiki.api.User rather than in XWikiUser. But of course I would check for the Programming Rights to avoid Brute force.

Thanks, Thomas On Thu, Apr 25, 2013 at 5:02 PM, Vincent Massol &lt;vincent(a)massol.net&gt; wrote: > Hi, > > On Apr 25, 2013, at 12:15 AM, Denis Gervalle &lt;dgl(a)softec.lu&gt; wrote: > >> On Wed, Apr 24, 2013 at 3:38 PM, Thomas Delafosse < >> thomas.delafosse(a)xwiki.com&gt; wrote: >> >>> Hello all, >>> >>> I've been working on some improvements on user changing password (see >>> XWiki-6882). In particular, I tried to make mandatory, for an user > wanting >>> to change his password, to submit also his current password, so that I >>> could check it. >>> The problem is that there is no way to make this check through > velocity. I >>> tried to use some groovy instead, but it breaks the functional tests. > So I >>> need to introduce a new method "checkPassword" accessible from velocity >>> scripts. The question is, where should I implement it ? >>> There are two possibilities >>> 1) Wrote a new component >>> 2) Add this method in an existing API. >>> I don't really like 1), as I feel it would be strange to introduce a new >>> service with only one method. >>> In the meanwhile, for 2), I don't really know in which API this method >>> could fit. Sergiu told me that I could perhaps put it in >>> com.xpn.xwiki.plugin.rightsmanager.RightsManagerPluginApi, >>> but that it wasn't really good either. Any ideas ? >>> >> >> IMO, you should use an existing API that will be deprecated as soon as we >> have a real security authentication module. However, I not think >> com.xpn.xwiki.plugin.rightsmanager.RightsManagerPluginApi to be the right >> place, I would see it more in com.xpn.xwiki.user.api.XWikiUser, with >> the advantage that reaching it will require PR (preventing brute force >> attack). >> >> In the new authentication module, the abstraction should be really >> improved, allowing to change the password outside of the XWiki as well, > if >> the authentication backend support such feature. The notion of password >> will need to be abstracted as well, since there is more then just > password >> for authentication. So, this will surely be another story, and it is >> not foreseeable now. > > I agree with Denis here. Regarding the location in the existing code, I > don't have any strong opinion. > > Thanks > -Vincent

You mean that there is no way we could make functional tests work if I need to check the user password, since this should require PR? Should we give up this password verification then ?

On Mon, Apr 29, 2013 at 1:34 PM, Sergiu Dumitriu &lt;sergiu(a)xwiki.com&gt; wrote: > On 04/29/2013 06:22 AM, Thomas Delafosse wrote: >> Hi, >> >> Thanks for the piece of advice. Since I need access to the context > and >> would like to be able to put some warning in the logs if an error occurs >> while checking the password, I think I would put the method in >> com.xpn.xwiki.api.User rather than in XWikiUser. But of course I would >> check for the Programming Rights to avoid Brute force. > > This won't solve the failing tests problem, since they fail because we > don't have an Admin account with programming rights. > >> Thanks, >> >> Thomas >> >> >> On Thu, Apr 25, 2013 at 5:02 PM, Vincent Massol &lt;vincent(a)massol.net&gt; > wrote: >> >>> Hi, >>> >>> On Apr 25, 2013, at 12:15 AM, Denis Gervalle &lt;dgl(a)softec.lu&gt; wrote: >>> >>>> On Wed, Apr 24, 2013 at 3:38 PM, Thomas Delafosse < >>>> thomas.delafosse(a)xwiki.com&gt; wrote: >>>> >>>>> Hello all, >>>>> >>>>> I've been working on some improvements on user changing password > (see >>>>> XWiki-6882). In particular, I tried to make mandatory, for an user >>> wanting >>>>> to change his password, to submit also his current password, so that I >>>>> could check it. >>>>> The problem is that there is no way to make this check through >>> velocity. I >>>>> tried to use some groovy instead, but it breaks the functional tests. >>> So I >>>>> need to introduce a new method "checkPassword" accessible from > velocity >>>>> scripts. The question is, where should I implement it ? >>>>> There are two possibilities >>>>> 1) Wrote a new component >>>>> 2) Add this method in an existing API. >>>>> I don't really like 1), as I feel it would be strange to introduce a > new >>>>> service with only one method. >>>>> In the meanwhile, for 2), I don't really know in which API this method >>>>> could fit. Sergiu told me that I could perhaps put it in >>>>> com.xpn.xwiki.plugin.rightsmanager.RightsManagerPluginApi, >>>>> but that it wasn't really good either. Any ideas ? >>>>> >>>> >>>> IMO, you should use an existing API that will be deprecated as soon as > we >>>> have a real security authentication module. However, I not think >>>> com.xpn.xwiki.plugin.rightsmanager.RightsManagerPluginApi to be the > right >>>> place, I would see it more in com.xpn.xwiki.user.api.XWikiUser, with >>>> the advantage that reaching it will require PR (preventing brute force >>>> attack). >>>> >>>> In the new authentication module, the abstraction should be really >>>> improved, allowing to change the password outside of the XWiki as well, >>> if >>>> the authentication backend support such feature. The notion of password >>>> will need to be abstracted as well, since there is more then just >>> password >>>> for authentication. So, this will surely be another story, and it is >>>> not foreseeable now. >>> >>> I agree with Denis here. Regarding the location in the existing code, I >>> don't have any strong opinion. >>> >>> Thanks >>> -Vincent

Hi all, After discussing it with Vincent, it seems that it would be better to be able to access this method without PR : thus we could keep the code for changing the password in passwd.vm instead of having to make a new page with PR for that. To avoid malicious users to use it nonetheless, I propose that this method could only be used to check the current user password, and only on its profile page. Does this seems OK to you, or do you think this should be done another way ?

I think it's more secure to let it be used only on the current user profile page. Otherwise we can imagine an attacker creating a page where this check is performed against the current user, enabling him to gain information about the users visiting this page. (For example he could do something like #foreach($passwd in $passwdList) #if($xwiki.getUser().checkPassword($passwd)) Store this information somewhere (in another doc, in an object, or even by sending me a mail) #end #end)

And I don't think that users with PR need to be able to make this check on any user (and if they need they can still perform it through the core), so I prefer to keep it this way.

Cheers, Thomas On Mon, May 13, 2013 at 5:42 PM, Sergiu Dumitriu &lt;sergiu(a)xwiki.org&gt; wrote: > On 05/06/2013 09:44 AM, Thomas Delafosse wrote: >> Hi all, >> >> After discussing it with Vincent, it seems that it would be better to >> be able to access this method without PR : thus we could keep the code > for >> changing the password in passwd.vm instead of having to make a new page >> with PR for that. To avoid malicious users to use it nonetheless, I > propose >> that this method could only be used to check the current user password, > and >> only on its profile page. >> Does this seems OK to you, or do you think this should be done another > way ? > > Why only on the user's profile page? > > The method could allow public check only for the current user, and PR > check for any user.

On Mon, May 13, 2013 at 6:24 PM, Sergiu Dumitriu &lt;sergiu(a)xwiki.org&gt; wrote: You are right, there is currently ways to workaround this check. But I hope that with Andreas' branch it would be harder to find such leaks, and anyway it makes this attack available only to attackers having found these leaks. Even if this is not perfect, I feel more comfortable this way. I agree, it could be useful to check the password at some other points. So what we could do is allow public check only from the user's profile page, and PR check from any page. The issue with the change of password is that the template doesn't have PR, but I guess that in the scenarios you mention, this would be done from a normal wiki page, and thus we could ask this page to have PR. What do you think ?

> >> And I don't think that users with PR need to be able to make this check > on >> any user (and if they need they can still perform it through the core), > so >> I prefer to keep it this way. > > Agreed. > >> Cheers, >> >> Thomas >> >> >> On Mon, May 13, 2013 at 5:42 PM, Sergiu Dumitriu &lt;sergiu(a)xwiki.org&gt; > wrote: >> >>> On 05/06/2013 09:44 AM, Thomas Delafosse wrote: >>>> Hi all, >>>> >>>> After discussing it with Vincent, it seems that it would be better > to >>>> be able to access this method without PR : thus we could keep the code >>> for >>>> changing the password in passwd.vm instead of having to make a new page >>>> with PR for that. To avoid malicious users to use it nonetheless, I >>> propose >>>> that this method could only be used to check the current user password, >>> and >>>> only on its profile page. >>>> Does this seems OK to you, or do you think this should be done another >>> way ? >>> >>> Why only on the user's profile page? >>> >>> The method could allow public check only for the current user, and PR >>> check for any user. >