Merge pull request #13196 from lafirest/fix/rules_len

fix(authz_mnesia): add a soft limit in the API for the length of ACL rules

apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl

@@ -469,8 +469,8 @@ users(get, #{query_string := QueryString}) ->
             {200, Result}
     end;
 users(post, #{body := Body}) when is_list(Body) ->
-    case ensure_all_not_exists(<<"username">>, username, Body) of
-        [] ->
+    case ensure_rules_is_valid(<<"username">>, username, Body) of
+        ok ->
             lists:foreach(
                 fun(#{<<"username">> := Username, <<"rules">> := Rules}) ->
                     emqx_authz_mnesia:store_rules({username, Username}, Rules)
@@ -478,10 +478,19 @@ users(post, #{body := Body}) when is_list(Body) ->
                 Body
             ),
             {204};
-        Exists ->
+        {error, {Username, too_many_rules}} ->
+            {400, #{
+                code => <<"BAD_REQUEST">>,
+                message =>
+                    binfmt(
+                        <<"The rules length of User '~ts' exceeds the maximum limit.">>,
+                        [Username]
+                    )
+            }};
+        {error, {already_exists, Exists}} ->
             {409, #{
                 code => <<"ALREADY_EXISTS">>,
-                message => binfmt("Users '~ts' already exist", [binjoin(Exists)])
+                message => binfmt("User '~ts' already exist", [Exists])
             }}
     end.
 
@@ -507,8 +516,8 @@ clients(get, #{query_string := QueryString}) ->
             {200, Result}
     end;
 clients(post, #{body := Body}) when is_list(Body) ->
-    case ensure_all_not_exists(<<"clientid">>, clientid, Body) of
-        [] ->
+    case ensure_rules_is_valid(<<"clientid">>, clientid, Body) of
+        ok ->
             lists:foreach(
                 fun(#{<<"clientid">> := ClientID, <<"rules">> := Rules}) ->
                     emqx_authz_mnesia:store_rules({clientid, ClientID}, Rules)
@@ -516,10 +525,19 @@ clients(post, #{body := Body}) when is_list(Body) ->
                 Body
             ),
             {204};
-        Exists ->
+        {error, {ClientId, too_many_rules}} ->
+            {400, #{
+                code => <<"BAD_REQUEST">>,
+                message =>
+                    binfmt(
+                        <<"The rules length of Client '~ts' exceeds the maximum limit.">>,
+                        [ClientId]
+                    )
+            }};
+        {error, {already_exists, Exists}} ->
             {409, #{
                 code => <<"ALREADY_EXISTS">>,
-                message => binfmt("Clients '~ts' already exist", [binjoin(Exists)])
+                message => binfmt("Client '~ts' already exist", [Exists])
             }}
     end.
 
@@ -583,8 +601,17 @@ all(get, _) ->
             }}
     end;
 all(post, #{body := #{<<"rules">> := Rules}}) ->
-    emqx_authz_mnesia:store_rules(all, Rules),
-    {204};
+    case ensure_rules_len(Rules) of
+        ok ->
+            emqx_authz_mnesia:store_rules(all, Rules),
+            {204};
+        _ ->
+            {400, #{
+                code => <<"BAD_REQUEST">>,
+                message =>
+                    <<"The length of rules exceeds the maximum limit.">>
+            }}
+    end;
 all(delete, _) ->
     emqx_authz_mnesia:store_rules(all, []),
     {204}.
@@ -700,28 +727,45 @@ rules_example({ExampleName, ExampleType}) ->
         }
     }.
 
-ensure_all_not_exists(Key, Type, Cfgs) ->
-    lists:foldl(
-        fun(#{Key := Id}, Acc) ->
-            case emqx_authz_mnesia:get_rules({Type, Id}) of
-                not_found ->
-                    Acc;
-                _ ->
-                    [Id | Acc]
-            end
-        end,
-        [],
-        Cfgs
+ensure_rules_len(Rules) ->
+    emqx_authz_api_sources:with_source(
+        ?AUTHZ_TYPE_BIN,
+        fun(#{<<"max_rules">> := MaxLen}) ->
+            ensure_rules_len(Rules, MaxLen)
+        end
     ).
 
-binjoin([Bin]) ->
-    Bin;
-binjoin(Bins) ->
-    binjoin(Bins, <<>>).
+ensure_rules_len(Rules, MaxLen) ->
+    case erlang:length(Rules) =< MaxLen of
+        true ->
+            ok;
+        _ ->
+            {error, too_many_rules}
+    end.
 
-binjoin([H | T], Acc) ->
-    binjoin(T, <<H/binary, $,, Acc/binary>>);
-binjoin([], Acc) ->
-    Acc.
+ensure_rules_is_valid(Key, Type, Cfgs) ->
+    MaxLen = emqx_authz_api_sources:with_source(
+        ?AUTHZ_TYPE_BIN,
+        fun(#{<<"max_rules">> := MaxLen}) ->
+            MaxLen
+        end
+    ),
+    ensure_rules_is_valid(Key, Type, MaxLen, Cfgs).
+
+ensure_rules_is_valid(Key, Type, MaxLen, [Cfg | Cfgs]) ->
+    #{Key := Id, <<"rules">> := Rules} = Cfg,
+    case emqx_authz_mnesia:get_rules({Type, Id}) of
+        not_found ->
+            case ensure_rules_len(Rules, MaxLen) of
+                ok ->
+                    ensure_rules_is_valid(Key, Type, MaxLen, Cfgs);
+                {error, Reason} ->
+                    {error, {Id, Reason}}
+            end;
+        _ ->
+            {error, {already_exists, Id}}
+    end;
+ensure_rules_is_valid(_Key, _Type, _MaxLen, []) ->
+    ok.
 
 binfmt(Fmt, Args) -> iolist_to_binary(io_lib:format(Fmt, Args)).

apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl

@@ -30,12 +30,24 @@
     namespace/0
 ]).
 
+-define(MAX_RULES, 100).
+
 namespace() -> "authz".
 
 type() -> ?AUTHZ_TYPE.
 
 fields(builtin_db) ->
-    emqx_authz_schema:authz_common_fields(?AUTHZ_TYPE).
+    emqx_authz_schema:authz_common_fields(?AUTHZ_TYPE) ++
+        [
+            {max_rules,
+                ?HOCON(
+                    pos_integer(),
+                    #{
+                        default => ?MAX_RULES,
+                        desc => ?DESC(max_rules)
+                    }
+                )}
+        ].
 
 source_refs() ->
     [?R_REF(builtin_db)].

apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl

@@ -36,7 +36,7 @@ init_per_suite(Config) ->
             {emqx_conf,
                 "authorization.cache { enable = false },"
                 "authorization.no_match = deny,"
-                "authorization.sources = [{type = built_in_database}]"},
+                "authorization.sources = [{type = built_in_database, max_rules = 5}]"},
             emqx,
             emqx_auth,
             emqx_auth_mnesia,
@@ -66,6 +66,14 @@ t_api(_) ->
             [?USERNAME_RULES_EXAMPLE]
         ),
 
+    %% check length limit
+    {ok, 400, _} =
+        request(
+            post,
+            uri(["authorization", "sources", "built_in_database", "rules", "users"]),
+            [dup_rules_example(?USERNAME_RULES_EXAMPLE)]
+        ),
+
     {ok, 409, _} =
         request(
             post,
@@ -171,6 +179,13 @@ t_api(_) ->
             [?CLIENTID_RULES_EXAMPLE]
         ),
 
+    {ok, 400, _} =
+        request(
+            post,
+            uri(["authorization", "sources", "built_in_database", "rules", "clients"]),
+            [dup_rules_example(?CLIENTID_RULES_EXAMPLE)]
+        ),
+
     {ok, 409, _} =
         request(
             post,
@@ -238,6 +253,14 @@ t_api(_) ->
             uri(["authorization", "sources", "built_in_database", "rules", "all"]),
             ?ALL_RULES_EXAMPLE
         ),
+
+    {ok, 400, _} =
+        request(
+            post,
+            uri(["authorization", "sources", "built_in_database", "rules", "all"]),
+            dup_rules_example(?ALL_RULES_EXAMPLE)
+        ),
+
     {ok, 200, Request7} =
         request(
             get,
@@ -491,3 +514,10 @@ replace_parts(Parts, Replacements) ->
         end,
         Parts
     ).
+
+dup_rules_example(#{username := _, rules := Rules}) ->
+    #{username => user2, rules => Rules ++ Rules};
+dup_rules_example(#{clientid := _, rules := Rules}) ->
+    #{clientid => client2, rules => Rules ++ Rules};
+dup_rules_example(#{rules := Rules}) ->
+    #{rules => Rules ++ Rules}.

changes/ce/fix-13196.en.md

@@ -0,0 +1 @@
+In the built-in database of authorization, added a limit for the number of rules per client/user, and the default values is 100.

rel/i18n/emqx_authz_mnesia_schema.hocon

@@ -6,4 +6,7 @@ builtin_db.desc:
 builtin_db.label:
 """Builtin Database"""
 
+max_rules.desc:
+"""Maximum number of rules per client/user. Note that performance may decrease as number of rules increases."""
+
 }