MB-61292: Enforce secret name uniqueness

Strictly speaking name uniqueness is not needed, but when names are
not unique it comlicates life of UI users a lot, so it seems like
it is easier to enforce it.

Change-Id: Ib69381b9010ad4e08c8091e5c6f63e49c1b1ad47
Reviewed-on: https://review.couchbase.org/c/ns_server/+/216783
Tested-by: Build Bot <[email protected]>
Reviewed-by: Navdeep S Boparai <[email protected]>
Well-Formed: Build Bot <[email protected]>
Tested-by: Timofey Barmin <[email protected]>

Author: timofey-barmin

apps/ns_server/src/cb_cluster_secrets.erl

@@ -57,7 +57,8 @@
          sync_with_all_node_monitors/0,
          new_key_id/0,
          is_valid_key_id/1,
-         dek_drop_complete/1]).
+         dek_drop_complete/1,
+         is_name_unique/3]).
 
 %% gen_server callbacks
 -export([init/1, handle_call/3, handle_cast/2, handle_info/2,
@@ -402,6 +403,12 @@ dek_drop_complete(DekKind) ->
     ?MODULE ! {dek_drop_complete, DekKind},
     ok.
 
+-spec is_name_unique(secret_id(), string(), chronicle_snapshot()) -> boolean().
+is_name_unique(Id, Name, Snapshot) ->
+    lists:all(fun (#{id := Id2}) when Id == Id2 -> true;
+                  (#{name := Name2}) -> Name /= Name2
+              end, get_all(Snapshot)).
+
 %%%===================================================================
 %%% gen_server callbacks
 %%%===================================================================
@@ -1121,7 +1128,8 @@ validate_secret_in_txn(NewProps, PrevProps, Snapshot) ->
         ok ?= validate_secrets_encryption_usage_change(NewProps, PrevProps,
                                                        Snapshot),
         ok ?= validate_dek_related_usage_change(NewProps, PrevProps, Snapshot),
-        ok ?= validate_encryption_secret_id(NewProps, Snapshot)
+        ok ?= validate_encryption_secret_id(NewProps, Snapshot),
+        ok ?= validate_name_uniqueness(NewProps, Snapshot)
     end.
 
 -spec execute_on_master({module(), atom(), [term()]}) -> term().
@@ -2029,6 +2037,14 @@ initiate_deks_drop(Kind, IdsToDrop, #state{deks = DeksInfo} = State0) ->
             State0
     end.
 
+-spec validate_name_uniqueness(secret_props(), chronicle_snapshot()) ->
+          ok | {error, name_not_unique}.
+validate_name_uniqueness(#{id := Id, name := Name}, Snapshot) ->
+    case is_name_unique(Id, Name, Snapshot) of
+        true -> ok;
+        false -> {error, name_not_unique}
+    end.
+
 -ifdef(TEST).
 replace_secret_in_list_test() ->
     ?assertEqual(false, replace_secret_in_list(#{id => 3, p => 5}, [])),

apps/ns_server/src/menelaus_web_secrets.erl

@@ -96,6 +96,9 @@ handle_put_secret(IdStr, Req) ->
                       {error, {encrypt_id, not_allowed}} ->
                           menelaus_util:reply_global_error(
                             Req, "encryption secret not allowed");
+                      {error, name_not_unique} ->
+                          menelaus_util:reply_global_error(
+                            Req, "name is not unique");
                       {error, not_found} ->
                           menelaus_util:reply_not_found(Req)
                   end
@@ -109,6 +112,17 @@ handle_put_secret(IdStr, Req) ->
 secret_validators(CurProps) ->
     [validator:string(name, _),
      validator:required(name, _),
+     validator:validate(
+       fun ("") -> {error, "Must not not be empty"};
+           (Str) ->
+               Id = maps:get(id, CurProps, ?SECRET_ID_NOT_SET),
+               case cb_cluster_secrets:is_name_unique(Id, Str, direct) of
+                   true -> ok;
+                   %% Checking it here and inside transaction later
+                   %% Check here is needed mostly to make it user friendly in UI
+                   false -> {error, "Must be unique"}
+               end
+       end, name, _),
      validator:one_of(type, [?GENERATED_KEY_TYPE, ?AWSKMS_KEY_TYPE], _),
      validator:convert(type, binary_to_atom(_, latin1), _),
      validator:required(type, _),

cluster_tests/testsets/native_encryption_tests.py

@@ -572,6 +572,7 @@ def cfg_encryption_api_test(self):
         secret = auto_generated_secret(usage=['bucket-encryption-*'])
         bad_id = create_secret(self.random_node(), secret)
         secret['usage'] = ['bucket-encryption-*', 'configuration-encryption']
+        secret['name'] = secret['name'] + ' (good)' # has to be unique
         good_id = create_secret(self.random_node(), secret)
         node = self.random_node()
         set_cfg_encryption(node, 'disabled', -1)