Create a new repository for js lib

legalsylvain commented 6 years ago

hi all,

Findings

For a while, there are a lot of js libraries that are included in oca "web" modules. (for graphical purpose, or to make light web apps) and I guess that the quantity of such modules will increase.

sample :

stock-logistics-barcode / mobile_app_inventory : web app to make an inventory. V7 version (PR V8 by @hparfr and PR V10 by @alexis-via ) It is using angular, angular-cookie, angular-translate and ionic lib
web / web_widget_bokeh_chart : widget to use bokeh lib to make nice graph. V10 by @lreficent

Regarding that discussion.

The current deployment of js lib is very simple

add the js / css code in the static folder of the module
include a new path of js / css lib in odoo xml file)

That's cool and very easy to deploy. It works anywhere. (runbot, odoo.sh, appstore, etc...)

Limitations

but in other hand,

it make huge PR that are not simple to review
it is not possible to share the same lib between two modules. (duplicated code)
and above all, it is a possible fail of security, because if a contributor propose a JS lib, he can alter it without real peer control.

Proposals

Step 1

creating a new repository ~~js_librairy~~ web_library (comment) under OCA umbrella
in this repository, create a module per lib lib_bokeh, that contains only the lib (js & css files).
If many major incompatible versions are available (for exemple, for angular JS), create 1 module per major version. lib_angular_1, lib_angular_2. (comment)

If a module has to use a lib, developper just have to :

add an odoo dependency
add a path in a xml file
- lib_bokeh/static/lib/bokeh.js
- lib_bokeh/static/lib/bokeh.css

Step 2

In a second time, we can add a bot that check that the code is secured (not altered) up-to-date, ...

What do you think ?

CC : @hbrunn, @jbeficent, @sebastienbeau, @gurneyalex.

hbrunn commented 6 years ago

good idea. But let's call this web_library (or whatever, but don't pin it to js, because something like the bootstrap update module could go there too) I think the modules in there also should already add the appropriate xml to make the library available, so that just adding the dependency suffices.

legalsylvain commented 6 years ago

Hi holger, thanks for your review.

But let's call this web_library (or whatever, but don't pin it to js, because something like the bootstrap update module could go there too)

:+1:

I think the modules in there also should already add the appropriate xml to make the library available, so that just adding the dependency suffices.

yes I was firstly thinking about that implementation, but if we want to host multiple version of the same lib. (angular-1.1.js, angular-2.js) so we have to let the possibility to each modules to choose the version). (a few lines of code in each modules, but more modular).

(alternatively, we can create a module per lib version), but I fear that it will be a mess to maintain. Intermediate choice could be to create a module only per major version.

hparfr commented 6 years ago

Good idea. Your proprosal is simple and will work well for the moment.

Another solution, is to add js dependencies in packages.json and use tools like npm to download and update dependencies. It's a bit more complex since you have to add a tool in the build chain.

legalsylvain commented 6 years ago

Hi @hparfr,

Another solution, is to add js dependencies in packages.json and use tools like npm to download and update dependencies. It's a bit more complex since you have to add a tool in the build chain.

Well, this implementation will prevents users to use modules downloaded from the appstore for exemple. (except if they run extra commands). Or did I missed something ?

hparfr commented 6 years ago

I'm not familiar with the appstore and how to integrate with it.

legalsylvain commented 6 years ago

Well, appstore allow users to simply download code (with dependencies). downloaded modules should simply be placed in the addons folder. It is very used by a lot a people that doesn't work with an integrator.

hbrunn commented 6 years ago

we absolutely need modules instead of npm or whatever

For versions: Often it's a non-issue anyways because libs are backwards compatible, and if they are, angular-1, angular-2 etc. seems fine to me

yelizariev commented 6 years ago

It's important step to make OCA modules more safe. Good proposition :+1:

it is a possible fail of security, because if a contributor propose a JS lib, he can alter it without real peer control.

legalsylvain commented 6 years ago

@hbrunn. OK. I updated the initial proposal regarding your comments. I have a technical question about js / css imports. some time I inherit web assets and sometime point_of_sale assets (or others files). Is it different ? If yes, the import should be written in the custom module. If no, I agree with the following sentence :

I think the modules in there also should already add the appropriate xml to make the library available, so that just adding the dependency suffices.

thanks.

hbrunn commented 6 years ago

it's different. starting from 11, this is a moot point anyways because there widgets just say which libraries they use, and the webclient loads them dynamically: https://github.com/OCA/OCB/blob/11.0/addons/web/static/src/js/fields/abstract_field.js#L38

legalsylvain commented 6 years ago

OK, so if I sum up correctly :

10.0 and before : custom module includes the js / css files.
11.0 and after : lib module can includes the js / css files.

(anyway, it is a matter of minor changes).

How can we move forward ?

@pedrobaeza, @ other board member, can I create and init the new repository ? Or there is a script to make it automatically ? (or a procedure)

regards.

yajo commented 6 years ago

Hi there, I completely agree this is a real problem, but also I'm completely against having code in a separate repo. That would be moving the problem to another place, not fixing it.

Odoo has a real problem in the way it produces assets bundles. By falling into their common NIH syndrome, i.e. they make it almost impossible to, for instance, provide debug maps for assets written in metalanguages such as Sass, Less, TypeScript, etc. It will be fun now to see how do they upgrade to Bootstrap 4, which is now fully written in Sass, while Odoo relies almost completely into their awkward assets bundling system and Bootstrap's Less version.

But let's stick to the problem at hand, which is completely real.

Currently the JS world has one of the most advanced software packaging systems ever: npm. I always hated to bundle (a.k.a. vendor) JS code. That's not our code, and as such it shouldn't be in our repos: not here, not in any other.

So what should we do instead? @hparfr nailed it: Just build a bridge between npm and Odoo's assets bundle system. And believe me, the design is quite simple. Let's call this addon web_npm:

Models

`web_npm.collection`

Fields:

module_name: Your sub-addon name.
package_json_path: Path to the package.json file under you sub-addon tree structure.
assets_view: Choose among backend, frontend, editor and common.

Methods:

create,write: Overload these methods. When we create or edit a record, we leverage npm to download any dependencies into odoo.tools.conf.addons_data_dir + "/web_npm/{addon}/{web_npm_collection.id}", which will always be a writable directory. Alternatively, we can choose to use the filestore or the ir.attachment system.
unlink: Go remove that path then.
_register_hook: Make sure those paths are present and good.

Views

It should patch web.assets_[backend,frontend,editor,common] views to inject the generated contents. This would really be the bridge with Odoo's assets bundles.

Controllers

Possibly none, if we patch correctly the views instead.

Once all of that is done, all you have to do in your sub-addon is:

depend on web_npm
add a standard package.json
declare your web_npm.collection record(s) in a normal xml data file.

That would be a real fix, and would lead to having real OCA's code under OCA's repos, and others' code under others' repos.

hbrunn commented 6 years ago

I'm absolutely opposed to depend on a package manager, none of the modules produced by me will ever do this

legalsylvain commented 6 years ago

Hi @yajo,

thanks for your PoV.

That would be moving the problem to another place, not fixing it.

Could you explain why it doesn't fix anything ? For me :

it centralizes external js lib in a unique repository
it prevents duplication code, that is great. (i mean if two modules need angular / bokeh, or whatever lib)
it is more simple to check lib alterations, manually in a first time, or automatically in a second one.
it makes modules PR lighter to review.

I see improvment without regression in the design I propose. If you see any, please detail your PoV.

I don't say that npm is bad, just that using it in the OCA has pro and cons.

it is not a native technology in the Odoo core.
it requires extra knowledge from developpers to include js libs.
it requires connexion to the internet during the update of the module. (that is not the current behaviour)

so let's make OCA modules better one step at a time. I think that if npm is important for you or other OCA members, we should better lobby Odoo S.A.

Are there other poV from other developpers ? @StefanRijnhart ?

Regards.

yajo commented 6 years ago

Just some details:

it centralizes external js lib in a unique repository

You don't need that, it's already done: https://www.npmjs.com/search

it prevents duplication code, that is great. (i mean if two modules need angular / bokeh, or whatever lib)

You're actually duplicating code from those repos into OCA's. More or less exactly what's happening right now, just in a separate addon instead of inside the addon itself.

it is more simple to check lib alterations, manually in a first time, or automatically in a second one.

IMHO it's harder to maintain your own angularjs fork than to change one line angularjs==2.0 for angularjs==2.1.

it makes modules PR lighter to review.

PRs outside that very repo yes, indeed. Inside that repo, I'd like to know who will ever check the code you're copypasting comes from trusted sources. Besides that, we'd need a completely new set of tools to maintain the code quality, or directly skip it.

it is not a native technology in the Odoo core.

I think that's an Odoo bad design choice, but if they want to maintain their own fork of bootstrap, jQuery, fontawesome, etc., it's up to them of course.

it requires extra knowledge from developpers to include js libs.

We can try to make the xml data more descriptive or less dependant on package.json format if you prefer, or provide good example templates and docs for most common cases (just bundling a couple of deps).

legalsylvain commented 6 years ago

@Yajo.

Again I'm not comparing my proposal and npm (that I don't know), I just compare my proposal and the current state of the OCA modules (that include without norms many js libs). And I don't understand why you don't think the current proposal is a good step forward.

More generally, we are all good guys with good complementary skills. Debate can be done respectfully. In this direction, i'm not sure that setting :-1: on all comments you disagree is a constructive attitude. On another side, @hbrunn, could you explain more deeply why you're disagree with npm solution ?

Kind regards.

hbrunn commented 6 years ago

I disagree with everything that I can't (theoretically) check before distributing it to my customer with my signature. Whatever code is pulled from the network after deployment is a possible security breach, and there's a plethora of Odoo installations that for good reasons can't even connect to the internet.

When you use npm or whatever $ifixstuffforyoufromthenetwork, you can just as well pull the code from the google cdn, which is a bad idea exactly for the aforementioned reasons. https://hackernoon.com/im-harvesting-credit-card-numbers-and-passwords-from-your-site-here-s-how-9a8cb347c5b5 give a nice illustration, which is of course not npm specific

hparfr commented 6 years ago

When you use npm or whatever

There is no difference with python's libraries: you reference them in your manifests so you don't add them directly in the module.

hbrunn commented 6 years ago

and then the python runtime goes and fetches missing modules from the web? I don't think so

hparfr commented 6 years ago

and then the python runtime goes and fetches missing modules from the web? I don't think so

The dependency has to be resolved somehow: it can be reproducible (ie package.json + npm) or not (like today).

If reproducible: it can be done on your host by your tool of choice / by a module like Yajo proposal / on OCA/repo by a bot.

The question is not if you don't npm like or one of his concurrents (bower, yarn, put your custom tool here), it's how you can reproduce the build.

Because you can't trust the result (huge dump of js files, it's even worst if minified or transpilled), you have to put the trust elsewhere, and that's how we do with libs in python.

If we want to store all the js libs in a repo, the following is possible :

1) the dev push his package.json to web_library/module_name/ 2) an OCA bot do the npm install and add the result to the PR

Result: all the js libs are in the OCA repo.

Pro: offline and appstore compliant, reproducible, can be monitored*
Cons: it will take some place, especially if there is projects like angular2+ which have a huge amount of dependencies, it will take time and cpu to build js or ts projects, you have to do 2 PRs for adding a module.

*monitored: https://github.com/blog/2470-introducing-security-alerts-on-github

legalsylvain commented 6 years ago

Hi @hparfr. Thanks a lot.

can be monitored*

Interesting !

you have to do 2 PRs for adding a module.

Only the first time a guy uses a new lib to add it to web_library. After, you only have to make a dependency to the existing web_library module.

More generally, I like the design you describe, because it answers to the need @hbrunn says. (no connection required after deployment). i see it very compatible with the design of my proposal. We could imagine the following :

step 1 : -> create web_library repository and create module, adding code manually. -> make others module depend on web_library module.

step 2 : -> once done, when the oca bot is ready, add a package.json file in the module, and let the bot working. (more work at the beginning, but less work after, and more secure).

What do you think ?

lasley commented 6 years ago

I see the reasoning behind this, but I am against moving everything into its own repo and/or grouping all into one module.

From a high level perspective, we are not solving anything here. There will still be pull requests for the same code, except now we're going to have to manage multiple JS versions in the same repo. This means that the repo will always gain in size drastically, because instead of upgrading the same library in context we are instead going to have to add the entirely new version in order to maintain backwards compatibility with pre-existing code using the library that is unknown because we're out of context.

From a security perspective, grouping everything into one module will create undue maintenance burden while creating and maintaining a secure system due to the amount of unnecessary code that is being included. My team has to manually review every bit of code that is authorized for our healthcare customers to use, which absolutely includes all JavaScript dependencies. I do not want to have to pay for my team to review AngularJS so that my customers can use Bokeh charts. Including AngularJS for no reason will have obvious performance impact as well.

Why can this problem not be solved by creating a base_angular and base_bokeh module, or whatever else is needed in context? By keeping the library as close to the modules that use it, we keep interest in the proper location. I do not foresee many people following the repo dedicated to gigantic PRs, which will cause stagnant and cross-dependent PRs.

yelizariev commented 6 years ago

What about making a repo with a single module inside, say web_libs, which has big set of js\css libs in his static/ folder. The module would have 'installable': True, but is not going to be installed. Instead of that, it's just added to addons-path and included in an xml file of a target module which need some of the libs.

web_libs

.
├── __init__.py
├── __manifest__.py
└── static
    └── lib
        ├── angual
        │   └── 1.6.8
        └── bokeh
            ├── 0.12.5
            └── 0.12.6

some_module

<?xml version="1.0" encoding="utf-8"?>
<odoo>
        <template id="assets_backend" name="angual assets" inherit_id="web.assets_backend">
            <xpath expr="." position="inside">

                 <script type="text/javascript" src="/web_libs/static/lib/angual/1.6.8/angual.js"></script>

           </xpath>
        </template>
</odoo>

The idea is that /web_libs/static/... urls are still available even though web_libs is not installed.

yajo commented 6 years ago

I disagree with everything that I can't (theoretically) check before distributing it to my customer with my signature

npm provides the package-lock.json file, which includes [hashed integrity verification](I disagree with everything that I can't (theoretically) check before distributing it to my customer with my signature). We can set that as a requirement for web_npm addons.

Of course there's chance of injecting malicious code using any method available anywhere at any time. But just tell me one thing: did you take the time to get a diff between the jQuery version Odoo bundles and the jQuery version that is officially released through jQuery? And did you read all those 9500 LOC looking for security holes?

Let's get real.

there's a plethora of Odoo installations that for good reasons can't even connect to the internet

In such case, that's not a problem of the packaging method: not pip, docker, deb, npm, direct git clone, or anything else can cope with that. If that's your use case, then you'll have to deliver code manually through (I guess) an USB stick. That will still be your case after all, so I guess that's not the case we're trying to cover here.

I do not foresee many people following the repo dedicated to gigantic PRs, which will cause stagnant and cross-dependent PRs.

You nailed it. We need tools that give best balance between maintainability, usability and security. We don't need 5 layers of security if the 1st one just works. Hash-pinned deps verifications is quite enough AFAICS.

Also about @yelizariev's proposal: It makes sense, although it produces problems:

2 addons inlcuding the same lib would add it twice to assets, impacting in performance (and possibly becoming buggy).
Review process is hardened, since to review the code you should have to review 2 addons instead of one.
After all, we cannot bundle more than 1 library version into the same page at the same time, so instead of bundling many versions, OCA should decide one and make all of its addons work on it.

Interesting that point 3 affects also any other method we discussed here (including the npm one). We're going to get a packaging system that is way more flexible that we can possibly achieve in the Real World™️ due to Odoo's way of doing JS packaging... I wonder if all of this is any better than the current situation... 🤔

Why can this problem not be solved by creating a base_angular and base_bokeh module, or whatever else is needed in context?

After all said, it seems the most sensible solution indeed.

yelizariev commented 6 years ago

I wonder if all of this is any better than the current situation...

As for me, the main advantage of using centralized way to include libs is the security concern about OCA repos (nobody checks that attached libs were not modified).

It seems difficult to force all modules to use the same version of a lib. Imagine situation when there are module base_angular and 10 modules depended on it we decided to switch base_angular to newer version of the lib -- how to review and check 10 modules, that they work against newer version.

So, security issue probably the only advantage of centralized way of including libs.

legalsylvain commented 6 years ago

Hi @yelizariev and others. About version, @hbrunn talked about that. We can identify 2 cases :

Minor changes (minor version) The module should work with the changes. so, we should always use the latest stable version. For exemple, for bokeh lib, I imagine to have a module : lib_bokeh with the file bokeh.js (with the version 0.12.5). If the version 0.12.6 is released, we should replace the old lib by the new one and update the number of the revision of the odoo module. This is quite secure, because library are compatible between minor versions. (or should be).

If the new minor revision introduces a changes that breaks an odoo module, we should fix the module. In fact, the same occurs with python lib. We are not specifying by default, the revision of the lib in the openerp file. (just "need python-xxx-lib"). In fact, the same occures with all the rest of the odoo environment. When we develop a module for Odoo 10.0 we are not saying that it works with a given revision of odoo 10. We assume that it should works with the head of odoo 10.0. And sometimes, something is broken in the latest commits in Odoo. Since 3 monthes a OCA / pos 9.0 module is broken because of changes in Odoo Core, (changes introduced in a "stable" version). Ref.

If people wants to have secured deployment, (that I can understand !) it can be handled just by defining the revision of the odoo module (pypi deployment) or the commit (buildout deployment) (or wathever deployment system used).

And updating with the lastest revision is a good thing to pull last patches and so fix security issues too.

Major Changes (new major revision) The typical cases is for angular. So in that case, I think we should host two odoo module. angular_1 and angular_2, because the refactoring is complete, and because we all know that the libs are fully incompatible and we don't want to port 10 modules using new angular lib at the same time.

What do you think ?

About review, I'm convinced that if we have modules just to host a lib, it will be very easy to review. (In fact, get the js file from the web, make a meld, check that there is a no diff), and make a :+1: on the PR. (2 minutes). Like FP say : Super easy, super fast, super simple. ;-) If the js is hosted in each odoo modules that need the lib, the work of the reviewers will be focused on the features of the odoo modules, not on the JS lib.

And again, we should consider that proposal as a first step, and introduce after some improvments.

regards.

github-actions[bot] commented 2 years ago

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.

OCA / web