Re: Adopting Github Workflow
> I think you cannot make the tool responsible for how it is used in this > particular case. of course the tool is responsible! Jira is not a tool to review code! "Jira: Issue & Project Tracking Software" so nothing to do with code ;) Just imagine how it would be possible with another tool. For example with github each time James pushes new changes I will able to check only this new changes, we will be able to make discussion (question/answer/comment) on different points, I mean different threads in same PullRequest > The issue was opened for review only after the feature branch from > Jacques was opened, before the exchange was meant to be just between > Jacques and James. don't understand this statement. You mean sometimes jira discussion is understandable only by two people (so noone is able to enter discussion afterward) and we should consider this a normal process? What do you mean with `feature branch`? feature branch is related to git only, no? here we are talking about jira and github and how these tools are well suited for reviewing code. > > The contributors could have created a feature branch and work on it > together but chose to exchange patch files. > > Jira is not to be blamed here. signature.asc Description: signature
Re: Adopting Github Workflow
Hi Michael, > > To justify the need of making a change, to me the question is quite the > opposite: what does GitHub offer which Jira does not in the domain of > contributing/ project management/ issue tracking? Jira review process is awfull! I tried to review OFBIZ-11306 and give up after 3 revisions… Now there are 40 patches attached. How can you tell which one is ok? Tell me which one is fixing another after which discussion? … If we care about reviewing patches/contribution we should stop using jira. Samuel signature.asc Description: signature
Re: What is OFBiz public API?
Hi all, > Am 05.01.20 um 18:32 schrieb Mathieu Lirzin: > > > > I urge other contributors to join this discussion which is crucial to > > define our capability to work together as a community and my willing to > > continue to participate. I also hope others contributors will eventually join (many thanks to Jacques for joining!) since this discussion seems to me to be larger than a particular feature (component-load.xml): it is about the process of contributing to OFBiz. Michael has started to discuss because he had missed the commit which removes component-load.xml in applications and framework and he claimed [2] that we didn't discuss in d...@obfiz.apache.org before: completely true! Why do we need to discuss such an implementation detail? Some argue that we have to discuss before intruducing any *big* changement :confused: What is a *big* changement? In software library/framework it is quite easy to answer: a big changement is a breaking in public api. So here is the question from Mathieu: what is OFBiz public API? In my opinion we need an answer for this question otherwise we need to discuss every single changement! which seems to be really cumbersome! And even if we discuss every single changement how to decide it is good for our community: *one* other contributor thumb-up is enough? maybe *two*? do we need to wait forever if others don't care about a particular changement? > OFBiz is not just a library or core framework, it is a multi-level project: > > * a web development framework > > * a web based ERP system on base of this framework > > * highly flexible and extendable through various mechanisms. Like so many frameworks, OFBiz is not different according to this points. And like so many frameworks which are extendable we need API to ease extension. > > To my understanding, if we use depends-on exclusively for framework, > applications and plugins, this would not be possible anymore. This is where you're wrong. From the beginning using depends-on in framework doesn't imply using it in plugins! The thing which drive Mathieu to revert is that you cannot, in *framework* override depends-on with a component-load.xml. And here we are with the actual discussion: 1. component-load.xml in plugin directory seems to be feature (nobody discuss this point) 2. what about component-load.xml in framework and applications directories? is it also a feature (in other words a public API) or an implementation detail > [1] > https://cwiki.apache.org/confluence/display/OFBIZ/Ofbiz+as+a+development+framework+-+release+10.04 This reference is a bit old and stated as wip so I will consider it as irrelevant for our discussion ;) cheers, Samuel [1]: https://lists.apache.org/thread.html/c2612f1e296b6ea15872185871d3a9d83d6a4afc6d2a76f7a336a126%40%3Cdev.ofbiz.apache.org%3E [2]: https://lists.apache.org/thread.html/7eab3d2ae3bbeadb184b02f75f7b2b86263532485e88ecba4d4dc780%40%3Cdev.ofbiz.apache.org%3E signature.asc Description: signature
Re: Removing “base/config/component-load.xml”
Hi all, Quoting Jacques Le Roux (2019-12-18 12:39:56) > OK, I sent this message before seeing Mathieu's last one. I guess the revert > Mathieu should close this discussion. I suggest to create a new one about > feature forking (please stop this one). feature forking is another discussion => agree with this! But I'd like to finish this discussion about component-load.xml because I still want to remove this file (I mean in framework, in plugins you can do whatever you want) > > I must say I'm strongly against feature forking. I have already explained > myself why several times. I can reiterate if needed. Mostly for the same > reason Mathieu already exposed in his last message actually. > > For the new feature itself, it seems wise to me to have it working with > component-load.xml files before starting it on the trunk... It is working with component-load.xml from the start! but not with framework, applications components directories which is (from my point of view) an implementation detail: you should not rely on which order framework is loading its components! It seems to me that you should write your code in plugins and actually in plugins you can expect every components in framework to be loaded, but maybe I'm wrong about how users use obfiz framework... > > Thanks > > Jacques > > Le 18/12/2019 à 12:09, Jacques Le Roux a écrit : > > Hi, > > > > In this thread I superficially see a conversation where no one listens to > > anyone else's point of view. > > > > If I dig a bit more I see Mathieu's perspective who wants to achieve > > something new which depends on the discussed subject (his [1] below). > > > > And Michael's perspective who worries about custom projects in production. > > > > Those are facts, now let's analyse each last main arguments. > > > > Mathieu asks Michael to provide an "explanation regarding why it matters > > in production environments to be able to patch" component-load.xml files yes we are still waiting for your answer Michael ;) In my opinion we cannot go ahead in this discussion without your answer, without your need about component-load.xml: are you trying to avoid loading a particular framework component? do you patch a framework component and need another one to be loaded first to make your patch works? ... > > > > Michael does not answer this question but reiterate a question Mathieu did > > not answer yet: "is [this] tested working [together] with component-load.xm" Condidering poor test coverage of obfiz this question seems to me irrelevant! I will really appreciate to reject any patch proposal which does not come with a test, but, if I'm correct, this not a practise in ofbiz community. As I've done the requested test manually and Mathieu has reverted the commit I think we can left behind this question. Samuel > > > > I believe these points must be answered before we get further in this > > discussion > > > > Jacques > > signature.asc Description: signature
Re: Github PRs and Jira
Hi all, I see another point to consider on our workflow: running continuous integration (checkstyle, compilation, unit test, integration test, sonar) on every code submission (I mean before commiting on trunk!) It seems to me that is not really complicated with github/gitlab pull requests. But I have no idea on how to do this on jira. Samuel Quoting Jacques Le Roux (2019-11-29 08:50:05) > Hi, > > Yesterday I have a short discussion with Pierre Smits about Github PRs and > Jira. > > Pierre was asking about https://github.com/apache/ofbiz-framework/pulls I > answered that our workflow is to contribute patches through Jira. > > I want to clearly document that in > https://cwiki.apache.org/confluence/display/OFBIZ/OFBiz+Contributors+Best+Practices > > But I know that it will not be enough. The PR reflex is now an acquired > behaviour. So I'll also add a notice in the top of our main README.adoc to > make it clear on our main Github page. > > Before I do, please chime in if you have other ideas > > Thanks > > Jacques >
Re: question about ServiceHandler.checkSecureParameter
yes there is a need for csrf check on get request ;) I will write details in OFBIZ-11306 [1] Samuel [1]: https://issues.apache.org/jira/browse/OFBIZ-11306
Re: question about ServiceHandler.checkSecureParameter
Hi James, I still don't see any reason to keep checkSecureParameter in any form because it is related to ServiceEventHandler. Protection against csrf is a good idea but it has no thing to do with Service. It should be done upstream in http request processing so every type of event (ServiceEventHandler, JavaEventHandler,…) could benefit from this protection. Samuel Quoting James Yong (2019-11-26 17:26:59) > Hi Jacques, all, > > Haven't look into the POC yet. Please see the following updates: > > 1. Not a good practice to allow state-changing request via GET method without > a token to check for CSRF. > > 2. Not a good practice to expose CSRF token in url. See [1]. Moreover, token > in url may also be exposed via if user posts screenshots. > > 3. CSRF is not a concern for GET request if we avoid point 1 & 2. > > 4. In OFBiz, the same request handler generally can process both GET and POST > requests. The checkSecureParameter in service events would check for no query > string and as a result, state-changing operation is restricted to a POST > method when service event was used. > > 5. Propose to revert the removal of checkSecureParameter, but add new code to > allow exceptions to query-string check in service events. Exception can be > made by setting a new attribute, allow-query-string-for-event = true | false > (default), in security tag within request-map tag. Also extend > checkSecureParameter to other event types. > > 6. Propose to add new attribute, csrfToken = true | false (default), to > security tag to support anti-csrf for post request: > > a) For forms: if true, will generate hidden token field value using CSRF > Guard library ( see [3] ) for every rendered form and store this token inside > session map variable. Support may be added for token name to be randomized. > > b) For login form: There is a need to prevent Login CSRF. Token will also > be created using pre-session. See [2] for Login CSRF. > > c) For XMLHTTPRequest: Use default value for same-origin policy. 1 token > generated per user session for ajax call. Support may be added for token name > to be randomized for each session. > > 7. The service.http.parameters.require.encrypted property that was removed, > seems related to encrypting sensitive parameter values but wasn't > implemented. May look into this at a later time. > > 8. For implementations that aren't using OFBiz screens, a property may be > added to disable the check for CSRF token. > > Reference: > [1] > https://danielmiessler.com/blog/sensitive-information-sent-in-the-url-over-https. > [2] > https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html > [3] https://www.owasp.org/index.php?title=CSRF_Guard&redirect=no > > Regards, > James >
Re: Change commit message template?
Hi, agree with Mathieu: I prefer keeping issue id in footer (at the end of commit message) so we can save some characters in subject line to express something meaningful. In addition if we put this information in commit footer (or body) we can copy the complete link to issue (for example https://issues.apache.org/jira/browse/OFBIZ-4274 instead of just OFBIZ-4274) so that we can just "click" on it to display related issue :) But like Mathieu said, I don't mind if some people find this `OFBIZ-XXX` so meaningful that they want to keep it on subject line. Samuel
Re: question about ServiceHandler.checkSecureParameter
Hi James, actually `checkSecureParameter` is only for service event in a request map. So it doesn't mean you are updating server data. Moreover you can also update server data with a java event and in this case `checkSecureParameter` is not called. So in my opinion this protection is very limited. Samuel
