MB-61292: Add formatting for delete secret errors

timofey-barmin · timofey-barmin · commit 16fcc1554860 · 2024-09-26T20:43:24.000Z
Needed mostly for UI and future CLI Change-Id: Id8d4d66fa929b321ba6468d307567fe601970829 Reviewed-on: https://review.couchbase.org/c/ns_server/+/216782 Well-Formed: Build Bot <build@couchbase.com> Reviewed-by: Navdeep S Boparai <navdeep.boparai@couchbase.com> Tested-by: Timofey Barmin <timofey.barmin@couchbase.com>
diff --git a/apps/ns_server/src/cb_cluster_secrets.erl b/apps/ns_server/src/cb_cluster_secrets.erl
@@ -129,8 +129,9 @@
 -type bad_encrypt_id() :: {encrypt_id, not_allowed | not_found}.
 -type bad_usage_change() :: {usage, in_use}.
 
--type secret_in_use() :: {used_by, [{bucket, string()} |
-                                    {secret, secret_id()}]}.
+-type secret_in_use() :: {used_by, #{by_config := [cb_deks:dek_kind()],
+                                     by_secret := [secret_id()],
+                                     by_deks := [cb_deks:dek_kind()]}}.
 
 -type deks_info() :: #{active_id := cb_deks:dek_id() | undefined,
                        deks := [cb_deks:dek()],
@@ -1141,7 +1142,7 @@ can_delete_secret(#{id := Id}, Snapshot) ->
                case call_dek_callback(encryption_method_callback, Kind,
                                       [Snapshot]) of
                   {succ, {ok, {secret, Id}}} ->
-                      {true, maps:get(name, cb_deks:dek_config(Kind))};
+                      {true, Kind};
                   {succ, {ok, _}} ->
                       false;
                   {succ, {error, not_found}} ->
@@ -1153,12 +1154,19 @@ can_delete_secret(#{id := Id}, Snapshot) ->
     %% Places where this secret is used to encrypt deks (such deks can exist
     %% even if encryption is disabled for this entity)
     Deks = get_dek_kinds_used_by_secret_id(Id, Snapshot),
-    AllEntities =
-        EncryptionConfigUsages ++ [{secret, SId} || SId <- Secrets] ++
-        [{deks, D} || D <- Deks],
-    case AllEntities of
-        [] -> ok;
-        _ -> {error, {used_by, AllEntities}}
+    SecretNames =
+        lists:map(fun (SId) ->
+                      {ok, #{name := SName}} = get_secret(SId, Snapshot),
+                      SName
+                  end, Secrets),
+
+    case length(EncryptionConfigUsages) + length(SecretNames) + length(Deks) of
+        0 -> ok;
+        _ ->
+            M = #{by_config => EncryptionConfigUsages,
+                  by_secrets => SecretNames,
+                  by_deks => Deks},
+            {error, {used_by, M}}
     end.
 
 -spec get_secrets_used_by_secret_id(secret_id(), chronicle_snapshot()) ->
diff --git a/apps/ns_server/src/cb_deks.erl b/apps/ns_server/src/cb_deks.erl
@@ -316,8 +316,7 @@ dek_chronicle_keys_filter(Key) ->
 %% required_usage - the secret usage that secret must contain in order to be
 %% allowed to encrypt this kind of deks
 dek_config(chronicleDek) ->
-    #{name => chronicle,
-      encryption_method_callback => cb_crypto:get_encryption_method(
+    #{encryption_method_callback => cb_crypto:get_encryption_method(
                                       config_encryption, _),
       set_active_key_callback => fun chronicle_local:set_active_dek/1,
       lifetime_callback => cb_crypto:get_dek_kind_lifetime(
@@ -329,8 +328,7 @@ dek_config(chronicleDek) ->
       chronicle_txn_keys => [?CHRONICLE_ENCR_AT_REST_SETTINGS_KEY],
       required_usage => config_encryption};
 dek_config(configDek) ->
-    #{name => configuration,
-      encryption_method_callback => cb_crypto:get_encryption_method(
+    #{encryption_method_callback => cb_crypto:get_encryption_method(
                                       config_encryption, _),
       set_active_key_callback => fun set_config_active_key/1,
       lifetime_callback => cb_crypto:get_dek_kind_lifetime(
@@ -342,8 +340,7 @@ dek_config(configDek) ->
       chronicle_txn_keys => [?CHRONICLE_ENCR_AT_REST_SETTINGS_KEY],
       required_usage => config_encryption};
 dek_config({bucketDek, Bucket}) ->
-    #{name => {bucket, Bucket},
-      encryption_method_callback => ns_bucket:get_encryption(Bucket, _),
+    #{encryption_method_callback => ns_bucket:get_encryption(Bucket, _),
       set_active_key_callback => ns_memcached:set_active_dek_for_bucket(Bucket,
                                                                         _),
       lifetime_callback => ns_bucket:get_dek_lifetime(Bucket, _),
diff --git a/apps/ns_server/src/menelaus_web_secrets.erl b/apps/ns_server/src/menelaus_web_secrets.erl
@@ -15,6 +15,9 @@
 -include("ns_common.hrl").
 -include_lib("ns_common/include/cut.hrl").
 -include("cb_cluster_secrets.hrl").
+-ifdef(TEST).
+-include_lib("eunit/include/eunit.hrl").
+-endif.
 
 -export([handle_get_secrets/1,
          handle_get_secret/2,
@@ -125,9 +128,10 @@ handle_delete_secret(IdStr, Req) ->
             menelaus_util:reply(Req, 200);
         {error, not_found} ->
             menelaus_util:reply_not_found(Req);
-        {error, Reason} ->
+        {error, {used_by, UsedByList}} ->
+            Formatted = format_secrets_used_by_list(UsedByList),
             menelaus_util:reply_global_error(
-              Req, io_lib:format("Error: ~p", [Reason]))
+              Req, io_lib:format("Can't be removed because ~s", [Formatted]))
     end.
 
 handle_rotate(IdStr, Req) ->
@@ -402,3 +406,84 @@ parse_id(Str) when is_list(Str) ->
         _:_ ->
             menelaus_util:web_exception(404, menelaus_util:reply_text_404())
     end.
+
+format_secrets_used_by_list(UsedByMap) ->
+    UsedByCfg = maps:get(by_config, UsedByMap, []),
+    Secrets = maps:get(by_secrets, UsedByMap, []),
+    UsedByDeks = maps:get(by_deks, UsedByMap, []),
+
+    FormatUsages =
+        fun (Usages) ->
+            {Buckets, Other} = misc:partitionmap(
+                                 fun ({bucket_encryption, B}) -> {left, B};
+                                     (K) -> {right, K}
+                                 end, Usages),
+            FormattedUsages = lists:map(fun (config_encryption) ->
+                                            "configuration"
+                                        end, Other),
+            Buckets2 = ["\"" ++ B ++ "\"" || B <- Buckets],
+            BucketsStr =
+                case length(Buckets) of
+                    0 -> [];
+                    1 -> ["bucket " ++ Buckets2];
+                    _ -> ["buckets " ++ lists:join(", ", Buckets2)]
+                end,
+            FormattedUsages ++ BucketsStr
+        end,
+    Kind2Usage = ?cut(maps:get(required_usage, cb_deks:dek_config(_))),
+    UsagesUsedByCfg = lists:uniq(lists:map(Kind2Usage, UsedByCfg)),
+    UsagesUsedByDeks = lists:uniq(lists:map(Kind2Usage, UsedByDeks)),
+    SecretsStrs = ["secrets " ++ lists:join(", ", Secrets) || Secrets /= []],
+    Strings1 = FormatUsages(UsagesUsedByCfg) ++ SecretsStrs,
+    Strings2 = FormatUsages(UsagesUsedByDeks -- UsagesUsedByCfg),
+
+    case {Strings1, Strings2} of
+        {_, []} ->
+            "this secret is configured to encrypt " ++
+            lists:join(", ", Strings1);
+        {[], _} ->
+            "this secret still encrypts some data in " ++
+            lists:join(", ", Strings2);
+        {_ , _} ->
+            "this secret is configured to encrypt " ++
+            lists:join(", ", Strings1) ++
+            "; it also still encrypts some data in " ++
+            lists:join(", ", Strings2)
+    end.
+
+-ifdef(TEST).
+
+format_secrets_used_by_list_test() ->
+    All = cb_deks:dek_kinds_list(#{bucket_names => {["b1", "b2"], 1}}),
+    Secrets = ["s1", "s2"],
+    F = ?cut(lists:flatten(format_secrets_used_by_list(_))),
+    ?assertEqual("this secret is configured to encrypt configuration, "
+                 "buckets \"b1\", \"b2\"",
+                 F(#{by_deks => All, by_config => All, by_secrets => []})),
+    ?assertEqual("this secret is configured to encrypt configuration, "
+                 "buckets \"b1\", \"b2\"",
+                 F(#{by_deks => [], by_config => All, by_secrets => []})),
+    ?assertEqual("this secret still encrypts some data in configuration, "
+                 "buckets \"b1\", \"b2\"",
+                 F(#{by_deks => All, by_config => [], by_secrets => []})),
+    ?assertEqual("this secret is configured to encrypt secrets s1, s2",
+                 F(#{by_deks => [], by_config => [], by_secrets => Secrets})),
+    ?assertEqual("this secret is configured to encrypt configuration, "
+                 "buckets \"b1\", \"b2\", secrets s1, s2",
+                 F(#{by_deks => [], by_config => All, by_secrets => Secrets})),
+    ?assertEqual("this secret is configured to encrypt configuration, "
+                 "buckets \"b1\", \"b2\", secrets s1, s2",
+                 F(#{by_deks => All, by_config => All, by_secrets => Secrets})),
+    ?assertEqual("this secret is configured to encrypt configuration; it also "
+                 "still encrypts some data in buckets \"b1\", \"b2\"",
+                 F(#{by_deks => All, by_config => [configDek],
+                     by_secrets => []})),
+    ?assertEqual("this secret is configured to encrypt configuration, "
+                 "bucket \"b2\", secrets s1, s2; "
+                 "it also still encrypts some data in bucket \"b1\"",
+                 F(#{by_deks => [configDek, {bucketDek, "b1"}],
+                     by_config => [{bucketDek, "b2"}, chronicleDek],
+                     by_secrets => Secrets})).
+
+
+-endif.
