Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support loading Elixir modules for auth #4315

Merged

Conversation

marc0s
Copy link
Contributor

@marc0s marc0s commented Nov 22, 2024

Allow to specify an Elixir module name in auth_method. This would allow to create custom auth modules using Elixir.

If the referenced module, M, cannot be loaded as ejabberd_auth_M, try to load it as Elixir.M.

This is my first contribution to this repo (and my first in Erlang at all), so please excuse me any basic mistake I could have done. Tell me and I'll fix them :)

@marc0s marc0s marked this pull request as draft November 22, 2024 09:44
@marc0s marc0s force-pushed the support-loading-elixir-modules-for-auth branch 2 times, most recently from dfbec82 to b86c081 Compare November 22, 2024 10:21
@marc0s marc0s marked this pull request as ready for review November 22, 2024 10:23
@marc0s marc0s marked this pull request as draft November 22, 2024 12:04
@marc0s marc0s marked this pull request as ready for review November 22, 2024 12:11
@marc0s
Copy link
Contributor Author

marc0s commented Nov 22, 2024

This can be tested with this dummy module https://gist.github.com/marc0s/754bf23e3a4a01ab831246342b1e7621

Tell me if you prefer it to be included in the PR to be added to the sources (like mod_presence_demo.ex, for instance)

@marc0s
Copy link
Contributor Author

marc0s commented Nov 25, 2024

Is there something I can do about the failing CI checks?

Seems that some files are missing and that they're failing for other PRs...

@badlop
Copy link
Member

badlop commented Nov 25, 2024

Great PR, thanks!

Is there something I can do about the failing CI checks?

All the failed checks are produced by just one single error, reported by the Dialyzer static analysis tool. That small patch should solve it . Please apply to your code and resubmit:

diff --git a/src/econf.erl b/src/econf.erl
index f94d0db77..0ffd05e4e 100644
--- a/src/econf.erl
+++ b/src/econf.erl
@@ -511,7 +511,7 @@ db_type(M) ->
             ElixirModule = "Elixir." ++ atom_to_list(T),
             case code:ensure_loaded(list_to_atom(ElixirModule)) of
               {module, _} -> list_to_atom(ElixirModule);
-              {error, x} -> fail({bad_db_type, M, T})
+              {error, _} -> fail({bad_db_type, M, T})
             end
           end
       end).

Tell me if you prefer it to be included in the PR to be added to the sources (like mod_presence_demo.ex, for instance)

Yes, can you include it with this name lib/ejabberd_auth_example.ex? Don't worry if it doesn't work correctly, I'm planning a few improvements in that and other elixir files, will commit them after merging your PR.

@badlop badlop added this to the ejabberd 24.xx milestone Nov 25, 2024
@marc0s marc0s force-pushed the support-loading-elixir-modules-for-auth branch 2 times, most recently from 0075c75 to f0dc821 Compare November 25, 2024 13:39
Allow to specify an Elixir module name in `auth_method`.

If the referenced module, `M`,  cannot be loaded as `ejabberd_auth_M`,
try to load it as `Elixir.M`.
@marc0s marc0s force-pushed the support-loading-elixir-modules-for-auth branch from f0dc821 to 17b5b34 Compare November 25, 2024 14:31
@badlop
Copy link
Member

badlop commented Nov 25, 2024

Now that Dialyzer succeeds, the test progress and now the Elvis style reviewer complains:

# src/ejabberd.erl [FAIL]
  - no_space (https://github.com/inaka/elvis_core/tree/main/doc_rules/elvis_style/no_space.md)
    - Unexpected space to the left of "," on line 185

It refers to that line:

<<"Elixir" , _/binary>> ->  misc:binary_to_atom(hd(T));

Right now it has a space, for example <<"one" , "two">> and it is preferable to write <<"one", "two">>

@coveralls
Copy link

Coverage Status

coverage: 32.91% (-0.002%) from 32.912%
when pulling 17b5b34 on Quobis:support-loading-elixir-modules-for-auth
into c291c20 on processone:master.

@badlop badlop merged commit f400993 into processone:master Nov 25, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants