[OFBIZ-4959] Logout do not remove autoLogin

[Jacques Le Roux]

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

[Jacques Le Roux]

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

[taher]

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
<[hidden email]> 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
>

[Jacques Le Roux]

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
> <[hidden email]> 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
>>

[taher]

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
<[hidden email]> 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
>> <[hidden email]> 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
>>>
>

[Jacques Le Roux]

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
> <[hidden email]> 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
>>> <[hidden email]> 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
>>>>

[Jacques Le Roux]

Done

Jacques

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
>> <[hidden email]> 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
>>>> <[hidden email]> 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
>>>>>
>
>

[taher]

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" <[hidden email]>
wrote:

Done

Jacques


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
>> <[hidden email]> 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
>>>> <[hidden email]> 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
>>>>>
>>>>>
>
>

[Michael Brohl-3]

+1

We are discussing this over and over again. I wonder what's so difficult
to stick to some basic rules of collaboration.


Am 19.02.18 um 20:48 schrieb Taher Alkhateeb:

> 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" <[hidden email]>
> wrote:
>
> Done
>
> Jacques
>
>
> 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
>>> <[hidden email]> 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
>>>>> <[hidden email]> 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
>>>>>>
>>>>>>
>>

[Jacques Le Roux]

I proposed a 1st patch, we discussed it, it was not OK, I agreed, I implemented the way Taher proposed, we use the RTC, please now review

Thanks

Jacques


Le 19/02/2018 à 20:57, Michael Brohl a écrit :

> +1
>
> We are discussing this over and over again. I wonder what's so difficult to stick to some basic rules of collaboration.
>
>
> Am 19.02.18 um 20:48 schrieb Taher Alkhateeb:
>> 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" <[hidden email]>
>> wrote:
>>
>> Done
>>
>> Jacques
>>
>>
>> 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
>>>> <[hidden email]> 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
>>>>>> <[hidden email]> 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
>>>>>>>
>>>>>>>
>>>
>
>

[taher]

So here is what happened from my view:
- you committed something which impacts design without taking it to the
community
- we objected to the design and suggested to revert and cooperate on a new
design
- instead you went ahead and immediately committed a new design without
asking anyone for reviews

Is this the spirit of how community works? Do we just shove in code and
then tell people if you don't like it change it? I am assuming that we
don't want to turn to strict rules, but rather support the concept of
community and team work. Maybe others have differing views so I leave it to
everyone to express their views here.

On Feb 20, 2018 12:00 AM, "Jacques Le Roux" <[hidden email]>
wrote:

> I proposed a 1st patch, we discussed it, it was not OK, I agreed, I
> implemented the way Taher proposed, we use the RTC, please now review
>
> Thanks
>
> Jacques
>
>
> Le 19/02/2018 à 20:57, Michael Brohl a écrit :
>
>> +1
>>
>> We are discussing this over and over again. I wonder what's so difficult
>> to stick to some basic rules of collaboration.
>>
>>
>> Am 19.02.18 um 20:48 schrieb Taher Alkhateeb:
>>
>>> 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" <
>>> [hidden email]>
>>> wrote:
>>>
>>> Done
>>>
>>> Jacques
>>>
>>>
>>> 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
>>>>> <[hidden email]> 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
>>>>>>> <[hidden email]> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>
>>
>>
>

[Michael Brohl-3]

Hi Taher, all,

we already had several discussions about this pattern and the bad
effects we have to suffer from it. As an example, see [1], [2], there
are others.

Appealing and discussing does not seem to work for a a few people so I
am more and more in favor of a general review-then-commit (RTC) process
for the following types of changes:

- architectural changes

- new features, modules, plugins

- API changes of bigger impact, deprecations

- changes to core functionalities like security, request handling,
login, etc.

Most people by choice follow the process to propose/discuss a change,
provide a patch as an example or base for discussions, work together on
this base until there is consensus among the ones interested and only
then commit their work to the codebase. This works effectively, reduces
noise, the need for reverts or endless discussions about them and so on.

For those who cannot (or don't want to) follow this natural process, we
might need to have a more strict policy. An easy policy is that a commit
in the above mentioned areas of changes needs a proposed patch and at
least 2 independent committers who agree to the changes.

Another policy should be that a commit has to be reverted immediately
when at least 2 independent committers ask the committer to do so. The
arguments  can be discussed after the revert. This prevents delays and
effects like we have with e.g. [3] and is easy to revert again if the
original commit was correct.

Let's see what others think about it.

Regards,

Michael Brohl
ecomify GmbH
www.ecomify.de

[1] https://lists.apache.org/thread.html/0b62f4912bc4a340b0526012bb5e1c120dd7dac496ca9b1395d7e13e@%3Cdev.ofbiz.apache.org%3E

[2]
https://lists.apache.org/thread.html/1cc84ded035247ec439199ae0694d699fe1bcffb5d067c3ae732a0e9@%3Cdev.ofbiz.apache.org%3E

[3] https://issues.apache.org/jira/browse/OFBIZ-9833


Am 21.02.18 um 11:43 schrieb Taher Alkhateeb:

> So here is what happened from my view:
> - you committed something which impacts design without taking it to the
> community
> - we objected to the design and suggested to revert and cooperate on a new
> design
> - instead you went ahead and immediately committed a new design without
> asking anyone for reviews
>
> Is this the spirit of how community works? Do we just shove in code and
> then tell people if you don't like it change it? I am assuming that we
> don't want to turn to strict rules, but rather support the concept of
> community and team work. Maybe others have differing views so I leave it to
> everyone to express their views here.
>
> On Feb 20, 2018 12:00 AM, "Jacques Le Roux" <[hidden email]>
> wrote:
>
>> I proposed a 1st patch, we discussed it, it was not OK, I agreed, I
>> implemented the way Taher proposed, we use the RTC, please now review
>>
>> Thanks
>>
>> Jacques
>>
>>
>> Le 19/02/2018 à 20:57, Michael Brohl a écrit :
>>
>>> +1
>>>
>>> We are discussing this over and over again. I wonder what's so difficult
>>> to stick to some basic rules of collaboration.
>>>
>>>
>>> Am 19.02.18 um 20:48 schrieb Taher Alkhateeb:
>>>
>>>> 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" <
>>>> [hidden email]>
>>>> wrote:
>>>>
>>>> Done
>>>>
>>>> Jacques
>>>>
>>>>
>>>> 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
>>>>>> <[hidden email]> 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
>>>>>>>> <[hidden email]> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>