You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lenya.apache.org by an...@apache.org on 2006/06/09 11:01:59 UTC

svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Author: andreas
Date: Fri Jun  9 02:01:58 2006
New Revision: 412987

URL: http://svn.apache.org/viewvc?rev=412987&view=rev
Log:
Use separate usecase for changing password with check of old password. This avoids the security issue that the checkPassword parameter can be set by the client.

Added:
    lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPasswordWithCheck.java
    lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePasswordAdmin.xconf
Modified:
    lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPassword.java
    lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePassword.xconf
    lenya/trunk/src/webapp/lenya/usecases/admin/changePassword.jx

Modified: lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPassword.java
URL: http://svn.apache.org/viewvc/lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPassword.java?rev=412987&r1=412986&r2=412987&view=diff
==============================================================================
--- lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPassword.java (original)
+++ lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPassword.java Fri Jun  9 02:01:58 2006
@@ -23,18 +23,8 @@
  */
 public class UserPassword extends AccessControlUsecase {
 
-    protected static final String OLD_PASSWORD = "oldPassword";
     protected static final String NEW_PASSWORD = "password";
     protected static final String CONFIRM_PASSWORD = "confirmPassword";
-    
-    protected static final String CHECK_PASSWORD = "checkPassword";
-
-    /**
-     * Ctor.
-     */
-    public UserPassword() {
-        super();
-    }
 
     /**
      * @see org.apache.lenya.cms.usecase.AbstractUsecase#doCheckExecutionConditions()
@@ -42,15 +32,11 @@
     protected void doCheckExecutionConditions() throws Exception {
         super.doCheckExecutionConditions();
         
-        String checkOldPassword = getParameterAsString(CHECK_PASSWORD);
-        if (checkOldPassword != null && checkOldPassword.equals(Boolean.toString(true))) {
-            String oldPassword = getParameterAsString(OLD_PASSWORD);
-            boolean authenticated = this.user.authenticate(oldPassword);
-            if (!authenticated) {
-                addErrorMessage("The old password is not correct.");
-            }
+        if (this.user == null) {
+            addErrorMessage("The user ID has to be provided when executing this usecase.");
+            return;
         }
-        
+
         checkNewPassword(this);
     }
 
@@ -63,11 +49,16 @@
     }
 
     private User user;
+    
+    protected User getUser() {
+        return this.user;
+    }
 
     /**
      * @see org.apache.lenya.cms.usecase.Usecase#setParameter(java.lang.String, java.lang.Object)
      */
     public void setParameter(String name, Object value) {
+
         super.setParameter(name, value);
 
         if (name.equals(UserProfile.USER_ID)) {
@@ -76,7 +67,6 @@
             if (this.user == null) {
                 throw new RuntimeException("User [" + userId + "] not found.");
             }
-
         }
     }
 
@@ -100,6 +90,5 @@
             usecase.addErrorMessage("The password must contain at least one number.");
         }
     }
-
 
 }

Added: lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPasswordWithCheck.java
URL: http://svn.apache.org/viewvc/lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPasswordWithCheck.java?rev=412987&view=auto
==============================================================================
--- lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPasswordWithCheck.java (added)
+++ lenya/trunk/src/java/org/apache/lenya/cms/ac/usecases/UserPasswordWithCheck.java Fri Jun  9 02:01:58 2006
@@ -0,0 +1,37 @@
+/*
+ * Copyright  1999-2004 The Apache Software Foundation
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ *
+ */
+package org.apache.lenya.cms.ac.usecases;
+
+/**
+ * Usecase to change a user's password. The old password is checked.
+ */
+public class UserPasswordWithCheck extends UserPassword {
+
+    protected static final String OLD_PASSWORD = "oldPassword";
+
+    protected void doCheckExecutionConditions() throws Exception {
+
+        super.doCheckExecutionConditions();
+
+        String oldPassword = getParameterAsString(OLD_PASSWORD);
+        boolean authenticated = getUser().authenticate(oldPassword);
+        if (!authenticated) {
+            addErrorMessage("The old password is not correct.");
+        }
+    }
+
+}

Modified: lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePassword.xconf
URL: http://svn.apache.org/viewvc/lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePassword.xconf?rev=412987&r1=412986&r2=412987&view=diff
==============================================================================
--- lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePassword.xconf (original)
+++ lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePassword.xconf Fri Jun  9 02:01:58 2006
@@ -22,9 +22,11 @@
 
   <xconf xpath="/cocoon/usecases" unless="/cocoon/usecases/component-instance[@name = 'admin.changePassword']">
 
-    <component-instance name="admin.changePassword" logger="lenya.admin" class="org.apache.lenya.cms.ac.usecases.UserPassword">
+    <component-instance name="admin.changePassword" logger="lenya.admin"
+      class="org.apache.lenya.cms.ac.usecases.UserPasswordWithCheck">
       <view template="usecases/admin/changePassword.jx" menu="true">
         <tab group="admin" name="users"/>
+        <parameter name="checkPassword" value="true"/>
       </view>
       <exit usecase="admin.user"/>
     </component-instance>

Added: lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePasswordAdmin.xconf
URL: http://svn.apache.org/viewvc/lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePasswordAdmin.xconf?rev=412987&view=auto
==============================================================================
--- lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePasswordAdmin.xconf (added)
+++ lenya/trunk/src/webapp/lenya/config/cocoon-xconf/usecases/admin/usecase-admin-changePasswordAdmin.xconf Fri Jun  9 02:01:58 2006
@@ -0,0 +1,33 @@
+<?xml version="1.0"?>
+<!--
+  Copyright 1999-2005 The Apache Software Foundation
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+
+<!-- $Id: usecases-workflow-deactivate.xconf 348547 2005-11-23 20:13:01Z chestnut $ -->
+<!--
+    This file defines the publication specific use-cases
+-->
+
+  <xconf xpath="/cocoon/usecases" unless="/cocoon/usecases/component-instance[@name = 'admin.changePasswordAdmin']">
+
+    <component-instance name="admin.changePasswordAdmin" logger="lenya.admin" class="org.apache.lenya.cms.ac.usecases.UserPassword">
+      <view template="usecases/admin/changePassword.jx" menu="true">
+        <tab group="admin" name="users"/>
+        <parameter name="checkPassword" value="false"/>
+      </view>
+      <exit usecase="admin.user"/>
+    </component-instance>
+    
+  </xconf>

Modified: lenya/trunk/src/webapp/lenya/usecases/admin/changePassword.jx
URL: http://svn.apache.org/viewvc/lenya/trunk/src/webapp/lenya/usecases/admin/changePassword.jx?rev=412987&r1=412986&r2=412987&view=diff
==============================================================================
--- lenya/trunk/src/webapp/lenya/usecases/admin/changePassword.jx (original)
+++ lenya/trunk/src/webapp/lenya/usecases/admin/changePassword.jx Fri Jun  9 02:01:58 2006
@@ -38,7 +38,7 @@
             <jx:import uri="fallback://lenya/usecases/templates/messages.jx"/>
           </td>
         </tr>
-        <jx:if test="${usecase.getParameter('checkPassword').equals('true')}">
+        <jx:if test="${usecase.getView().getParameter('checkPassword').equals('true')}">
           <tr>
             <td class="lenya-entry-caption"><label for="oldPassword"><i18n:text>Old Password</i18n:text> *</label></td>
             <td><input type="password" name="oldPassword" class="lenya-form-element" value="${usecase.getParameter('oldPassword')}"/></td>



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@lenya.apache.org
For additional commands, e-mail: commits-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Jörn Nettingsmeier <po...@uni-duisburg.de>.
Andreas Hartmann wrote:
> Jörn Nettingsmeier wrote:
>>> That can be solved by lazy loading. But communication between classes
>>> should happen over methods, not fields - even in class hierarchies.
>>
>> i don't understand... does my code (see attachment) violate this?
> 
> I wouldn't call it "violate", it's rather a personal preference.
> I prefer to call a getter when necessary (A) to a setter which is
> invoked in any case (B):

thanks for the explanation. i tried to follow that pattern in the new 
version of my patch (should be in bugzilla by now).



-- 
"Open source takes the bullshit out of software."
	- Charles Ferguson on TechnologyReview.com

--
Jörn Nettingsmeier, EDV-Administrator
Institut für Politikwissenschaft
Universität Duisburg-Essen, Standort Duisburg
Mail: pol-admin@uni-due.de, Telefon: 0203/379-2736

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lenya.apache.org
For additional commands, e-mail: dev-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Andreas Hartmann <an...@apache.org>.
Jörn Nettingsmeier wrote:
> me again... :(
> 
> Andreas Hartmann wrote:
>> Jörn Nettingsmeier wrote:
>>> Andreas Hartmann wrote:
>>>> This sounds reasonable, and at the first glance your patch looks
>>>> very good. Maybe it would make sense to make
>>>>
>>>>   AbstractChangePassword.getUser()
>>>>
>>>> abstract. ChangePassword.getUser() would just return the currently
>>>> logged-in user, and AdminChangePassword() would return the user
>>>> determined by the userId parameter. WDYT?
>>>
>>> i tried it, but then it seemed better to have a variable
>>>   private User user
>>> that defaults to null in AbstractChangePassword, and to set it to the 
>>> current user in the constuctor of ChangePassword. otherwise the code 
>>> to determine the current user would be called many times...
>>
>> That can be solved by lazy loading. But communication between classes
>> should happen over methods, not fields - even in class hierarchies.
> 
> i don't understand... does my code (see attachment) violate this?

I wouldn't call it "violate", it's rather a personal preference.
I prefer to call a getter when necessary (A) to a setter which is
invoked in any case (B):

(A)

class Super {

     protected abstract User getUser();

     public void process() {
         doSomethingWith( getUser() );
     }

}

class Sub {

     private User user;

     protected User getUser() {
         // lazy load
         if (this.user == null) {
             this.user = ...
         }
         return this.user;
     }

}


(B)

class Super {

     private User user;

     protected void init() {
         ...
         (expect that subclass sets the user -> no explicit contract)
     }

     public void process() {
         doSomethingWith( this.user );
     }

}

class Sub {

     protected void init() {
         setUser(...);
     }

}


I'll reply to the rest of the mail separately.

HTH,

-- Andreas



-- 
Andreas Hartmann
Wyona Inc.  -   Open Source Content Management   -   Apache Lenya
http://www.wyona.com                      http://lenya.apache.org
andreas.hartmann@wyona.com                     andreas@apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lenya.apache.org
For additional commands, e-mail: dev-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Andreas Hartmann <an...@apache.org>.
Jörn Nettingsmeier wrote:
> Andreas Hartmann wrote:
>> Jörn Nettingsmeier wrote:
>>
>> [...]
>>
>>> anyways, right now i'm totally mystified by something else. somehow 
>>> the "user" attribute is reset when the usecase moves on, but i don't 
>>> know when or by what.
>>
>> This happens because the usecase component is released before the
>> response is sent, and recreated when the continuation is called.
>> This is necessary to avoid that stale components aren't released.
>>
>> All usecase handler classes have to be stateless, but the parameters
>> are restored, so you have to use parameters instead of fields.
> 
> ahh. ok, that explains many things :)
> 
> but i wonder: are these parameters sent to the client? iiuc yes.
> does that mean there is no way to store persistent information in a way 
> that cannot be tampered with by injecting POST or GET parameters, i.e. 
> to keep state information on the server side?

Yes, unfortunately this is true. It was the main reason why I had
to introduce a new class ChangePasswordWithCheck.

The most straightforward way would be to convert the Usecase Avalon
components to POJOs. This would mean that the initialization and
disposal wouldn't be handled by the container, but it would solve
the state information problem.

Here's a thread about this issue (no idea how to get a thread view):

http://mail-archives.apache.org/mod_mbox/lenya-dev/200504.mbox/%3Cd3iq49$6ef$1@sea.gmane.org%3E

What do the others think?

-- Andreas

-- 
Andreas Hartmann
Wyona Inc.  -   Open Source Content Management   -   Apache Lenya
http://www.wyona.com                      http://lenya.apache.org
andreas.hartmann@wyona.com                     andreas@apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lenya.apache.org
For additional commands, e-mail: dev-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Jörn Nettingsmeier <po...@uni-duisburg.de>.
Andreas Hartmann wrote:
> Jörn Nettingsmeier wrote:
> 
> [...]
> 
>> anyways, right now i'm totally mystified by something else. somehow 
>> the "user" attribute is reset when the usecase moves on, but i don't 
>> know when or by what.
> 
> This happens because the usecase component is released before the
> response is sent, and recreated when the continuation is called.
> This is necessary to avoid that stale components aren't released.
> 
> All usecase handler classes have to be stateless, but the parameters
> are restored, so you have to use parameters instead of fields.

ahh. ok, that explains many things :)

but i wonder: are these parameters sent to the client? iiuc yes.
does that mean there is no way to store persistent information in a way 
that cannot be tampered with by injecting POST or GET parameters, i.e. 
to keep state information on the server side?




-- 
"Open source takes the bullshit out of software."
	- Charles Ferguson on TechnologyReview.com

--
Jörn Nettingsmeier, EDV-Administrator
Institut für Politikwissenschaft
Universität Duisburg-Essen, Standort Duisburg
Mail: pol-admin@uni-due.de, Telefon: 0203/379-2736

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lenya.apache.org
For additional commands, e-mail: dev-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Andreas Hartmann <an...@apache.org>.
Jörn Nettingsmeier wrote:

[...]

> anyways, right now i'm totally mystified by something else. somehow the 
> "user" attribute is reset when the usecase moves on, but i don't know 
> when or by what.

This happens because the usecase component is released before the
response is sent, and recreated when the continuation is called.
This is necessary to avoid that stale components aren't released.

All usecase handler classes have to be stateless, but the parameters
are restored, so you have to use parameters instead of fields.

-- Andreas

-- 
Andreas Hartmann
Wyona Inc.  -   Open Source Content Management   -   Apache Lenya
http://www.wyona.com                      http://lenya.apache.org
andreas.hartmann@wyona.com                     andreas@apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lenya.apache.org
For additional commands, e-mail: dev-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Jörn Nettingsmeier <po...@uni-duisburg.de>.
me again... :(

Andreas Hartmann wrote:
> Jörn Nettingsmeier wrote:
>> Andreas Hartmann wrote:
>>> This sounds reasonable, and at the first glance your patch looks
>>> very good. Maybe it would make sense to make
>>>
>>>   AbstractChangePassword.getUser()
>>>
>>> abstract. ChangePassword.getUser() would just return the currently
>>> logged-in user, and AdminChangePassword() would return the user
>>> determined by the userId parameter. WDYT?
>>
>> i tried it, but then it seemed better to have a variable
>>   private User user
>> that defaults to null in AbstractChangePassword, and to set it to the 
>> current user in the constuctor of ChangePassword. otherwise the code 
>> to determine the current user would be called many times...
> 
> That can be solved by lazy loading. But communication between classes
> should happen over methods, not fields - even in class hierarchies.

i don't understand... does my code (see attachment) violate this?

anyways, right now i'm totally mystified by something else. somehow the 
"user" attribute is reset when the usecase moves on, but i don't know 
when or by what.
i log in and call my new changePassword usecase. whenever i submit, i 
get an npe:


java.lang.NullPointerException
	at 
org.apache.lenya.cms.ac.usecases.ChangePassword.doCheckExecutionConditions(ChangePassword.java:50)


i've added some debug statements, to see what happens to "user":

1, usecase is called:
**** user : lenya
Set this.user to "lenya".
getUser() called. Returning "lenya".
**** getUser(): lenya

all is fine and dandy so far.

2. form is completed with old and new password, and when i submit:

getUser() called. Returning "null".
****>> getUser(): null
getUser() called. Returning "null".


it seems that this variable is not persistent.

when i compare my code with the stuff in trunk, i see that it overrides 
the setParameter method, but i don't understand how that could make a 
difference.

please help!


jörn




-- 
"Open source takes the bullshit out of software."
	- Charles Ferguson on TechnologyReview.com

--
Jörn Nettingsmeier, EDV-Administrator
Institut für Politikwissenschaft
Universität Duisburg-Essen, Standort Duisburg
Mail: pol-admin@uni-due.de, Telefon: 0203/379-2736

Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Andreas Hartmann <an...@apache.org>.
Jörn Nettingsmeier wrote:
> Andreas Hartmann wrote:
>> Jörn Nettingsmeier wrote:
>>> as a java exercise, i hacked together an alternate version that uses 
>>> an abstract class AbstractChangePassword that has all the common 
>>> features, and two derived classes ChangePassword and 
>>> ChangePasswordAdmin that each add their own extensions. (i chose  to 
>>> do it this way because each of these classes has features that the 
>>> other hasn't, so there was no obvious way to do it with inheritance.)
>>
>> This sounds reasonable, and at the first glance your patch looks
>> very good. Maybe it would make sense to make
>>
>>   AbstractChangePassword.getUser()
>>
>> abstract. ChangePassword.getUser() would just return the currently
>> logged-in user, and AdminChangePassword() would return the user
>> determined by the userId parameter. WDYT?
> 
> i tried it, but then it seemed better to have a variable
>   private User user
> that defaults to null in AbstractChangePassword, and to set it to the 
> current user in the constuctor of ChangePassword. otherwise the code to 
> determine the current user would be called many times...

That can be solved by lazy loading. But communication between classes
should happen over methods, not fields - even in class hierarchies.


>>> a patch is attached, but it does not work, since i'm still stuck with 
>>> another problem: i want the AbstractChangePassword to initialize 
>>> "user" with the userId of the currently logged in user, but i can't 
>>> seem to find out where to get that kind of information... i tried
>>>   Map objectModel = ContextHelper.getObjectModel(getContext());
>>>   Request request = ObjectModelHelper.getRequest(objectModel);
>>>   this.user = Identity.getIdentity(request.getSession(true)).getUser();
>>> but that gives an npe since getContext returns null.
>>
>> Strange, the same code is used in other usecases ...
>>
>> BTW, an easier way to get the currently logged-in user from a usecase is:
>>
>>   User user = getSession().getIdentity().getUser();
> 
> the same problem. there seems to be a clash:
> 
> compile-src:
> Compiling 2 source files to /build/lenya-1_4_X/build/lenya/api
> /build/lenya-1_4_X/src/java/org/apache/lenya/cms/ac/usecases/ChangePassword.java:32: 
> incompatible types
> found   : org.apache.lenya.cms.repository.Session
> required: org.apache.cocoon.environment.Session
>        Session session = getSession();

You have to use explicit naming (incl. package) for one session object.

>                                    ^
> however, the session interface does not define a getSession method. i'm 
> confused....

You need the o.a.l.cms.repository.Session for this purpose.

-- Andreas


-- 
Andreas Hartmann
Wyona Inc.  -   Open Source Content Management   -   Apache Lenya
http://www.wyona.com                      http://lenya.apache.org
andreas.hartmann@wyona.com                     andreas@apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lenya.apache.org
For additional commands, e-mail: dev-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Jörn Nettingsmeier <po...@uni-duisburg.de>.
Andreas Hartmann wrote:
> Jörn Nettingsmeier wrote:
>> as a java exercise, i hacked together an alternate version that uses 
>> an abstract class AbstractChangePassword that has all the common 
>> features, and two derived classes ChangePassword and 
>> ChangePasswordAdmin that each add their own extensions. (i chose  to 
>> do it this way because each of these classes has features that the 
>> other hasn't, so there was no obvious way to do it with inheritance.)
> 
> This sounds reasonable, and at the first glance your patch looks
> very good. Maybe it would make sense to make
> 
>   AbstractChangePassword.getUser()
> 
> abstract. ChangePassword.getUser() would just return the currently
> logged-in user, and AdminChangePassword() would return the user
> determined by the userId parameter. WDYT?

i tried it, but then it seemed better to have a variable
   private User user
that defaults to null in AbstractChangePassword, and to set it to the 
current user in the constuctor of ChangePassword. otherwise the code to 
determine the current user would be called many times...

>> a patch is attached, but it does not work, since i'm still stuck with 
>> another problem: i want the AbstractChangePassword to initialize 
>> "user" with the userId of the currently logged in user, but i can't 
>> seem to find out where to get that kind of information... i tried
>>   Map objectModel = ContextHelper.getObjectModel(getContext());
>>   Request request = ObjectModelHelper.getRequest(objectModel);
>>   this.user = Identity.getIdentity(request.getSession(true)).getUser();
>> but that gives an npe since getContext returns null.
> 
> Strange, the same code is used in other usecases ...
> 
> BTW, an easier way to get the currently logged-in user from a usecase is:
> 
>   User user = getSession().getIdentity().getUser();

the same problem. there seems to be a clash:

compile-src:
Compiling 2 source files to /build/lenya-1_4_X/build/lenya/api
/build/lenya-1_4_X/src/java/org/apache/lenya/cms/ac/usecases/ChangePassword.java:32: 
incompatible types
found   : org.apache.lenya.cms.repository.Session
required: org.apache.cocoon.environment.Session
        Session session = getSession();
                                    ^
however, the session interface does not define a getSession method. i'm 
confused....

any hints?




-- 
"Open source takes the bullshit out of software."
	- Charles Ferguson on TechnologyReview.com

--
Jörn Nettingsmeier, EDV-Administrator
Institut für Politikwissenschaft
Universität Duisburg-Essen, Standort Duisburg
Mail: pol-admin@uni-due.de, Telefon: 0203/379-2736

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lenya.apache.org
For additional commands, e-mail: dev-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Andreas Hartmann <an...@apache.org>.
Jörn Nettingsmeier wrote:

[...]

> one minor thing remains: the usecase handler for non-admin users still 
> allows to set userId to arbitrary users.
> no security implication here, because they will be prompted for the 
> password of the user accound they try to hijack, but still...

Yes, I would be nice to check for the currently logged-in user.

> as a java exercise, i hacked together an alternate version that uses an 
> abstract class AbstractChangePassword that has all the common features, 
> and two derived classes ChangePassword and ChangePasswordAdmin that each 
> add their own extensions. (i chose  to do it this way because each of 
> these classes has features that the other hasn't, so there was no 
> obvious way to do it with inheritance.)

This sounds reasonable, and at the first glance your patch looks
very good. Maybe it would make sense to make

   AbstractChangePassword.getUser()

abstract. ChangePassword.getUser() would just return the currently
logged-in user, and AdminChangePassword() would return the user
determined by the userId parameter. WDYT?

> 
> could you comment on this? it might be a little over-engineered, but i 
> want to get some hands-on experience regarding oo design.
> 
> a patch is attached, but it does not work, since i'm still stuck with 
> another problem: i want the AbstractChangePassword to initialize "user" 
> with the userId of the currently logged in user, but i can't seem to 
> find out where to get that kind of information... i tried
>   Map objectModel = ContextHelper.getObjectModel(getContext());
>   Request request = ObjectModelHelper.getRequest(objectModel);
>   this.user = Identity.getIdentity(request.getSession(true)).getUser();
> but that gives an npe since getContext returns null.

Strange, the same code is used in other usecases ...

BTW, an easier way to get the currently logged-in user from a usecase is:

   User user = getSession().getIdentity().getUser();


-- Andreas


-- 
Andreas Hartmann
Wyona Inc.  -   Open Source Content Management   -   Apache Lenya
http://www.wyona.com                      http://lenya.apache.org
andreas.hartmann@wyona.com                     andreas@apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lenya.apache.org
For additional commands, e-mail: dev-help@lenya.apache.org


Re: svn commit: r412987 - in /lenya/trunk/src: java/org/apache/lenya/cms/ac/usecases/ webapp/lenya/config/cocoon-xconf/usecases/admin/ webapp/lenya/usecases/admin/

Posted by Jörn Nettingsmeier <po...@uni-duisburg.de>.
hi andreas!

andreas@apache.org wrote:
> Author: andreas
> Date: Fri Jun  9 02:01:58 2006
> New Revision: 412987
> 
> URL: http://svn.apache.org/viewvc?rev=412987&view=rev
> Log:
> Use separate usecase for changing password with check of old password. This avoids the security issue that the checkPassword parameter can be set by the client.

after some testing, this seems to fix the issue i reported for good.


one minor thing remains: the usecase handler for non-admin users still 
allows to set userId to arbitrary users.
no security implication here, because they will be prompted for the 
password of the user accound they try to hijack, but still...

as a java exercise, i hacked together an alternate version that uses an 
abstract class AbstractChangePassword that has all the common features, 
and two derived classes ChangePassword and ChangePasswordAdmin that each 
add their own extensions. (i chose  to do it this way because each of 
these classes has features that the other hasn't, so there was no 
obvious way to do it with inheritance.)

could you comment on this? it might be a little over-engineered, but i 
want to get some hands-on experience regarding oo design.

a patch is attached, but it does not work, since i'm still stuck with 
another problem: i want the AbstractChangePassword to initialize "user" 
with the userId of the currently logged in user, but i can't seem to 
find out where to get that kind of information... i tried
   Map objectModel = ContextHelper.getObjectModel(getContext());
   Request request = ObjectModelHelper.getRequest(objectModel);
   this.user = Identity.getIdentity(request.getSession(true)).getUser();
but that gives an npe since getContext returns null.

can anyone help me here?


tia,


jörn







-- 
"Open source takes the bullshit out of software."
	- Charles Ferguson on TechnologyReview.com

--
Jörn Nettingsmeier, EDV-Administrator
Institut für Politikwissenschaft
Universität Duisburg-Essen, Standort Duisburg
Mail: pol-admin@uni-due.de, Telefon: 0203/379-2736