Thank you for the work Jacques. I was hoping as stated earlier that you
share the work before committing it since it is an architectural decision
that requires community consensus.

On Feb 19, 2018 10:29 PM, "Jacques Le Roux" <>



Le 18/02/2018 à 20:33, Jacques Le Roux a écrit :

> Taher,
> I agree using a property is hackish.
> I'll try to implement what you suggest using a keep-autologin-cookie
> webapp attribute which will be false by default and true for the
> applications mentioned below.
> I'll check it make sense for webpos before using true there.
> Thanks
> Jacques
> Le 18/02/2018 à 11:43, Taher Alkhateeb a écrit :
>> Hi Jacques,
>> I don't think your proposed solution works either. It seems you might
>> be missing the underlying problem. There are patterns that I see over
>> and over and I wish we can eliminate them, I will explain the general
>> patterns and then a suggestion for reasonable solution:
>> 1- Wrong dependency direction: Framework should never know about core
>> apps. Core apps should never know about plugins. The arrow is always
>> from the outside to the inside. So when you create a solution, it
>> should always be generic and allow "plugging in" values from the
>> outside to the inside, not pulling or coding by hand
>> 2- Global Shared Mutable State: The two keywords here are "shared" and
>> "mutable". The less we have of both, the better. So no, I don't have a
>> problem with having properties, I have a problem with global mutable
>> variables. They make the system much harder to reason about and debug.
>> So it is always better for a global variable to be immutable, and even
>> better, to not exist. However all systems require some level of global
>> variables but we should use them carefully and try to isolate their
>> impact. Essential things like database connections, tomcat sessions,
>> etc ... are reasonable, but going too far might be a problematic
>> architectural design, not a necessity.
>> So in the example we are discussing here, one idea for a solution
>> would be some kind of configuration (perhaps in ofbiz-component.xml)
>> where you declare whatever settings you want, and then the login
>> worker would simply loop over all components to decide. This is a
>> clean (outside-to-inside) solution instead of this hack job.
>> My recommendation is to revert all of this work, discuss a proper
>> solution in the ML, open a JIRA and provide a patch. I also recommend
>> _not_ committing any architectural changes without discussing them in
>> the community first.
>> On Sun, Feb 18, 2018 at 11:36 AM, Jacques Le Roux
>> <> wrote:
>>> Thanks for the review Taher,
>>> Sorry I completely forgot this thread. When I wrote the
>>> autoLogoutFromAllBackendSessions() method I let a TODO there
>>> // remove all the autoLoginCookies but if in ecommerce/ecomseo and webpos
>>> (it's done manually there, not sure for webpos TODO: check)
>>> and I remember I thought about harcoding some applications was not good.
>>> And
>>> especially, how to let know users, who need this feature, to keep the
>>> autologin cookie in their custom plugins applications.
>>> I know you don't like having too much properties. But to avoid hard
>>> wiring
>>> these applications in the framework, Isuggest to have a new
>>> keep-autologin-cookie security properties, with default OOTB plugins
>>> applications
>>> # -- Names of the component where the autologin cookie should be kept one
>>> year
>>> keep-autologin-cookie=ecommerce,ecomseo,webpos
>>> This way, users who need to keep the autologin cookie just have to add
>>> the
>>> concerned application/s to this list.
>>> And autoLogoutFromAllBackendSessions() can then use this list to avoid
>>> removing autologin cookies from these applications.
>>> Would this work for you?
>>> As I wrote in the TODO, I'm not sure why webpos have a the autoLogout
>>> request-map in its controller. I'm not even sure the webpos really
>>> implements it, like ecommerce does.
>>> So we could remove webpos from the  keep-autologin-cookie list.
>>> I'll check that.
>>> If it's really needed, as the autologin-cookie feature just remembers you
>>> but does not sign you automatically (you need to know the password) I
>>> don't
>>> think it's a security flaw.
>>> Anyway, if it was the only problem with the webpos :/
>>> Jacques
>>> Le 12/02/2018 à 19:48, Taher Alkhateeb a écrit :
>>>> I just checked this code and it looks really worrying to me. You have
>>>> hard wired the ecommerce component with logic into the heart of the
>>>> framework, I think we need to review the entire body of work and maybe
>>>> revert it.
>>>> On Sat, Feb 10, 2018 at 2:38 PM, Jacques Le Roux
>>>> <> wrote:
>>>>> Le 10/02/2018 à 12:33, Jacques Le Roux a écrit :
>>>>>> Hi,
>>>>>> Almost 6 years ago OFBIZ-4959 "Logout do not remove autoLogin" was
>>>>>> created
>>>>>> and I closed as incomplete.
>>>>>> Recently while working on OFBIZ-10206 "Security issue in Token Based
>>>>>> Authentication" which followed my work in OFBIZ-9833 "Token Based
>>>>>> Authentication" I needed a way to get the userLoginId (or userLogin)
>>>>>> from
>>>>>> the session.
>>>>>> But, as explained in OFBIZ-10206, at this stage it was unavailable.
>>>>>> So I
>>>>>> decided to go with autoLoginCookies. I then " remembered" OFBIZ-4959.
>>>>>> So I'd like to commit the patch I provided at OFBIZ-4959. But before
>>>>>> that
>>>>>> I want to discuss about autoLoginCookies and the feature to be sure we
>>>>>> are
>>>>>> all on the same field.
>>>>>> The auto login feature is used in ecommerce applications (ie OOTB
>>>>>> ecommerce and ecomseo) to welcome an user when s/he gets back. It does
>>>>>> not
>>>>>> really log the user in but eases the login process. From the code, the
>>>>>> same
>>>>>> feature exists in the webpos, I did not check.
>>>>>> AutoLoginCookies are also generated for all applications, but are not
>>>>>> used
>>>>>> for the auto login feature like in ecommerce applications. It can be
>>>>>> nevertheless useful as proves OFBIZ-10206 "Security issue in Token
>>>>>> Based
>>>>>> Authentication". But for OFBIZ-10206 and security in general it's
>>>>>> better
>>>>>> to
>>>>>> remove the autoLoginCookies of the other applications (ie no ecommerce
>>>>>> and
>>>>>> webpos) when the user logout. Of course if the user quits the session
>>>>>> w/o
>>>>>> login out the autoLoginCookies remains so it's best to start with a
>>>>>> clean
>>>>>> state and remove the autoLoginCookies at start.
>>>>>> Without negative opinions I'll commit the OFBIZ-4959.patch in 1 week.
>>>>>> Jacques
>>>>>> Forgot to say that the autoLoginCookies have a time to live of 1 year.
>>>>> Jacques

Reply via email to