Merge pull request juju#17362 from ycliuhw/fix-secret-ci

juju#17362 This PR fixes two bugs, and a few secret-related bash test fixes: - fix UpdateUserSecret secret service method to support for updating secret metadata (config autoprune for example); - convert the secreterrors.SecretAccessScopeNotFound to apiservererrors.ErrPerm in GetSecretAccessScope facade method because the existing client side does not handle this error and also the permission denied error makes more sense for users. After this PR landed, below test should be fixed: - test-secrets_iaas-test-secrets-cmr-lxd - test-secrets_iaas-test-secrets-juju-lxd - test-secrets_iaas-test-secrets-vault-lxd - test-secrets_k8s-test-user-secrets-microk8s
manadart · May 9, 2024 · 7f380e6 · 7f380e6
2 parents 6cfce0f + 7d2b86f
commit 7f380e6
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 38 deletions.
diff --git a/apiserver/facades/controller/crossmodelsecrets/crossmodelsecrets.go b/apiserver/facades/controller/crossmodelsecrets/crossmodelsecrets.go
@@ -108,6 +108,9 @@ func (s *CrossModelSecretsAPI) getSecretAccessScope(ctx stdcontext.Context, arg
 
 	secretService := s.secretServiceGetter(uri.SourceUUID)
 	scopeTag, err := s.accessScope(ctx, secretService, uri, consumerUnit)
+	if errors.Is(err, secreterrors.SecretAccessScopeNotFound) {
+		return "", apiservererrors.ErrPerm
+	}
 	if err != nil {
 		return "", errors.Trace(err)
 	}

diff --git a/domain/secret/service/service.go b/domain/secret/service/service.go
@@ -285,10 +285,6 @@ func (s *SecretService) CreateCharmSecret(ctx context.Context, uri *secrets.URI,
 // the secret owner already has a secret with the same label.
 // It returns [secreterrors.PermissionDenied] if the secret cannot be managed by the accessor.
 func (s *SecretService) UpdateUserSecret(ctx context.Context, uri *secrets.URI, params UpdateUserSecretParams) (errOut error) {
-	if len(params.Data) == 0 {
-		return errors.NotValidf("empty secret value")
-	}
-
 	if err := s.canManage(ctx, uri, params.Accessor, nil); err != nil {
 		return errors.Trace(err)
 	}
@@ -305,44 +301,44 @@ func (s *SecretService) UpdateUserSecret(ctx context.Context, uri *secrets.URI,
 		for k, v := range params.Data {
 			p.Data[k] = v
 		}
-	}
-
-	err := s.loadBackendInfo(ctx, true)
-	if err != nil {
-		return errors.Trace(err)
-	}
-	// loadBackendInfo will error is there's no active backend.
-	backend := s.backends[s.activeBackendID]
+		err := s.loadBackendInfo(ctx, true)
+		if err != nil {
+			return errors.Trace(err)
+		}
+		// loadBackendInfo will error is there's no active backend.
+		backend := s.backends[s.activeBackendID]
 
-	md, err := s.GetSecret(ctx, uri)
-	if err != nil {
-		// Check if the uri exists or not.
-		return errors.Trace(err)
-	}
-	revId, err := backend.SaveContent(ctx, uri, md.LatestRevision+1, secrets.NewSecretValue(params.Data))
-	if err != nil && !errors.Is(err, errors.NotSupported) {
-		return errors.Annotatef(err, "saving secret content to backend")
-	}
-	if err == nil {
-		defer func() {
-			if errOut != nil {
-				// If we failed to update the secret, we should delete the
-				// secret value from the backend for the new revision.
-				if err2 := backend.DeleteContent(ctx, revId); err2 != nil &&
-					!errors.Is(err2, errors.NotSupported) &&
-					!errors.Is(err2, secreterrors.SecretRevisionNotFound) {
-					s.logger.Warningf("failed to delete secret %q: %v", revId, err2)
+		// TODO: use a bespoke "GetLatestRevision(ctx, uri) method instead of GetSecret().
+		md, err := s.GetSecret(ctx, uri)
+		if err != nil {
+			// Check if the uri exists or not.
+			return errors.Trace(err)
+		}
+		revId, err := backend.SaveContent(ctx, uri, md.LatestRevision+1, secrets.NewSecretValue(params.Data))
+		if err != nil && !errors.Is(err, errors.NotSupported) {
+			return errors.Annotatef(err, "saving secret content to backend")
+		}
+		if err == nil {
+			defer func() {
+				if errOut != nil {
+					// If we failed to update the secret, we should delete the
+					// secret value from the backend for the new revision.
+					if err2 := backend.DeleteContent(ctx, revId); err2 != nil &&
+						!errors.Is(err2, errors.NotSupported) &&
+						!errors.Is(err2, secreterrors.SecretRevisionNotFound) {
+						s.logger.Warningf("failed to delete secret %q: %v", revId, err2)
+					}
 				}
+			}()
+			p.Data = nil
+			p.ValueRef = &secrets.ValueRef{
+				BackendID:  s.activeBackendID,
+				RevisionID: revId,
 			}
-		}()
-		p.Data = nil
-		p.ValueRef = &secrets.ValueRef{
-			BackendID:  s.activeBackendID,
-			RevisionID: revId,
 		}
 	}
 
-	err = s.st.UpdateSecret(ctx, uri, p)
+	err := s.st.UpdateSecret(ctx, uri, p)
 	return errors.Annotatef(err, "updating user secret %q", uri.ID)
 }
 

diff --git a/tests/suites/secrets_iaas/cmr.sh b/tests/suites/secrets_iaas/cmr.sh
@@ -48,7 +48,7 @@ run_secrets_cmr() {
 	juju switch "model-secrets-offer"
 	juju suspend-relation "$relation_id"
 	juju switch "model-secrets-consume"
-	check_contains "$(juju exec --unit dummy-sink/0 -- secret-get "$secret_uri" 2>&1)" 'is not allowed to read this secret'
+	check_contains "$(juju exec --unit dummy-sink/0 -- secret-get "$secret_uri" 2>&1)" 'permission denied'
 	echo "Checking: resume relation and access is restored"
 	juju switch "model-secrets-offer"
 	juju resume-relation "$relation_id"
@@ -59,7 +59,7 @@ run_secrets_cmr() {
 	juju switch "model-secrets-offer"
 	juju exec --unit dummy-source/0 -- secret-revoke "$secret_uri" --relation "$relation_id"
 	juju switch "model-secrets-consume"
-	check_contains "$(juju exec --unit dummy-sink/0 -- secret-get "$secret_uri" 2>&1)" 'is not allowed to read this secret'
+	check_contains "$(juju exec --unit dummy-sink/0 -- secret-get "$secret_uri" 2>&1)" 'permission denied'
 }
 
 test_secrets_cmr() {