Allow apps to add/modify config js output via hook.

[ringmaster]

This will allow apps to output some dynamic data in javascript that their static javscript files can act upon.

---

To use, connect the hook to a class method in the app in app.php:

OC_Hook::connect('\OCP\Config', 'js', '\OCA\Testapp\Testapp', 'getJs');

Then implement the connected method in the specified app class:

<?php

namespace OCA\Testapp;

class Testapp {
    public static function getJs(array $data) {
        $data['array']['testapp'] = 14;
    }
}

The value added to the array will appear in /index.php/core/js/oc.js

This PR replaces #6879. Intend to backport for 6.0.2

---

@DeepDiver1975 @PVince81 @jancborchardt @kabum @icewind1991 ?

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/ Alternatively you can add a comment here where you state that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[ringmaster]

@owncloud-bot C'mon now. This is insulting.

[DeepDiver1975]

nice approach! Would it make sense to use a new array and merge with $array after the hook being emitted?
Otherwise the array could be manipulated

[ringmaster]

My intent was to allow the array to be manipulated by apps if they want. Flexibility++

[jancborchardt]

@ringmaster I’m not enough of a coder to review that. ;)

[LukasReschke]

While I was the person that introduced core/js/config.php, I do nowadays believe that this was a wrong decision and we should get rid of this file in the long term.

I see the following problems:

1. There is no same-origin policy on JS, this means an attacker is able to include this file. Therefore he knows the value of all of this variables. While this is not such a big problem, it is still bad practice to allow an attacker to gain more information as necessary.
2. The values are not really properly sanitized in there, if an hook is insecure this leads to a huge XSS vector.
3. App developers are usually not aware that these values are exposed to the public, we should document that no confidential data should be stored in there.

Therefore I'm somewhat ambivalent regarding this patch. I absolutely see the point that app developers may want to add more values, but I also believe that we should get rid of this file.

So, I'm okay to merge this as a short term solution, but in the long term we should switch to JSON. (which doesn't seem to be possible yet as @PVince81 explained to me)

[DeepDiver1975]

👍

[MorrisJobke]

@owncloud-bot This is okay to test

[ringmaster]

@LukasReschke I understand your concerns. Using JSON would mitigate some potential parsing issues, but won't to anything about XSS or privacy.

Assuming ownCloud continues to use this file (and I'm not sure how it can do without it), it has the advantage of reducing the calls to index.php for javascript to a single request and allowing us to centralize the changes (for example, if we wanted to convert the output to JSON) in that single file.

IMO, more benefits than detriments. 😄

[MorrisJobke]

👍 Jenkins c'mon ... test this stuff

[bartv2]

@PVince81 @LukasReschke any hint what the problem is with switching to json? other then that it is async, and that could be solved with Promisses

[PVince81]

One potential problem is that many apps are doing stuff in global namespace or in $(document).ready(). That code would be run before the config.js has even been loaded, so if it needs values it won't work. My point was that a lot of code needs to be changed and made to work even with promises, or at least that ALL init code for ALL JS files is run in functions and not directly.

I hope to be able to reach that stage eventually, because being able to manually init apps is needed for JS unit tests.

[scrutinizer-notifier]

A new inspection was created.

[LukasReschke]

@LukasReschke I understand your concerns. Using JSON would mitigate some potential parsing issues, but won't to anything about XSS or privacy.

It will, due to the same-origin policy you are not able to include an external JSON from another domain.

[ringmaster]

Ah, I misunderstood. What you mean is not converting this output to a single javascript object (ala OCConfig.somekey), but serving this data as JSON through XMLHTTPRequest, hence my confusion.

Either way, adding the ability for apps to modify the output is possible and very useful for app developers.

[LukasReschke]

Either way, adding the ability for apps to modify the output is possible and very useful for app developers.

Sure - that's why I've said we should merge it :-)

[PVince81]

@owncloud-bot retest this please

[ghost]

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3311/

[ringmaster]

@karlitschek Ok to backport to OC6?

[karlitschek]

yes please