rss-bridge icon indicating copy to clipboard operation
rss-bridge copied to clipboard

[BridgeFactory.php] Add RSSBRIDGE_WHITELIST environment variable support

Open gileri opened this issue 3 years ago • 8 comments

Enabling/disabling bridges via the whitelist.txt file can be cumbersome in some setups, for example in containers. This PR introduces a RSSBRIDGE_WHITELIST environment variable that accepts a wildcard (*) or bridges names separated by a comma instead of newlines.

After merging, the Whitelisting and Docker will need to be updated.

Edit : I'm of course open to modifications regarding the environment variable name or other changes.

gileri avatar Apr 08 '21 10:04 gileri

This is integrated in #2100

Bockiii avatar Jun 28 '21 17:06 Bockiii

Your PR looks more complete, I'll decline mine if yours it gets merged.

gileri avatar Jun 29 '21 20:06 gileri

Fixed by https://github.com/RSS-Bridge/rss-bridge/pull/2100

dvikan avatar Mar 24 '22 22:03 dvikan

Fixed by #2100

Was it really? Seems like the whitelist config got eventually removed from that PR by this commit: https://github.com/RSS-Bridge/rss-bridge/pull/2100/commits/99d6203c3095021d180c0694a2a5076e20d7450f.

ikajdan avatar Jun 12 '22 12:06 ikajdan

@gileri, can we get this reopened?

ikajdan avatar Jul 10 '22 18:07 ikajdan

I can see the advantage in this pr. But the container/environment still must be restarted for new env vars to take effect.

An alternative:

diff --git a/config.default.ini.php b/config.default.ini.php
index c1627aac..ae96d108 100644
--- a/config.default.ini.php
+++ b/config.default.ini.php
@@ -12,6 +12,8 @@
 ; timezone = "UTC" (default)
 timezone = "UTC"
 
+enabled_bridges = Gettr Vimeo
+

dvikan avatar Jul 10 '22 19:07 dvikan

If we add it to the config, wouldn't that make it more confusing as to where to actually configure the whitelisted bridges? because then we would have the whitelist.txt and also the config.

One of the reasons for doing it through the environment variable was also to make it easier to deploy to heroku et al.

I dont have a strong feeling either way, but we should come up with a strategy first before we add it to the config file.

Bockiii avatar Jul 11 '22 10:07 Bockiii

I think we should deprecate the usage of whitelist.txt and move it into a config value enabled_bridges and also en env value RSSBRIDGE_ENABLED_BRIDGES or perhaps RSSBRIDGE_ENABLEDBRIDGES.

dvikan avatar Jul 31 '22 01:07 dvikan

After having learnt about arrays in ini files it should be done like this:

enabled_bridges[] = TwitterBridge                                                                                       
enabled_bridges[] = YoutubeBridge                                                                                          
enabled_bridges[] = FooBridge

For the env var, they cant be arrays. They are strings. Not sure how to handle that. Maybe space separated:

RSSBRIDGE_ENABLED_BRIDGES="TwitterBridge YoutubeBridge FooBridge"

dvikan avatar May 04 '23 14:05 dvikan

So do you want 3 options?

whitelist.txt enabled_bridges config array in config.ini RSSBRIDGE_WHITELIST env variable?

Do you want them to all add up?

Bockiii avatar May 04 '23 16:05 Bockiii

Yeah I think all config should be in .ini file. And whitelist.txt and env overshadows for convenience.

dvikan avatar May 05 '23 06:05 dvikan

overshadow meaning replace? or just add.

So if I have a whitelist with BridgeA, an ini selection of BridgeB and BridgeC and an env variable of BridgeA and BridgeD, should A B C and D be selected?

Bockiii avatar May 05 '23 17:05 Bockiii

Had not thought about that. I mean replace yes.

dvikan avatar May 06 '23 14:05 dvikan

So in my example, only A and D would be selected since B and C are only in the .ini and you want that replaced?

I think thats very complicated. A simple addition would make more sense for the users I think.

Its also how most other things work. Access rights are usually additive and in this case I would see it as "bridges a user has access to".

Bockiii avatar May 06 '23 17:05 Bockiii

What you think @gileri ?

dvikan avatar May 07 '23 10:05 dvikan

fixed in https://github.com/RSS-Bridge/rss-bridge/pull/3428

@gileri

dvikan avatar Jul 02 '23 04:07 dvikan

Sorry for the delayed answer. Had a look over #3428, seems perfect. Thank you @dvikan, and @Bockiii for the review !

gileri avatar Jul 02 '23 20:07 gileri