Re: Possible auth issue in the createPartyAndUserLogin service

Wed, 07 Feb 2007 13:47:27 -0800 

David,
I can actually commit the patch #2 without problem; I'm just worried about the side effects (if any) that this could cause on the existing code. For example, I don't know if the set-service-fields operation is copying also the user login from one map to another one; if this is happening, then (with the existing code) the user login is overriden by the one of the calling service; if I apply my patch this will be no more true... could this cause problems in your opinion? Or am I just too anxious? 

Thanks for your feedback,

Jacopo



David E. Jones wrote:


Sorry for the delay on this Jacopo.


Looking at this I actually like the fix in #2, ie changing the 
simple-method operations because what they do doesn't make sense. In 
other words, if you add a userLogin yourself you wouldn't expect the 
system to blow it away.



If you agree feel free to commit that, your patch of the CallService and 
CallServiceAsynch classes looks just fine. If I don't see your response 
on it I'll throw that in later this evening or tomorrow.


-David


On Feb 7, 2007, at 10:38 AM, Jacopo Cappellato wrote:


Ah,

forgot to mention that I'd vote for solution #1.

Jacopo

Jacopo Cappellato wrote:

Should we take some action about this subject?

I know that probably my explanations were not clear enough, so I'm 
proposing the two ways of fixing what I think it's an outstanding 
issue; I hope that, reviewing the (extremely simple) fixes I'm 
proposing will be easier to understand the issue I'm trying to report.
Solution #1: just fix the service call to "createUserLogin" inside of 
createPersonAndUserLogin by adding the include-user-login="false"; in 
this way the manually "system" user, manually put in the input 
context will be not overriden by the user available in the context of 
the calling service ("createPersonAndUserLogin"); see fix1.patch
Solution #2: fix for the minilang service call methods; if 
include-user-login is not set or it is true, the user login is put in 
the context but *only* if in the context there is not already a user 
login; see fix2.patch

I really think that one of the two solutions should be committed.
What do you think?
Jacopo
Jacopo Cappellato wrote:

David,

I really did not describe it very well, sorry.

I don't think there is a problem with the createUserLogin service 
definition or implementation.
The issue (maybe) could be the way the framework handles the 
minilang attribute include-user-login="true": it just gets the 
"userLogin" object from the context of the calling service and put 
it in the context of the called service, even if in the context of 
the called service there is already a "userLogin" object.
So for example the code in the createPersonAndUserLogin service (for 
which I have provided the patch) is:



<!-- call the service with the system account to get around security 
constraints for this special create -->
<entity-one entity-name="UserLogin" 
value-name="createUlInMap.userLogin">

    <field-map field-name="userLoginId" value="system"/>
</entity-one>


<call-service service-name="createUserLogin" 
in-map-name="createUlInMap" include-user-login="false"/>



Unfotunately the "system" userLogin object that is manually put in 
the input context for the createUserLogin service is overriden (by 
the framework), if one is available in the context of the 
createPersonAndUserLogin service because the include-user-login 
attribute defaults to true.



Just adding the include-user-login="false" attribute to the service 
call fixed the issue.


Does it make sense?

Jacopo



David E. Jones wrote:


On Jan 25, 2007, at 7:41 AM, Jacopo Cappellato wrote:


Hi all,


could you have a look at the attached simple patch? It fixes an 
authorization problem under some special situations (switching 
from anonymous user to authenticated one...); however the issue is 
that, when the "createUserLogin" is called, if the attribute 
include-user-login is not set to false the manually passed in 
"system" user is overwritten by the user in the context.



I just traced through the code to try to figure out why this might 
be happening, and either I don't understand what you're describing, 
or I just can't find why this would happen.



In the implementation of the service 
(LoginServices.java:createUserLogin) I don't see anything that 
would do this. In the service definition I don't see it either 
because the service definition does not have a "userLogin" 
parameter going out.



Have you traced down the code that would be putting the user in the 
context? Also, do you mean by this the context of the current 
service call (ie for other services called by ECA rules, etc), or 
in the result (returned) Map of the service, or somewhere else?



I apologize for pushing on the detail, I may just be too brain-dead 
to see it right now.


-David



Should I commit this patch?

Or, in general, would be better, even if include-user-login is 
true, to set the user login only if one is not already there in 
the service in map?


Jacopo

Index: 
applications/party/script/org/ofbiz/party/party/PartySimpleMethods.xml 


===================================================================

--- 
applications/party/script/org/ofbiz/party/party/PartySimpleMethods.xml    
(revisione 499802)
+++ 
applications/party/script/org/ofbiz/party/party/PartySimpleMethods.xml    
(copia locale)

@@ -85,7 +85,7 @@
             <field-map field-name="userLoginId" value="system"/>
         </entity-one>


-        <call-service service-name="createUserLogin" 
in-map-name="createUlInMap"/>
+        <call-service service-name="createUserLogin" 
in-map-name="createUlInMap" include-user-login="false"/>
         <entity-one entity-name="UserLogin" 
value-name="newUserLogin"/>


         <field-to-result field-name="newUserLogin"/>





------------------------------------------------------------------------

Index: 
applications/party/script/org/ofbiz/party/party/PartySimpleMethods.xml

===================================================================

--- 
applications/party/script/org/ofbiz/party/party/PartySimpleMethods.xml    
(revisione 503593)
+++ 
applications/party/script/org/ofbiz/party/party/PartySimpleMethods.xml    
(copia locale)

@@ -85,7 +85,7 @@
             <field-map field-name="userLoginId" value="system"/>
         </entity-one>

         -        <call-service service-name="createUserLogin" 
in-map-name="createUlInMap"/>
+        <call-service service-name="createUserLogin" 
in-map-name="createUlInMap" include-user-login="false"/>

         <entity-one entity-name="UserLogin" value-name="newUserLogin"/>
          <field-to-result field-name="newUserLogin"/>
------------------------------------------------------------------------

Index: 
framework/minilang/src/org/ofbiz/minilang/method/callops/CallServiceAsynch.java 


===================================================================

--- 
framework/minilang/src/org/ofbiz/minilang/method/callops/CallServiceAsynch.java    
(revisione 503593)
+++ 
framework/minilang/src/org/ofbiz/minilang/method/callops/CallServiceAsynch.java    
(copia locale)

@@ -68,8 +68,9 @@
         if (includeUserLogin) {
             GenericValue userLogin = methodContext.getUserLogin();
 -            if (userLogin != null)
+            if (userLogin != null && inMap.get("userLogin") == null) {
                 inMap.put("userLogin", userLogin);
+            }
         }
                  // always add Locale to context unless null

Index: 
framework/minilang/src/org/ofbiz/minilang/method/callops/CallService.java 


===================================================================

--- 
framework/minilang/src/org/ofbiz/minilang/method/callops/CallService.java    
(revisione 503593)
+++ 
framework/minilang/src/org/ofbiz/minilang/method/callops/CallService.java    
(copia locale)

@@ -213,7 +213,7 @@
         if (includeUserLogin) {
             GenericValue userLogin = methodContext.getUserLogin();
 -            if (userLogin != null) {
+            if (userLogin != null && inMap.get("userLogin") == null) {
                 inMap.put("userLogin", userLogin);
             }
         }






























					Reply via email to