MagicMirror icon indicating copy to clipboard operation
MagicMirror copied to clipboard

Fixes #3253 - make `User-Agent` configurable

Open m-idler opened this issue 2 years ago • 8 comments

Adds a configuration option to overwrite the default User-Agent header that is send at least by the calendar and news module. Allows other modules to use the individual user agent as well.

The configuration accepts either a string or a function:

var config =
	{
		...
		userAgent: 'Mozilla/5.0 (My User Agent)',
		...
	}

or

var config =
	{
		...
		userAgent: () => 'Mozilla/5.0 (My User Agent)',
		...
	}

m-idler avatar Oct 31 '23 10:10 m-idler

Thank you for this PR!

First thing, the test fails are unrelated to your changes, we had problems with one unit test which are now fixed on develop, so can you please merge develop into your branch (or rebase on develop), thanks.

I'm not sure if we need the new getUserAgent() function because we store defaults in js/defaults.js, so AFAIK would be the normal way to add the entry

userAgent: `Mozilla/5.0 (Node.js ${Number(process.version.match(/^v(\d+\.\d+)/)[1])}) MagicMirror/${global.version}`,

in this file and then use this property with config.userAgent in the modules and the tests.

But before refactoring this may @rejas can review and tell us what he prefers ...

khassel avatar Oct 31 '23 23:10 khassel

I'm not sure if we need the new getUserAgent() function because we store defaults in js/defaults.js, so AFAIK would be the normal way to add the entry

userAgent: `Mozilla/5.0 (Node.js ${Number(process.version.match(/^v(\d+\.\d+)/)[1])}) MagicMirror/${global.version}`,

in this file and then use this property with config.userAgent in the modules and the tests.

In the beginning I thought about that as well, but it felt a little bit odd to have all those variables in a configuration string by default. Also I wasn't sure when those variables are available and in case someone wants to use other runtime variables in here, that might lead to issues - that's why I added the possibility to use it with a callback as well.

I also thought about, wether it would make sense to add a parameter (e.g. the module name, url, ...) to the method, so that it could be possible to use different user agents based upon those parameters.

Based on those thoughts i decided to wrap it in a utility method to allow easier, non-breaking, changes in the future. But if you prefer to consume the config.userAgent directly, I'm also fine with it 😉

m-idler avatar Nov 01 '23 19:11 m-idler

and I forgot something yesterday because config.userAgent doesn't work, the module only gets his own config section, not the global one (so the default userAgent had to be set in every module).

So I'm fine with merging this.

khassel avatar Nov 01 '23 20:11 khassel

since when does config. not work?

you have to pass it from browser side to node helper,

sdetweil avatar Nov 01 '23 20:11 sdetweil

may I'm wrong but AFAIK you can't get e.g. config.language in the node_helper.js of the module (not without special coding)

khassel avatar Nov 01 '23 20:11 khassel

yes, but 'special' is tiny..

from my MMM-Config

  notificationReceived: function (notification, payload, sender) {
    // once everybody is loaded up
    if (notification === "ALL_MODULES_STARTED") {
      // send our config to our node_helper
      
      // copy  global config info to module config  
      this.config.address = config.address;     
      this.config.port = config.port;
      this.config.whiteList = config.ipWhitelist;
      
      this.sendSocketNotification("CONFIG", this.config);   // this sends to node_helper
    }
  },

sdetweil avatar Nov 01 '23 20:11 sdetweil

so for using config.userAgent instead of the implemented function in this PR we had to do this.config.userAgent= config.userAgent; in the 2 modules.

I'm fine with both solutions.

khassel avatar Nov 01 '23 20:11 khassel

and then in node_helper to use it, its payload.config.userAgent (or whatever variable payload is stored in) from the sockerReceivedNotifications.

and you can get it ALL with

this.config.global_config = config

we have no cross module protection on the config data

sdetweil avatar Nov 01 '23 20:11 sdetweil