Backport OFBIZ-11317

[Jacques Le Roux]

Hi All,

Even if OFBIZ-11306 does not directly depend upon it, it's safer to have been backported with it.

If nobody disagree, I'll do so in a week

Thanks

Jacques

[Michael Brohl-3]

Hi Jacques,

what exactly are you going to do? And why?

OFBIZ-11317 contains a huge patch and we should be really careful
backporting IMO.

Regards,

Michael Brohl

ecomify GmbH - www.ecomify.de


Am 12.02.20 um 16:08 schrieb Jacques Le Roux:

> Hi All,
>
> Even if OFBIZ-11306 does not directly depend upon it, it's safer to
> have been backported with it.
>
> If nobody disagree, I'll do so in a week
>
> Thanks
>
> Jacques
>

[Jacques Le Roux]

Hi Michael,

I'll backport to R17 and R17 because this will be needed to fix the CSRF vulnerability.

I was not clear with my saying. Actually the CSRF fix (OFBIZ-11316) depends upon OFBIZ-11317 because the CSRF fix uses the ofbizURL macro to set the
CSRF token.

So without the changes in OFBIZ-11317 the ofbizURL macro would not apply to the cases fixed in OFBIZ-11317 and the CSRF vulnerability would not be
fixed there.

So I should not even ask this question, OFBIZ-11316 depends on OFBIZ-11317 so OFBIZ-11317 needs to be backported

I set all that already (as the link between OFBIZ-11316 and OFBIZ-11317shows) but forgot about it.

Case close, thanks to care.

Jacques

Le 12/02/2020 à 16:49, Michael Brohl a écrit :

> Hi Jacques,
>
> what exactly are you going to do? And why?
>
> OFBIZ-11317 contains a huge patch and we should be really careful backporting IMO.
>
> Regards,
>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
>
> Am 12.02.20 um 16:08 schrieb Jacques Le Roux:
>> Hi All,
>>
>> Even if OFBIZ-11306 does not directly depend upon it, it's safer to have been backported with it.
>>
>> If nobody disagree, I'll do so in a week
>>
>> Thanks
>>
>> Jacques
>>
>

[Jacques Le Roux]

To be crystal clear: I'll only do the backport at the "same time" than when we will backport for OFBIZ-11316. It's not needed before. This should be
expected for 17.12.02 version...

Jacques

Le 13/02/2020 à 06:45, Jacques Le Roux a écrit :

> Hi Michael,
>
> I'll backport to R17 and R17 because this will be needed to fix the CSRF vulnerability.
>
> I was not clear with my saying. Actually the CSRF fix (OFBIZ-11316) depends upon OFBIZ-11317 because the CSRF fix uses the ofbizURL macro to set the
> CSRF token.
>
> So without the changes in OFBIZ-11317 the ofbizURL macro would not apply to the cases fixed in OFBIZ-11317 and the CSRF vulnerability would not be
> fixed there.
>
> So I should not even ask this question, OFBIZ-11316 depends on OFBIZ-11317 so OFBIZ-11317 needs to be backported
>
> I set all that already (as the link between OFBIZ-11316 and OFBIZ-11317shows) but forgot about it.
>
> Case close, thanks to care.
>
> Jacques
>
> Le 12/02/2020 à 16:49, Michael Brohl a écrit :
>> Hi Jacques,
>>
>> what exactly are you going to do? And why?
>>
>> OFBIZ-11317 contains a huge patch and we should be really careful backporting IMO.
>>
>> Regards,
>>
>> Michael Brohl
>>
>> ecomify GmbH - www.ecomify.de
>>
>>
>> Am 12.02.20 um 16:08 schrieb Jacques Le Roux:
>>> Hi All,
>>>
>>> Even if OFBIZ-11306 does not directly depend upon it, it's safer to have been backported with it.
>>>
>>> If nobody disagree, I'll do so in a week
>>>
>>> Thanks
>>>
>>> Jacques
>>>
>>

[Michael Brohl-3]

Jacques,

as I said, this is a huge patch which spreads over many functionalies in
the codebase.

It was submitted yesterday and got committed on the same day without
enough time for others to review and test. You even acknowledged that
you did not test.

How can this be considered as a valid base for a security fix without
in-depth testing?


Michael Brohl

ecomify GmbH - www.ecomify.de


Am 13.02.20 um 06:45 schrieb Jacques Le Roux:

> Hi Michael,
>
> I'll backport to R17 and R17 because this will be needed to fix the
> CSRF vulnerability.
>
> I was not clear with my saying. Actually the CSRF fix (OFBIZ-11316)
> depends upon OFBIZ-11317 because the CSRF fix uses the ofbizURL macro
> to set the CSRF token.
>
> So without the changes in OFBIZ-11317 the ofbizURL macro would not
> apply to the cases fixed in OFBIZ-11317 and the CSRF vulnerability
> would not be fixed there.
>
> So I should not even ask this question, OFBIZ-11316 depends on
> OFBIZ-11317 so OFBIZ-11317 needs to be backported
>
> I set all that already (as the link between OFBIZ-11316 and
> OFBIZ-11317shows) but forgot about it.
>
> Case close, thanks to care.
>
> Jacques
>
> Le 12/02/2020 à 16:49, Michael Brohl a écrit :
>> Hi Jacques,
>>
>> what exactly are you going to do? And why?
>>
>> OFBIZ-11317 contains a huge patch and we should be really careful
>> backporting IMO.
>>
>> Regards,
>>
>> Michael Brohl
>>
>> ecomify GmbH - www.ecomify.de
>>
>>
>> Am 12.02.20 um 16:08 schrieb Jacques Le Roux:
>>> Hi All,
>>>
>>> Even if OFBIZ-11306 does not directly depend upon it, it's safer to
>>> have been backported with it.
>>>
>>> If nobody disagree, I'll do so in a week
>>>
>>> Thanks
>>>
>>> Jacques
>>>
>>

[Jacques Le Roux]

Michael,

Inline...

Le 13/02/2020 à 07:58, Michael Brohl a écrit :
> Jacques,
>
> as I said, this is a huge patch which spreads over many functionalies in the codebase.
>
> It was submitted yesterday and got committed on the same day without enough time for others to review and test.

You confuse, the commit you speak about was only to complete one missing instance, spotted by Pierre Smits, in the commit done one month ago.

Since then James and I work on OFBIZ-11306 (and not OFBIZ-11316 as written below) without any issues related to OFBIZ-11317 on which OFBIZ-11306
depends upon.

Actually we are working on it for 2 months. Only one month ago I suggested to James to extract this part.


> You even acknowledged that you did not test.

Of course I test, everyday for a month with OFBIZ-11306


>
> How can this be considered as a valid base for a security fix without in-depth testing?

I think you got it answered

Jacques

>
>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
>
> Am 13.02.20 um 06:45 schrieb Jacques Le Roux:
>> Hi Michael,
>>
>> I'll backport to R17 and R17 because this will be needed to fix the CSRF vulnerability.
>>
>> I was not clear with my saying. Actually the CSRF fix (OFBIZ-11316) depends upon OFBIZ-11317 because the CSRF fix uses the ofbizURL macro to set
>> the CSRF token.
>>
>> So without the changes in OFBIZ-11317 the ofbizURL macro would not apply to the cases fixed in OFBIZ-11317 and the CSRF vulnerability would not be
>> fixed there.
>>
>> So I should not even ask this question, OFBIZ-11316 depends on OFBIZ-11317 so OFBIZ-11317 needs to be backported
>>
>> I set all that already (as the link between OFBIZ-11316 and OFBIZ-11317shows) but forgot about it.
>>
>> Case close, thanks to care.
>>
>> Jacques
>>
>> Le 12/02/2020 à 16:49, Michael Brohl a écrit :
>>> Hi Jacques,
>>>
>>> what exactly are you going to do? And why?
>>>
>>> OFBIZ-11317 contains a huge patch and we should be really careful backporting IMO.
>>>
>>> Regards,
>>>
>>> Michael Brohl
>>>
>>> ecomify GmbH - www.ecomify.de
>>>
>>>
>>> Am 12.02.20 um 16:08 schrieb Jacques Le Roux:
>>>> Hi All,
>>>>
>>>> Even if OFBIZ-11306 does not directly depend upon it, it's safer to have been backported with it.
>>>>
>>>> If nobody disagree, I'll do so in a week
>>>>
>>>> Thanks
>>>>
>>>> Jacques
>>>>
>>>
>

[Pierre Smits-3]

OFBIZ-11317 is NOT a huge commit. It is nothing more than a removal of a
hard-coded path in 66 files spread over 4 commits. With impact, as the
stated in the ticket, classified as minor. The code changes have been
tested by the project's CI, since incorporated into the code base and have
not led to breaking the code.

Best regards,

Pierre Smits
*Proud* *contributor* (but without privileges)* of* Apache OFBiz
<https://ofbiz.apache.org/>, since 2008

*Apache Trafodion <https://trafodion.apache.org>, Vice President*
*Apache Directory <https://directory.apache.org>, PMC Member*
Apache Incubator <https://incubator.apache.org>, committer
Apache Steve <https://steve.apache.org>, committer


On Thu, Feb 13, 2020 at 8:51 AM Jacques Le Roux <
[hidden email]> wrote:

> Michael,
>
> Inline...
>
> Le 13/02/2020 à 07:58, Michael Brohl a écrit :
> > Jacques,
> >
> > as I said, this is a huge patch which spreads over many functionalies in
> the codebase.
> >
> > It was submitted yesterday and got committed on the same day without
> enough time for others to review and test.
>
> You confuse, the commit you speak about was only to complete one missing
> instance, spotted by Pierre Smits, in the commit done one month ago.
>
> Since then James and I work on OFBIZ-11306 (and not OFBIZ-11316 as written
> below) without any issues related to OFBIZ-11317 on which OFBIZ-11306
> depends upon.
>
> Actually we are working on it for 2 months. Only one month ago I suggested
> to James to extract this part.
>
>
> > You even acknowledged that you did not test.
>
> Of course I test, everyday for a month with OFBIZ-11306
>
>
> >
> > How can this be considered as a valid base for a security fix without
> in-depth testing?
>
> I think you got it answered
>
> Jacques
>
> >
> >
> > Michael Brohl
> >
> > ecomify GmbH - www.ecomify.de
> >
> >
> > Am 13.02.20 um 06:45 schrieb Jacques Le Roux:
> >> Hi Michael,
> >>
> >> I'll backport to R17 and R17 because this will be needed to fix the
> CSRF vulnerability.
> >>
> >> I was not clear with my saying. Actually the CSRF fix (OFBIZ-11316)
> depends upon OFBIZ-11317 because the CSRF fix uses the ofbizURL macro to
> set
> >> the CSRF token.
> >>
> >> So without the changes in OFBIZ-11317 the ofbizURL macro would not
> apply to the cases fixed in OFBIZ-11317 and the CSRF vulnerability would
> not be
> >> fixed there.
> >>
> >> So I should not even ask this question, OFBIZ-11316 depends on
> OFBIZ-11317 so OFBIZ-11317 needs to be backported
> >>
> >> I set all that already (as the link between OFBIZ-11316 and
> OFBIZ-11317shows) but forgot about it.
> >>
> >> Case close, thanks to care.
> >>
> >> Jacques
> >>
> >> Le 12/02/2020 à 16:49, Michael Brohl a écrit :
> >>> Hi Jacques,
> >>>
> >>> what exactly are you going to do? And why?
> >>>
> >>> OFBIZ-11317 contains a huge patch and we should be really careful
> backporting IMO.
> >>>
> >>> Regards,
> >>>
> >>> Michael Brohl
> >>>
> >>> ecomify GmbH - www.ecomify.de
> >>>
> >>>
> >>> Am 12.02.20 um 16:08 schrieb Jacques Le Roux:
> >>>> Hi All,
> >>>>
> >>>> Even if OFBIZ-11306 does not directly depend upon it, it's safer to
> have been backported with it.
> >>>>
> >>>> If nobody disagree, I'll do so in a week
> >>>>
> >>>> Thanks
> >>>>
> >>>> Jacques
> >>>>
> >>>
> >
>

[Michael Brohl-3]

Hi Jacques,

also inline...

Am 13.02.20 um 08:50 schrieb Jacques Le Roux:

> Jacques,
>>
>> as I said, this is a huge patch which spreads over many functionalies
>> in the codebase.
>>
>> It was submitted yesterday and got committed on the same day without
>> enough time for others to review and test.
>
> You confuse, the commit you speak about was only to complete one
> missing instance, spotted by Pierre Smits, in the commit done one
> month ago.

Yes, I confused the date (Jan vs. Feb, time goes by too quick).

I speak of the commits towards
https://issues.apache.org/jira/browse/OFBIZ-11317. The issue was created
and on the same day it was committed. It was not yesterday but the
timeline between submit and commit is the same.

>
> Since then James and I work on OFBIZ-11306 (and not OFBIZ-11316 as
> written below) without any issues related to OFBIZ-11317 on which
> OFBIZ-11306 depends upon.
>
> Actually we are working on it for 2 months. Only one month ago I
> suggested to James to extract this part.
>
>
>> You even acknowledged that you did not test.
>
> Of course I test, everyday for a month with OFBIZ-11306

I was refering to
https://issues.apache.org/jira/browse/OFBIZ-11317?focusedCommentId=17013724&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17013724 


Together with the above confusion it raised an alarm for me.


>>
>> How can this be considered as a valid base for a security fix without
>> in-depth testing?
>
> I think you got it answered

Thank you Jacques. I did not mean to question the work in general, just
being sensible to quick commits. I already layed out my motives in other
dev threads.

I suggest to provide the OFBIZ-11306 patch once you and James think you
are finished for others to review.


I'll have some questions towards OFBIZ-11317 also but I need time to dig
deeper.

Thanks,

Michael Brohl

ecomify GmbH - www.ecomify.de

[Michael Brohl-3]

Hi Pierre,

inline...


Am 13.02.20 um 09:04 schrieb Pierre Smits:
> OFBIZ-11317 is NOT a huge commit. It is nothing more than a removal of a
> hard-coded path in 66 files spread over 4 commits. With impact, as the

The paths are still hard-coded, the hard-coded part is just moved to a
macro parameter. Just to be precise.


> stated in the ticket, classified as minor. The code changes have been
> tested by the project's CI, since incorporated into the code base and have
> not led to breaking the code.

The changes are of a type the CI cannot detect automatically, at least I
don't know of a way how the ofbizUrl macro changes are testet. The
commit does not contain additional tests.

But those details are not my main point, see my mail to Jacques.

I will have a closer review when I find time.

Thanks,

Michael Brohl

ecomify GmbH - www.ecomify.de

[Pierre Smits-3]

I agree: with current CI tooling and its setup up it can't be detected. If
the project want that changed (automatic testing of PRs before they go into
the master branche(s)) then it needs to step up and change the CI
configuration and/or tooling.

Best regards,

Pierre Smits
*Proud* *contributor* (but without privileges)* of* Apache OFBiz
<https://ofbiz.apache.org/>, since 2008

*Apache Trafodion <https://trafodion.apache.org>, Vice President*
*Apache Directory <https://directory.apache.org>, PMC Member*
Apache Incubator <https://incubator.apache.org>, committer
Apache Steve <https://steve.apache.org>, committer


On Thu, Feb 13, 2020 at 9:35 AM Michael Brohl <[hidden email]>
wrote:

> Hi Pierre,
>
> inline...
>
>
> Am 13.02.20 um 09:04 schrieb Pierre Smits:
> > OFBIZ-11317 is NOT a huge commit. It is nothing more than a removal of a
> > hard-coded path in 66 files spread over 4 commits. With impact, as the
>
> The paths are still hard-coded, the hard-coded part is just moved to a
> macro parameter. Just to be precise.
>
>
> > stated in the ticket, classified as minor. The code changes have been
> > tested by the project's CI, since incorporated into the code base and
> have
> > not led to breaking the code.
>
> The changes are of a type the CI cannot detect automatically, at least I
> don't know of a way how the ofbizUrl macro changes are testet. The
> commit does not contain additional tests.
>
> But those details are not my main point, see my mail to Jacques.
>
> I will have a closer review when I find time.
>
> Thanks,
>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
>
>
>

[Jacques Le Roux]

Le 13/02/2020 à 09:31, Michael Brohl a écrit :

> Hi Jacques,
>
> also inline...
>
> Am 13.02.20 um 08:50 schrieb Jacques Le Roux:
>> Jacques,
>>>
>>> as I said, this is a huge patch which spreads over many functionalies in the codebase.
>>>
>>> It was submitted yesterday and got committed on the same day without enough time for others to review and test.
>>
>> You confuse, the commit you speak about was only to complete one missing instance, spotted by Pierre Smits, in the commit done one month ago.
>
>
> Yes, I confused the date (Jan vs. Feb, time goes by too quick).
>
> I speak of the commits towards https://issues.apache.org/jira/browse/OFBIZ-11317. The issue was created and on the same day it was committed. It was
> not yesterday but the timeline between submit and commit is the same.

I don't want to argue too much about that, so I hope it will be the end of this exchange. You are right about the Jira and commit moment, they are same.

But I see 2 points here:

  * It's something you can review in ½ a hour, if not even the "famous" 10 minutes. You can even use a regexp to help you...
  * These changes were present exactly the same for a month in OFBIZ-11306

So I did not and still don't see any reasons to delay the commit (the backports were straightforward). We can't wait for everybody to valid a such
simple commit. For such commits CTR[1] is appropriate, and so far we use CTR.

[1] https://www.apache.org/foundation/glossary.html#CommitThenReview


>>>
>>> How can this be considered as a valid base for a security fix without in-depth testing?
>>
>> I think you got it answered
>
> Thank you Jacques. I did not mean to question the work in general, just being sensible to quick commits. I already layed out my motives in other dev
> threads.

Yes, sometimes we must be careful. It's not the case here, and there will other such cases. We don't need to rehearse the same tune everytime, else
the project will be really stale ;)


>
> I suggest to provide the OFBIZ-11306 patch once you and James think you are finished for others to review.

Sure, that's what we have planned. We will need all available eyeballs to review and hands to test! This said don't refrain (not particularly you
Michael) to begin to review and as the work is going on. There are still some edges to smooth but it's pretty stable now.


>
> I'll have some questions towards OFBIZ-11317 also but I need time to dig deeper.

Sure, shoot :)

Jacques

>
> Thanks,
>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
>
>

[Jacques Le Roux]

Le 13/02/2020 à 09:35, Michael Brohl a écrit :
> The paths are still hard-coded, the hard-coded part is just moved to a macro parameter. Just to be precise.

See it's simple ;)

I can't see anything complex there. Are there other things which make you worry?

Jacques

[Michael Brohl-3]

Hi Jacques,

inline...

Am 13.02.20 um 11:43 schrieb Jacques Le Roux:

>>
>> Yes, I confused the date (Jan vs. Feb, time goes by too quick).
>>
>> I speak of the commits towards
>> https://issues.apache.org/jira/browse/OFBIZ-11317. The issue was
>> created and on the same day it was committed. It was not yesterday
>> but the timeline between submit and commit is the same.
>
> I don't want to argue too much about that, so I hope it will be the
> end of this exchange. You are right about the Jira and commit moment,
> they are same.
>
> But I see 2 points here:
>
>  * It's something you can review in ½ a hour, if not even the "famous"
> 10 minutes. You can even use a regexp to help you...

It's a new feature and IMO the community should have a chance to review
and decide if a feature should go into the codebase.

A quick commit on the same day simply removes this chance and builds the
impression that the committer does not care about what others think. IMO
that's not good for community work.

But I'm getting tired of trying to get this spirit transported somehow.
Especially when it seems that nobody else really cares about this
approach. So we can end this exchange, like you hoped.

>
>>
>> I'll have some questions towards OFBIZ-11317 also but I need time to
>> dig deeper.
>
> Sure, shoot :)


There's currently only few questions left for me: we have one
configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform
for ofbizUrl and this would not support the controlPath configuration if
I don't miss something. Wouldn't it be better to change this to
UrlRegexpTransform and deprecate OfbizUrlTransform?

UrlRegexpTransform is located inside the product component but the macro
is used in several other components as well. I think it belongs to the
framework/webapp instead of applications/product. What do you think?

Thanks,

Michael Brohl

ecomify GmbH - www.ecomify.de

[Pierre Smits-3]

OFBIZ-11317 is NOT a new feature. It is an improvement.

If you want to discuss the implications (and impact), of 11306 I suggest to
start a new thread in this ml (or voice your concerns in comment postings
in the appropriate ticket).

As for

There's currently only few questions left for me: we have one
configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform
for ofbizUrl and this would not support the controlPath configuration if
I don't miss something. Wouldn't it be better to change this to
UrlRegexpTransform and deprecate OfbizUrlTransform?

UrlRegexpTransform is located inside the product component but the macro
is used in several other components as well. I think it belongs to the
framework/webapp instead of applications/product. What do you think?


I also suggest to start either a new thread in this ml, or open a ticket in
JIRA.

Best regards,

Pierre Smits
*Proud* *contributor* (but without privileges)* of* Apache OFBiz
<https://ofbiz.apache.org/>, since 2008

*Apache Trafodion <https://trafodion.apache.org>, Vice President*
*Apache Directory <https://directory.apache.org>, PMC Member*
Apache Incubator <https://incubator.apache.org>, committer
Apache Steve <https://steve.apache.org>, committer


On Thu, Feb 13, 2020 at 1:06 PM Michael Brohl <[hidden email]>
wrote:

> Hi Jacques,
>
> inline...
>
> Am 13.02.20 um 11:43 schrieb Jacques Le Roux:
> >>
> >> Yes, I confused the date (Jan vs. Feb, time goes by too quick).
> >>
> >> I speak of the commits towards
> >> https://issues.apache.org/jira/browse/OFBIZ-11317. The issue was
> >> created and on the same day it was committed. It was not yesterday
> >> but the timeline between submit and commit is the same.
> >
> > I don't want to argue too much about that, so I hope it will be the
> > end of this exchange. You are right about the Jira and commit moment,
> > they are same.
> >
> > But I see 2 points here:
> >
> >  * It's something you can review in ½ a hour, if not even the "famous"
> > 10 minutes. You can even use a regexp to help you...
>
> It's a new feature and IMO the community should have a chance to review
> and decide if a feature should go into the codebase.
>
> A quick commit on the same day simply removes this chance and builds the
> impression that the committer does not care about what others think. IMO
> that's not good for community work.
>
> But I'm getting tired of trying to get this spirit transported somehow.
> Especially when it seems that nobody else really cares about this
> approach. So we can end this exchange, like you hoped.
>
> >
> >>
> >> I'll have some questions towards OFBIZ-11317 also but I need time to
> >> dig deeper.
> >
> > Sure, shoot :)
>
>
> There's currently only few questions left for me: we have one
> configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform
> for ofbizUrl and this would not support the controlPath configuration if
> I don't miss something. Wouldn't it be better to change this to
> UrlRegexpTransform and deprecate OfbizUrlTransform?
>
> UrlRegexpTransform is located inside the product component but the macro
> is used in several other components as well. I think it belongs to the
> framework/webapp instead of applications/product. What do you think?
>
> Thanks,
>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
>
>
>
>

[Jacques Le Roux]

Le 13/02/2020 à 13:06, Michael Brohl a écrit :
> There's currently only few questions left for me: we have one configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for ofbizUrl
> and this would not support the controlPath configuration if I don't miss something. Wouldn't it be better to change this to UrlRegexpTransform and
> deprecate OfbizUrlTransform?
>
> UrlRegexpTransform is located inside the product component but the macro is used in several other components as well. I think it belongs to the
> framework/webapp instead of applications/product. What do you think?

Hi Michael,

I'm not sure to understand. Are you speaking about OFBiz OOTB?

Because the changes in OFBIZ-11317 are only related to ofbizUrl, hence  org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course ofbizUrl supports
the controlPath configuration. Also why would you want to replace OfbizUrlTransform by UrlRegexpTransform?

Jacques

[Michael Brohl-3]

Please have a closer look at your commit, the controlPath functionality
is implemented in the UrlRegexpTransform class...

The implementing class for ofbizUrl is configured in the
freemarkerTransforms.properties which is OfbizurlTransform for
framework/webapp but UrlRegexpTransform for applications/product.

This seems inconsistent to me.

Thanks,

Michael Brohl

ecomify GmbH - www.ecomify.de


Am 14.02.20 um 14:24 schrieb Jacques Le Roux:

> Le 13/02/2020 à 13:06, Michael Brohl a écrit :
>> There's currently only few questions left for me: we have one
>> configuration which uses
>> org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for ofbizUrl and this
>> would not support the controlPath configuration if I don't miss
>> something. Wouldn't it be better to change this to UrlRegexpTransform
>> and deprecate OfbizUrlTransform?
>>
>> UrlRegexpTransform is located inside the product component but the
>> macro is used in several other components as well. I think it belongs
>> to the framework/webapp instead of applications/product. What do you
>> think?
>
> Hi Michael,
>
> I'm not sure to understand. Are you speaking about OFBiz OOTB?
>
> Because the changes in OFBIZ-11317 are only related to ofbizUrl,
> hence  org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course
> ofbizUrl supports the controlPath configuration. Also why would you
> want to replace OfbizUrlTransform by UrlRegexpTransform?
>
> Jacques
>

[Jacques Le Roux]

Hi Michael,

It's a long answer, but I think it's worth it.

1St, technically it's not my commit but James's ;)

But I asked you, and reviewed/agreed with James's commit, so here we go.

Your intuition was right, there is a problem with this patch. In some place, like at least themes/tomahawk/template/AppBarClose.ftl the CSRF token is
not generated. Because in those places OfbizUrlTransform is used.

An immediate solution is to add the CSRF token generation in OfbizUrlTransform class. I just did so at OFBIZ-11317

But I agree that it's not satisfactory and your proposition to deprecate OfbizUrlTransform in favour of UrlRegexpTransform makes sense to me. That's
mostly why I asked you a second time.

Let's go a bit in the past. With  OFBIZ-5312, which was a major change started in 2013, we notably introduced UrlRegexpTransform.
We used a Svn feature branch for that: http://svn.apache.org/viewvc?view=revision&revision=r1535432

It's unrelated, but while at it since it was quite unfortunate, a conflict between Anil and Paul Piper (I was then working with Paul) started and I
had to find a solution which ended with ecomseo.
Fortunately OFBiz is flexible enough and eventually ecomseo is now a simple clone of the ecommerce webapp (like exampleext is for the example webapp).
Everyone can pick her/his preferred one :).

Now about OfbizUrlTransform vs UrlRegexpTransform: after OFBIZ-5312 UrlRegexpTransform was the only one used for ofbizUrl macro.
That stopped for good reason (Framework dependency on product component) in 2018 with Deepak's work on OFBIZ-10463.
It also shows that during this period (3 years) nobody encountered and issue with UrlRegexpTransform used to generate ofbizUrl macro.

So yes, we should definitely deprecate OfbizUrlTransform in favour of UrlRegexpTransform. But before that we need to be sure that UrlRegexpTransform
would not miss a feature of OfbizUrlTransform.
With OFBIZ-11229, Nicolas already started to merge UrlRegexpTransform and OfbizUrlTransform classes. This issue  is still open and we should use it to
reach our goal.
Maybe it's already OK and we have just to check. For that I'll soon attach a diff of the 2 classes at OFBIZ-11229

Thanks to care!

I wish a nice weekend to all of you.

Jacques

Le 14/02/2020 à 14:47, Michael Brohl a écrit :

> Please have a closer look at your commit, the controlPath functionality is implemented in the UrlRegexpTransform class...
>
> The implementing class for ofbizUrl is configured in the freemarkerTransforms.properties which is OfbizurlTransform for framework/webapp but
> UrlRegexpTransform for applications/product.
>
> This seems inconsistent to me.
>
> Thanks,
>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
>
> Am 14.02.20 um 14:24 schrieb Jacques Le Roux:
>> Le 13/02/2020 à 13:06, Michael Brohl a écrit :
>>> There's currently only few questions left for me: we have one configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for ofbizUrl
>>> and this would not support the controlPath configuration if I don't miss something. Wouldn't it be better to change this to UrlRegexpTransform and
>>> deprecate OfbizUrlTransform?
>>>
>>> UrlRegexpTransform is located inside the product component but the macro is used in several other components as well. I think it belongs to the
>>> framework/webapp instead of applications/product. What do you think?
>>
>> Hi Michael,
>>
>> I'm not sure to understand. Are you speaking about OFBiz OOTB?
>>
>> Because the changes in OFBIZ-11317 are only related to ofbizUrl, hence  org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course ofbizUrl
>> supports the controlPath configuration. Also why would you want to replace OfbizUrlTransform by UrlRegexpTransform?
>>
>> Jacques
>>
>

[Jacques Le Roux]

BTW, I'm not sure it's mandatory, but there are maybe some cases where the same (CSRF token generation) should be done for ofbizContentUrl

Jacques

Le 15/02/2020 à 13:27, Jacques Le Roux a écrit :

> Hi Michael,
>
> It's a long answer, but I think it's worth it.
>
> 1St, technically it's not my commit but James's ;)
>
> But I asked you, and reviewed/agreed with James's commit, so here we go.
>
> Your intuition was right, there is a problem with this patch. In some place, like at least themes/tomahawk/template/AppBarClose.ftl the CSRF token
> is not generated. Because in those places OfbizUrlTransform is used.
>
> An immediate solution is to add the CSRF token generation in OfbizUrlTransform class. I just did so at OFBIZ-11317
>
> But I agree that it's not satisfactory and your proposition to deprecate OfbizUrlTransform in favour of UrlRegexpTransform makes sense to me. That's
> mostly why I asked you a second time.
>
> Let's go a bit in the past. With  OFBIZ-5312, which was a major change started in 2013, we notably introduced UrlRegexpTransform.
> We used a Svn feature branch for that: http://svn.apache.org/viewvc?view=revision&revision=r1535432
>
> It's unrelated, but while at it since it was quite unfortunate, a conflict between Anil and Paul Piper (I was then working with Paul) started and I
> had to find a solution which ended with ecomseo.
> Fortunately OFBiz is flexible enough and eventually ecomseo is now a simple clone of the ecommerce webapp (like exampleext is for the example
> webapp). Everyone can pick her/his preferred one :).
>
> Now about OfbizUrlTransform vs UrlRegexpTransform: after OFBIZ-5312 UrlRegexpTransform was the only one used for ofbizUrl macro.
> That stopped for good reason (Framework dependency on product component) in 2018 with Deepak's work on OFBIZ-10463.
> It also shows that during this period (3 years) nobody encountered and issue with UrlRegexpTransform used to generate ofbizUrl macro.
>
> So yes, we should definitely deprecate OfbizUrlTransform in favour of UrlRegexpTransform. But before that we need to be sure that UrlRegexpTransform
> would not miss a feature of OfbizUrlTransform.
> With OFBIZ-11229, Nicolas already started to merge UrlRegexpTransform and OfbizUrlTransform classes. This issue  is still open and we should use it
> to reach our goal.
> Maybe it's already OK and we have just to check. For that I'll soon attach a diff of the 2 classes at OFBIZ-11229
>
> Thanks to care!
>
> I wish a nice weekend to all of you.
>
> Jacques
>
> Le 14/02/2020 à 14:47, Michael Brohl a écrit :
>> Please have a closer look at your commit, the controlPath functionality is implemented in the UrlRegexpTransform class...
>>
>> The implementing class for ofbizUrl is configured in the freemarkerTransforms.properties which is OfbizurlTransform for framework/webapp but
>> UrlRegexpTransform for applications/product.
>>
>> This seems inconsistent to me.
>>
>> Thanks,
>>
>> Michael Brohl
>>
>> ecomify GmbH - www.ecomify.de
>>
>>
>> Am 14.02.20 um 14:24 schrieb Jacques Le Roux:
>>> Le 13/02/2020 à 13:06, Michael Brohl a écrit :
>>>> There's currently only few questions left for me: we have one configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for ofbizUrl
>>>> and this would not support the controlPath configuration if I don't miss something. Wouldn't it be better to change this to UrlRegexpTransform
>>>> and deprecate OfbizUrlTransform?
>>>>
>>>> UrlRegexpTransform is located inside the product component but the macro is used in several other components as well. I think it belongs to the
>>>> framework/webapp instead of applications/product. What do you think?
>>>
>>> Hi Michael,
>>>
>>> I'm not sure to understand. Are you speaking about OFBiz OOTB?
>>>
>>> Because the changes in OFBIZ-11317 are only related to ofbizUrl, hence org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course ofbizUrl
>>> supports the controlPath configuration. Also why would you want to replace OfbizUrlTransform by UrlRegexpTransform?
>>>
>>> Jacques
>>>
>>

[Michael Brohl-3]

Hi Jacques,

thanks for confirming my concerns.

This is why I am advocating for longer review periods and RTC over CTR
for new features/improvements, especially in the core functionality with
potential to break something. More eyes help to detect potential
problems and flaws, they just need some time.

I will try to provide a patch for a proper deprecated OfbizUrlTransform
class (delegating everything to UrlRegexpTransform and with annotations)
which we will need for a proper backport where functional changes should
not be implemented. User implementations might rely on the
OfbizUrlTransform implementation.

I've assigned https://issues.apache.org/jira/browse/OFBIZ-11229 to me
for it.

Thanks,

Michael Brohl

ecomify GmbH - www.ecomify.de


Am 15.02.20 um 13:27 schrieb Jacques Le Roux:

> Hi Michael,
>
> It's a long answer, but I think it's worth it.
>
> 1St, technically it's not my commit but James's ;)
>
> But I asked you, and reviewed/agreed with James's commit, so here we go.
>
> Your intuition was right, there is a problem with this patch. In some
> place, like at least themes/tomahawk/template/AppBarClose.ftl the CSRF
> token is not generated. Because in those places OfbizUrlTransform is
> used.
>
> An immediate solution is to add the CSRF token generation in
> OfbizUrlTransform class. I just did so at OFBIZ-11317
>
> But I agree that it's not satisfactory and your proposition to
> deprecate OfbizUrlTransform in favour of UrlRegexpTransform makes
> sense to me. That's mostly why I asked you a second time.
>
> Let's go a bit in the past. With  OFBIZ-5312, which was a major change
> started in 2013, we notably introduced UrlRegexpTransform.
> We used a Svn feature branch for that:
> http://svn.apache.org/viewvc?view=revision&revision=r1535432
>
> It's unrelated, but while at it since it was quite unfortunate, a
> conflict between Anil and Paul Piper (I was then working with Paul)
> started and I had to find a solution which ended with ecomseo.
> Fortunately OFBiz is flexible enough and eventually ecomseo is now a
> simple clone of the ecommerce webapp (like exampleext is for the
> example webapp). Everyone can pick her/his preferred one :).
>
> Now about OfbizUrlTransform vs UrlRegexpTransform: after OFBIZ-5312
> UrlRegexpTransform was the only one used for ofbizUrl macro.
> That stopped for good reason (Framework dependency on product
> component) in 2018 with Deepak's work on OFBIZ-10463.
> It also shows that during this period (3 years) nobody encountered and
> issue with UrlRegexpTransform used to generate ofbizUrl macro.
>
> So yes, we should definitely deprecate OfbizUrlTransform in favour of
> UrlRegexpTransform. But before that we need to be sure that
> UrlRegexpTransform would not miss a feature of OfbizUrlTransform.
> With OFBIZ-11229, Nicolas already started to merge UrlRegexpTransform
> and OfbizUrlTransform classes. This issue  is still open and we should
> use it to reach our goal.
> Maybe it's already OK and we have just to check. For that I'll soon
> attach a diff of the 2 classes at OFBIZ-11229
>
> Thanks to care!
>
> I wish a nice weekend to all of you.
>
> Jacques
>
> Le 14/02/2020 à 14:47, Michael Brohl a écrit :
>> Please have a closer look at your commit, the controlPath
>> functionality is implemented in the UrlRegexpTransform class...
>>
>> The implementing class for ofbizUrl is configured in the
>> freemarkerTransforms.properties which is OfbizurlTransform for
>> framework/webapp but UrlRegexpTransform for applications/product.
>>
>> This seems inconsistent to me.
>>
>> Thanks,
>>
>> Michael Brohl
>>
>> ecomify GmbH - www.ecomify.de
>>
>>
>> Am 14.02.20 um 14:24 schrieb Jacques Le Roux:
>>> Le 13/02/2020 à 13:06, Michael Brohl a écrit :
>>>> There's currently only few questions left for me: we have one
>>>> configuration which uses
>>>> org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for ofbizUrl and this
>>>> would not support the controlPath configuration if I don't miss
>>>> something. Wouldn't it be better to change this to
>>>> UrlRegexpTransform and deprecate OfbizUrlTransform?
>>>>
>>>> UrlRegexpTransform is located inside the product component but the
>>>> macro is used in several other components as well. I think it
>>>> belongs to the framework/webapp instead of applications/product.
>>>> What do you think?
>>>
>>> Hi Michael,
>>>
>>> I'm not sure to understand. Are you speaking about OFBiz OOTB?
>>>
>>> Because the changes in OFBIZ-11317 are only related to ofbizUrl,
>>> hence org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course
>>> ofbizUrl supports the controlPath configuration. Also why would you
>>> want to replace OfbizUrlTransform by UrlRegexpTransform?
>>>
>>> Jacques
>>>
>>

[Michael Brohl-3]

UrlRegexpTransform should also be moved to the framework then, it does
not belong in the applications/product component when used elsewhere.

Michael Brohl

ecomify GmbH - www.ecomify.de

Am 15.02.20 um 17:20 schrieb Michael Brohl:

> Hi Jacques,
>
> thanks for confirming my concerns.
>
> This is why I am advocating for longer review periods and RTC over CTR
> for new features/improvements, especially in the core functionality
> with potential to break something. More eyes help to detect potential
> problems and flaws, they just need some time.
>
> I will try to provide a patch for a proper deprecated
> OfbizUrlTransform class (delegating everything to UrlRegexpTransform
> and with annotations) which we will need for a proper backport where
> functional changes should not be implemented. User implementations
> might rely on the OfbizUrlTransform implementation.
>
> I've assigned https://issues.apache.org/jira/browse/OFBIZ-11229 to me
> for it.
>
> Thanks,
>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
>
> Am 15.02.20 um 13:27 schrieb Jacques Le Roux:
>> Hi Michael,
>>
>> It's a long answer, but I think it's worth it.
>>
>> 1St, technically it's not my commit but James's ;)
>>
>> But I asked you, and reviewed/agreed with James's commit, so here we go.
>>
>> Your intuition was right, there is a problem with this patch. In some
>> place, like at least themes/tomahawk/template/AppBarClose.ftl the
>> CSRF token is not generated. Because in those places
>> OfbizUrlTransform is used.
>>
>> An immediate solution is to add the CSRF token generation in
>> OfbizUrlTransform class. I just did so at OFBIZ-11317
>>
>> But I agree that it's not satisfactory and your proposition to
>> deprecate OfbizUrlTransform in favour of UrlRegexpTransform makes
>> sense to me. That's mostly why I asked you a second time.
>>
>> Let's go a bit in the past. With  OFBIZ-5312, which was a major
>> change started in 2013, we notably introduced UrlRegexpTransform.
>> We used a Svn feature branch for that:
>> http://svn.apache.org/viewvc?view=revision&revision=r1535432
>>
>> It's unrelated, but while at it since it was quite unfortunate, a
>> conflict between Anil and Paul Piper (I was then working with Paul)
>> started and I had to find a solution which ended with ecomseo.
>> Fortunately OFBiz is flexible enough and eventually ecomseo is now a
>> simple clone of the ecommerce webapp (like exampleext is for the
>> example webapp). Everyone can pick her/his preferred one :).
>>
>> Now about OfbizUrlTransform vs UrlRegexpTransform: after OFBIZ-5312
>> UrlRegexpTransform was the only one used for ofbizUrl macro.
>> That stopped for good reason (Framework dependency on product
>> component) in 2018 with Deepak's work on OFBIZ-10463.
>> It also shows that during this period (3 years) nobody encountered
>> and issue with UrlRegexpTransform used to generate ofbizUrl macro.
>>
>> So yes, we should definitely deprecate OfbizUrlTransform in favour of
>> UrlRegexpTransform. But before that we need to be sure that
>> UrlRegexpTransform would not miss a feature of OfbizUrlTransform.
>> With OFBIZ-11229, Nicolas already started to merge UrlRegexpTransform
>> and OfbizUrlTransform classes. This issue  is still open and we
>> should use it to reach our goal.
>> Maybe it's already OK and we have just to check. For that I'll soon
>> attach a diff of the 2 classes at OFBIZ-11229
>>
>> Thanks to care!
>>
>> I wish a nice weekend to all of you.
>>
>> Jacques
>>
>> Le 14/02/2020 à 14:47, Michael Brohl a écrit :
>>> Please have a closer look at your commit, the controlPath
>>> functionality is implemented in the UrlRegexpTransform class...
>>>
>>> The implementing class for ofbizUrl is configured in the
>>> freemarkerTransforms.properties which is OfbizurlTransform for
>>> framework/webapp but UrlRegexpTransform for applications/product.
>>>
>>> This seems inconsistent to me.
>>>
>>> Thanks,
>>>
>>> Michael Brohl
>>>
>>> ecomify GmbH - www.ecomify.de
>>>
>>>
>>> Am 14.02.20 um 14:24 schrieb Jacques Le Roux:
>>>> Le 13/02/2020 à 13:06, Michael Brohl a écrit :
>>>>> There's currently only few questions left for me: we have one
>>>>> configuration which uses
>>>>> org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for ofbizUrl and
>>>>> this would not support the controlPath configuration if I don't
>>>>> miss something. Wouldn't it be better to change this to
>>>>> UrlRegexpTransform and deprecate OfbizUrlTransform?
>>>>>
>>>>> UrlRegexpTransform is located inside the product component but the
>>>>> macro is used in several other components as well. I think it
>>>>> belongs to the framework/webapp instead of applications/product.
>>>>> What do you think?
>>>>
>>>> Hi Michael,
>>>>
>>>> I'm not sure to understand. Are you speaking about OFBiz OOTB?
>>>>
>>>> Because the changes in OFBIZ-11317 are only related to ofbizUrl,
>>>> hence org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course
>>>> ofbizUrl supports the controlPath configuration. Also why would you
>>>> want to replace OfbizUrlTransform by UrlRegexpTransform?
>>>>
>>>> Jacques
>>>>
>>>
>