Ejabberd auth #1593
Ejabberd auth #1593
Conversation
Added get and set_generic_opt functions
Remove calls to ejabberd_config to access auth_opts Make those calls through functions in ejabberd_auth instead
src/ejabberd_auth.erl
Outdated
| set_generic_opts/3, | ||
| get_generic_opt/2, | ||
| get_generic_opt/3, | ||
| get_jwt_username_key/1, |
There was a problem hiding this comment.
Lines 36-42 are too specific, they shouldn't be provided as API here. 33-35 should be sufficient for ejabberd_auth_* modules. ejabberd_auth shouldn't be aware about specific possible config keys inside auth_opts.
src/ejabberd_auth.erl
Outdated
| Opt :: atom(), | ||
| KVs :: [tuple()]) -> {atomic|aborted, _}. | ||
| set_generic_opts(Host, Opt, KVs) -> | ||
| OldOpts = ejabberd_config:get_local_option(Opt, Host), |
There was a problem hiding this comment.
set_generic_opts should access auth_opts by default, so the API here should be set_opt(Host, Opt, Val) (including the function name change).
src/ejabberd_auth.erl
Outdated
| -spec get_generic_opt(Host :: ejabberd:server(), | ||
| Opt :: atom(), | ||
| Key :: ejabberd:key()) -> undefined | ejabberd:value(). | ||
| get_generic_opt(Host, Opt) -> |
There was a problem hiding this comment.
Is it that useful to fetch all options? I would rather expect an API like get_opt(Host, Opt) and get_opt(Host, Opt, Default).
src/ejabberd_auth_http.erl
Outdated
| AuthOpts = ejabberd_config:get_local_option(auth_opts, Host), | ||
| {_, AuthHost} = lists:keyfind(host, 1, AuthOpts), | ||
| AuthOpts = ejabberd_auth:get_generic_opt(Host, auth_opts), | ||
| AuthHost = ejabberd_auth:get_host(Host), | ||
| PoolSize = proplists:get_value(connection_pool_size, AuthOpts, 10), |
There was a problem hiding this comment.
These should be fetched with accessors from ejabberd_auth as well.
The API functions made less specific and should prevent backend modules any knowledge of the auth_opts config tuples
src/scram.erl
Outdated
| false; | ||
| AuthOpts -> | ||
| {password_format, scram} == lists:keyfind(password_format, 1, AuthOpts) | ||
| case ejabberd_auth:get_opt(Host, password_format) of |
There was a problem hiding this comment.
ejabberd_auth:get_opt(Host, password_format) == scram looks even better. :)
Change from case construct to comparison
This PR addresses #
Proposed changes include: