Hi Jacques, Can look at JWT enhancement later.
+1 for commit. Regards, James On 2020/04/04 13:10:18, Jacques Le Roux <[email protected]> wrote: > Hi James, > > 1. I like the idea. Maybe we could create the class but let the > implementation (with explanations) for those who really need it? > 2. I did not mean there was a correlation between csrf-token check and auth > check. My main idea is to avoid hardcoded things like > > if (requestUri.equals("main")) { return false; } > else { > > We can set that in controllers using csrf-token="false", like it is for > "logout". It's not difficult, just a global S/R. I thought there were other > cases but it seems not. so I now wonder if it's a good idea. > > I don't think there is a need to systematise a default to csrf-token="false" > when auth="false". I just want to work on OFBIZ-4956 and while doing so > check that if we change auth="false" to true, as it implies > csrf-token="true", there will not be undesired side effects. And in other > cases > (auth="false" must remain) we need to decide if should set the CSRF token > check to false. > > Anyway it does not hinder our work on CSRF, but I feel I now miss something > that I wanted to say then, just an intuitive feeling, certainly nothing > serious > > I think we are ready and I'll soon commit... > > Jacques > > Le 29/03/2020 à 03:28, James Yong a écrit : > > Hi Jacques, > > > > For 1, seems like a ICsrfDefenseStrategy class implementation issue. We can > > use another Jira for the enhancement / discussion when this JIRA > > (OFBIZ-11306) is completed. > > > > For 2, csrf-token check is independent of auth check, and the current > > implementation should work as it is. So reviewing whether auth="false" be > > "true", should be in another JIRA (i.e. OFBIZ-4956). If there is a need for > > all auth="false" to default to csrf-token="false", we can implement another > > ICsrfDefenseStrategy class or modify the existing CsrfDefenseStrategy class. > > > > Regards, > > James > > > > On 2020/03/27 18:16:58, Jacques Le Roux<[email protected]> > > wrote: > >> Hi All, > >> > >> Before I create a PR as a last opportunity to allow reviews and tests, I'd > >> like to ask 2 last questions: > >> > >> 1. should we not use a JWT rather than a (pseudo) random value for the > >> CSRF token, this for timeout reason? Don't get me wrong I'm sure that the > >> random values generated by java.security.SecureRandom, as currently > >> used, are safe enough. It's just that I wonder about the timeout. Should > >> we care? > >> 2. In relation with OFBIZ-4956, we need to check the remaining 195 cases > >> where auth="false" and decide if we should change to "true", with the CSRF > >> defense then used by default. In other cases (auth="false" must > >> remain) we need to decide if should set the CSRF token check to false. > >> > >> Apart that > >> myhttps://github.com/JacquesLeRoux/ofbiz-framework/tree/POC-for-CSRF-Token-OFBIZ-11306 > >> branch is ready to create a PR. We can't wait too > >> long about those 2 points, even if the 2nd needs a "bit" of work. Anyway, > >> for now I'll wait answers, and hopefully help for OFBIZ-4956. > >> > >> Thanks > >> > >> Jacques > >> > >> > >> Le 26/03/2020 à 07:39, James Yong a écrit : > >>> +1 with CSRF defense enabled in Demo > >>> > >>>> Hi, > >>>> > >>>> I thought about that a bit more. I suggest to let the stable version > >>>> (soon, R17) as is, ie with CSRF defense enabled. This way users, mostly > >>>> interested in stable, would see the real situation. > >>>> > >>>> And to use the NoCsrfDefenseStrategy in trunk. So developers, often > >>>> brought to use the trunk for development reasons, would have more > >>>> latitude; as > >>>> they certainly will do locally. > >>>> > >>>> If nobody disagree we will do so > >>>> athttps://issues.apache.org/jira/browse/OFBIZ-11472 with Swapnil > >>>> > >>>> If we do so, the > >>>> linkhttps://demo-stable.ofbiz.apache.org/ordermgr/control/main?USERNAME=admin&PASSWORD=ofbiz&JavaScriptEnabled=Y > >>>> will no longer work. > >>>> > >>>> https://demo-stable.ofbiz.apache.org/ordermgr should be used and we > >>>> need to updatehttps://ofbiz.apache.org/ofbiz-demos.html for that. > >>>> > >>>> Jacques > >>>> > >>>> >
