fix(variform and authz): do not initialize empty client_attrs field

when client_attrs_init expression renders to empty string,
do not initialize the attribute.

also fixed an ACL error: a template render failure for a topic
would stop the ACL checks for the following topics if more
than one topic is configured.

apps/emqx/src/emqx_channel.erl

@@ -1638,6 +1638,15 @@ initialize_client_attrs(Inits, ClientInfo) ->
         fun(#{expression := Variform, set_as_attr := Name}, Acc) ->
             Attrs = maps:get(client_attrs, ClientInfo, #{}),
             case emqx_variform:render(Variform, ClientInfo) of
+                {ok, <<>>} ->
+                    ?SLOG(
+                        debug,
+                        #{
+                            msg => "client_attr_rednered_to_empty_string",
+                            set_as_attr => Name
+                        }
+                    ),
+                    Acc;
                 {ok, Value} ->
                     ?SLOG(
                         debug,

apps/emqx_auth/etc/acl.conf

@@ -6,8 +6,10 @@
 
 {deny, all, subscribe, ["$SYS/#", {eq, "#"}]}.
 
-%% NOTE: change to {deny, all} in production!
 {allow, all}.
+%% NOTE! when deploy in production:
+%% - Change the last rule to `{deny, all}.`
+%% - Set config `authorization.no_match = deny`
 
 %% See docs below
 %%

apps/emqx_auth/src/emqx_authz/emqx_authz_rule.erl

@@ -351,8 +351,9 @@ match_who(_, _) ->
 match_topics(_ClientInfo, _Topic, []) ->
     false;
 match_topics(ClientInfo, Topic, [{pattern, PatternFilter} | Filters]) ->
-    TopicFilter = bin(emqx_template:render_strict(PatternFilter, ClientInfo)),
-    match_topic(emqx_topic:words(Topic), emqx_topic:words(TopicFilter)) orelse
+    TopicFilter = render_topic(PatternFilter, ClientInfo),
+    (is_binary(TopicFilter) andalso
+        match_topic(emqx_topic:words(Topic), emqx_topic:words(TopicFilter))) orelse
         match_topics(ClientInfo, Topic, Filters);
 match_topics(ClientInfo, Topic, [TopicFilter | Filters]) ->
     match_topic(emqx_topic:words(Topic), TopicFilter) orelse
@@ -362,3 +363,16 @@ match_topic(Topic, {'eq', TopicFilter}) ->
     Topic =:= TopicFilter;
 match_topic(Topic, TopicFilter) ->
     emqx_topic:match(Topic, TopicFilter).
+
+render_topic(Topic, ClientInfo) ->
+    try
+        bin(emqx_template:render_strict(Topic, ClientInfo))
+    catch
+        error:Reason ->
+            ?SLOG(debug, #{
+                msg => "failed_to_render_topic_template",
+                template => Topic,
+                reason => Reason
+            }),
+            error
+    end.

apps/emqx_auth/test/emqx_authz/emqx_authz_SUITE.erl

@@ -173,7 +173,16 @@ end_per_testcase(_TestCase, _Config) ->
 -define(SOURCE_FILE_CLIENT_ATTR,
     ?SOURCE_FILE(
         <<
-            "{allow,all,all,[\"${client_attrs.alias}/#\"]}.\n"
+            "{allow,all,all,[\"${client_attrs.alias}/#\",\"client_attrs_backup\"]}.\n"
+            "{deny, all}."
+        >>
+    )
+).
+
+-define(SOURCE_FILE_CLIENT_NO_SUCH_ATTR,
+    ?SOURCE_FILE(
+        <<
+            "{allow,all,all,[\"${client_attrs.nonexist}/#\",\"client_attrs_backup\"]}.\n"
             "{deny, all}."
         >>
     )
@@ -572,11 +581,41 @@ t_alias_prefix(_Config) ->
     ?assertMatch({ok, _}, emqtt:connect(C)),
     ?assertMatch({ok, _, [?RC_SUCCESS]}, emqtt:subscribe(C, SubTopic)),
     ?assertMatch({ok, _, [?RC_NOT_AUTHORIZED]}, emqtt:subscribe(C, SubTopicNotAllowed)),
+    ?assertMatch({ok, _, [?RC_NOT_AUTHORIZED]}, emqtt:subscribe(C, <<"/#">>)),
     unlink(C),
     emqtt:stop(C),
+    NonMatching = <<"clientid_which_has_no_dash">>,
+    {ok, C2} = emqtt:start_link([{clientid, NonMatching}, {proto_ver, v5}]),
+    ?assertMatch({ok, _}, emqtt:connect(C2)),
+    ?assertMatch({ok, _, [?RC_SUCCESS]}, emqtt:subscribe(C2, <<"client_attrs_backup">>)),
+    %% assert '${client_attrs.alias}/#' is not rendered as '/#'
+    ?assertMatch({ok, _, [?RC_NOT_AUTHORIZED]}, emqtt:subscribe(C2, <<"/#">>)),
+    unlink(C2),
+    emqtt:stop(C2),
     emqx_config:put_zone_conf(default, [mqtt, client_attrs_init], []),
     ok.
 
+t_non_existing_attr(_Config) ->
+    {ok, _} = emqx_authz:update(?CMD_REPLACE, [?SOURCE_FILE_CLIENT_NO_SUCH_ATTR]),
+    %% '^.*-(.*)$': extract the suffix after the last '-'
+    {ok, Compiled} = emqx_variform:compile("concat(regex_extract(clientid,'^.*-(.*)$'))"),
+    emqx_config:put_zone_conf(default, [mqtt, client_attrs_init], [
+        #{
+            expression => Compiled,
+            %% this is intended to be different from 'nonexist'
+            set_as_attr => <<"existing">>
+        }
+    ]),
+    ClientId = <<"org1-name3">>,
+    {ok, C} = emqtt:start_link([{clientid, ClientId}, {proto_ver, v5}]),
+    ?assertMatch({ok, _}, emqtt:connect(C)),
+    ?assertMatch({ok, _, [?RC_SUCCESS]}, emqtt:subscribe(C, <<"client_attrs_backup">>)),
+    %% assert '${client_attrs.nonexist}/#' is not rendered as '/#'
+    ?assertMatch({ok, _, [?RC_NOT_AUTHORIZED]}, emqtt:subscribe(C, <<"/#">>)),
+    unlink(C),
+    emqtt:stop(C),
+    ok.
+
 %% client is allowed by ACL to publish to its LWT topic, is connected,
 %% and then gets banned and kicked out while connected.  Should not
 %% publish LWT.