processone / ejabberd

Robust, Ubiquitous and Massively Scalable Messaging Platform (XMPP, MQTT, SIP Server)
https://www.process-one.net/en/ejabberd/
Other
6.07k stars 1.51k forks source link

"auth_cache_life_time" option only works on top level #4075

Closed poVoq closed 1 year ago

poVoq commented 1 year ago

Environment

Bug description

When trying to set auth_cache_life_time: 600 option for an external auth script, like described here https://docs.ejabberd.im/admin/configuration/authentication/#external-script ejabberd returns:

Error: "Invalid value of option host_config->'example.org': Option 'auth_cache_life_time' is not allowed in this context"

Apparently this option is only available at the top level, which means you can't have different cache durations for different external auth backends, which seems desirable depending on the how reliable the external auth source is.

Enabling and disabling it on individual host level via auth_use_cache: true seems to work, but the default of 1h is rather long.

prefiks commented 1 year ago

There is just one global cache, and not individual per host, so that's why you can't define this option per host. You can change it's value, but it will affect all vhosts. I think all modules use caches this way, and changing it will be tricky, it will be problem for inter node cache synchronization, as i think it requires defined cache names upfront.

poVoq commented 1 year ago

I see, but maybe it would be good to update the documentation then as the wording strongly implies that this is a vhost level feature.

badlop commented 1 year ago

Apparently this option is only available at the top level

More concretely, looking at the source code, the top-level options listed in this function can only be used globally, not inside host_config or append_host_config: https://github.com/processone/ejabberd/blob/eeacace02a5dd9b78de0193070374e0e8fed5d09/src/ejabberd_options.erl#L721

update the documentation then as the wording strongly implies that this is a vhost level feature.

Right, the documentation should be as clear as possible about how a feature or option is expected to work. Where exactly did you find the confusing or misleading sentence? What alternative wording do you propose?

poVoq commented 1 year ago

It doesn't explicitly say so, but I think the wording here: https://docs.ejabberd.im/admin/configuration/authentication/#external-script easily misleads to thinking this is an option available individually for each external auth.

badlop commented 1 year ago

I think the wording here: https://docs.ejabberd.im/admin/configuration/authentication/#external-script easily misleads to thinking this is an option available individually for each external auth.

What sentences do you find misleading? And what alternative wording do you propose?

Or maybe, what sentence would you add, and in what paragraph, so the text is more clear?

poVoq commented 1 year ago

Authentication caching is enabled by default, and can be configured with the options: auth_use_cache, auth_cache_missed, auth_cache_size, auth_cache_life_time.

This one.

Maybe it would be more clear if it said:

A single shared authentication cache is enabled by default and can be configured with the top-level options...

badlop commented 1 year ago

Aha, now I understand, thanks!

There is a paragraph that mentions options that can be configured for extauth inside host_config, the next paragraph mentions global options that cannot be configured per-host, and this is not mentioned at all.

In fact, that explanation is relevant for all authentication methods, so it should be moved to the top of the page.

I've applied those changes to the documentation, it should be less confusing now: https://docs.ejabberd.im/admin/configuration/authentication/

poVoq commented 1 year ago

Thanks, seems much better now.