Remove hard-coding of API URL resolution

[pluby]

This PR just plugs a hole in the configuration to work around hard-coded domain names in the code-base. It is a minimal change that leaves all the hard-coded names in place and uses the configured value as a back-stop.

[patcon]

Nice! Thanks @pluby! There's def a need for something here :) Will review

[patcon]

To clarify, is this change working for you right now? If so, in what sort of environment? (docker?)

I'm uncertain if process.env can be used without changes elsewhere, as we're not using a framework like reactJS that search-replaces each process.env value automatically during build.

I suspect you'd need to add to the DefinePlugin config: https://github.com/compdemocracy/polis/blob/f33d0b0471fe2d31d08321b58c74b5e59bb4fe77/client-report/webpack.config.js#L18-L22

But further, I don't think we actually have a way to send envvars into the client builds right now. The docker-compose.yml passes in env_file for the server-side parts of polis, but the clients built for running in browser don't get passed anything.

It's not ideal, but we've been keeping config in polis.config.template.js up to now. We currently only load that in gulpfile.js:

https://github.com/compdemocracy/polis/blob/f33d0b0471fe2d31d08321b58c74b5e59bb4fe77/client-report/gulpfile.js#L17

But I can't see why we couldn't make it available in webpack.config.js

Counter-proposal:

• we add a new value for this to polis.config.template.js (if existing ones won't work)
• we load that file into webpack.config.js
• we add an entry to DefinePlugin to copy that in

Would that suffice? Thanks again for the PR and upstream contribution!

EDIT: The major downside here is that this will make client-build that hardcodes the hostname into the docker build. So it won't work when we eventually want to have a docker container that can be deployed multiple places without rebuild. But maybe that's ok for now, since everyone who's hosting polis is building it for themselves anyhow :)

[patcon]

Ah hey, realizing that client-admin has fresher code, which generalized this urlPrefix logic better, and used it the same way for ajax calls:

https://github.com/compdemocracy/polis/blob/f33d0b0471fe2d31d08321b58c74b5e59bb4fe77/client-admin/src/util/net.js#L20

https://github.com/compdemocracy/polis/blob/dev/client-admin/src/util/url.js

So you'll just need to:

1. Copy the slimmer client-admin/src/util/url.js to client-report
2. bring the same domainWhitelist logic into client-report, from either client-admin or client-participation (involved edits to polis.config.template.js, gulpfile.js, index.html

How's that sound?

[pluby]

Thanks for your help with this, I'm fairly new to the codebase hence my mistake here. I was using a hard-coded change to the above file which I replaced with the code in this PR for greater flexibility. I need to look into why it's working, or if I simply missed the issue in testing. That change you pointed out seems better. Leave it with me and I'll update this PR after further investigation.

[pluby]

I wonder if we should just default it to document.location.protocol + "//" + document.location.hostname + ":" + document.location.port + "/";.

It might be worth using SERVICE_URL if set in a polis.config.js but it's not essential for my use-case.

One question I have is the purpose of the domain whitelist? I'm used to seeing whitelists in security contexts but as this is client-side code I guess it has another purpose.

Finally, in https://github.com/compdemocracy/polis/blob/f33d0b0471fe2d31d08321b58c74b5e59bb4fe77/client-admin/src/util/net.js#L20 basePath appears to always be the empty string. I figure is's all in the prefixUrl. Any thoughts?

[pluby]

I just pushed some tested changes that use the whitelist as suggested.

[patcon]

Awesome! Much appreciate, @pluby. Looks great

Oh, but still needs the domainWhitelist copied over from client-admin polis.config.template.js. If you could do that, I'll test it out and give it a review 🎉

You've made me realize we don't have even a single smoke test for the reports page, so I can add that to a sister PR, and then we'd know if this was working through the e2e tests, without so much manual labour :)

[pluby]

I copied the one from another client module (I already had one configured in my system so the template didn't matter).

[patcon]

k, now it works for me locally! LGTM

[ballPointPenguin]

fixed in https://github.com/compdemocracy/polis/pull/1617

although a good follow up could probably be to eliminate the need for util/url.js in the client apps altogether since it's pretty meaningless now