Keep ACL conditions as maps #3501
Conversation
This comment has been minimized.
This comment has been minimized.
chrzaszcz
force-pushed
the
acl-spec-map
branch
from
3c11b78
to
3f66cb1
Compare
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #3501 +/- ##
==========================================
- Coverage 81.00% 80.94% -0.06%
==========================================
Files 418 418
Lines 32329 32318 -11
==========================================
- Hits 26187 26160 -27
- Misses 6142 6158 +16
Continue to review full report at Codecov.
|
chrzaszcz
force-pushed
the
acl-spec-map
branch
from
3f66cb1
to
b6bd1b7
Compare
Functional changes:
- More possibilities to combine rules
- Domain check is now explicit with #{match => all}
- Domain can't be 'global' anymore,
because user's one can be passed instead with the same effect
This comment has been minimized.
This comment has been minimized.
chrzaszcz
force-pushed
the
acl-spec-map
branch
from
b6bd1b7
to
9700a47
Compare
This comment has been minimized.
This comment has been minimized.
Maps are now handled in acl.erl 'nodeprep' for all values is kept as it seems to be good enough.
The name was updated as the functions were unified.
The module does not support dynamic domains yet, but the host type can be safely obtained from the server host.
The host type can be safely obtained here.
This is now used: - By s2s when the "user" is actually another server. - By components when the "server" is a component. In both cases the 'current_domain' check would never succeed, so the user would always have to add 'match = "all"' to the ACL.
Expect the default 'match => current_domain' condition as well.
chrzaszcz
force-pushed
the
acl-spec-map
branch
from
9700a47
to
2adbdf6
Compare
This comment has been minimized.
This comment has been minimized.
s2s tests need domain to host type resolution now
It is not needed, the pattern is valid without it.
The main change is the default condition 'match = "current_domain"', which was implicit and hard to control before the changes.
chrzaszcz
force-pushed
the
acl-spec-map
branch
from
2adbdf6
to
971e1c5
Compare
This comment has been minimized.
This comment has been minimized.
|
small_tests_24 / small_tests / 85bb20b small_tests_23 / small_tests / 85bb20b dynamic_domains_mysql_redis_24 / mysql_redis / 85bb20b dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 85bb20b dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 85bb20b dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 85bb20b ldap_mnesia_23 / ldap_mnesia / 85bb20b ldap_mnesia_24 / ldap_mnesia / 85bb20b internal_mnesia_24 / internal_mnesia / 85bb20b pgsql_mnesia_23 / pgsql_mnesia / 85bb20b pgsql_mnesia_24 / pgsql_mnesia / 85bb20b mysql_redis_24 / mysql_redis / 85bb20b elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 85bb20b mssql_mnesia_24 / odbc_mssql_mnesia / 85bb20b riak_mnesia_24 / riak_mnesia / 85bb20b |
There was a problem hiding this comment.
It seems very well written to me, got a better idea about what ACLs are, so this is already a win :)
I have one comment for something I suspect might be unsafe and this could be a good opportunity to improve.
I also see some places for micro performance improvements, like, non-compiled regexes, or more common, when we do match_acl for a non-global host-type, we fetch the acl lists for both the host-type and global, and do ++, which, if the lists are big, can incur into a lot of garbage being generated. I do mind a bit because this kind of code is run everywhere in all sorts of tight loops, so I just wanted to raise awareness for potentially slow code. Nevertheless this was already like that (I just never explored this code much), it is not related to the scope of this task, and we can work on that later when we get some client deployment using ACLs and asking for improvements 🚀
| case acl:match_rule(HostType, LServer, max_user_sessions, JID) of | ||
| Max when is_integer(Max) -> Max; |
There was a problem hiding this comment.
This just makes me realise, I think we're not checking that this number is strictly positive, right? 🤔
There was a problem hiding this comment.
We don't, that's right, it might be good to fix next.
Regarding the '++' on host and global rules, I am rewriting this right now so that it will be precomputed per host type. Expect a PR today 🙂
| case acl:match_rule( | ||
| From, max_s2s_connections, jid:make(<<"">>, To, <<"">>)) of | ||
| {ok, HostType} = mongoose_domain_api:get_host_type(From), | ||
| case acl:match_rule(HostType, max_s2s_connections, jid:make(<<"">>, To, <<"">>)) of |
There was a problem hiding this comment.
Oh, it was already there... 🙈
The goal is to simplify and unify the internal ACL format and the ACL matching logic.
Main changes:
{user = "alice", server = "localhost", resource = "res"}, which was unsupported before.{user = "alice"}but not for{resource = "res"}, which was counter-intuitive and undocumented. Now the check is calledmatch = "current_domain"and it is enabled by default unless the user disables it withmatch = "all".Not changed:
nodeprep. It is sketchy, but it passes the tests, so I left it unchanged for the sake of brevity.