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" <jacques.le.r...@les7arts.com> 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 >> <jacques.le.r...@les7arts.com> 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 >>>> <jacques.le.r...@les7arts.com> 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 >>>>> >>>>> > >
