Merge pull request #571 from zigbee-alliance/improve-error-message-texts

Improve error message texts

types/pki/errors.go

@@ -75,7 +75,7 @@ func NewErrProposedCertificateDoesNotExist(subject string, subjectKeyID string)
 	return errors.Wrapf(ErrProposedCertificateDoesNotExist,
 		"No proposed X509 root certificate associated "+
 			"with the combination of subject=%v and subjectKeyID=%v on the ledger. "+
-			"The cerificate either does not exists or already approved.",
+			"The certificate either does not exists, already approved or rejected",
 		subject, subjectKeyID)
 }
 
@@ -115,11 +115,15 @@ func NewErrProposedCertificateRevocationAlreadyExists(subject string, subjectKey
 		subject, subjectKeyID)
 }
 
-func NewErrProposedCertificateRevocationDoesNotExist(subject string, subjectKeyID string) error {
+func NewErrProposedCertificateRevocationDoesNotExist(subject string, subjectKeyID string, serialNumber string) error {
+	if serialNumber != "" {
+		serialNumber = " and serialNumber=" + serialNumber
+	}
+
 	return errors.Wrapf(ErrProposedCertificateRevocationDoesNotExist,
 		"No proposed X509 root certificate revocation associated "+
-			"with the combination of subject=%v and subjectKeyID=%v on the ledger.",
-		subject, subjectKeyID)
+			"with the combination of subject=%v, subjectKeyID=%v%v on the ledger.",
+		subject, subjectKeyID, serialNumber)
 }
 
 func NewErrRevokedCertificateDoesNotExist(subject string, subjectKeyID string) error {
@@ -235,6 +239,20 @@ func NewErrRootCertVidNotEqualToAccountVid(rootVID int32, accountVID int32) erro
 		rootVID, accountVID)
 }
 
+func NewErrRevokeRootCertVidNotEqualToAccountVid(rootVID int32, accountVID int32) error {
+	return errors.Wrapf(ErrCertVidNotEqualAccountVid,
+		"Only a Vendor associated with VID of root certificate can revoke certificate: "+
+			"Root certificate's VID = %v, Account VID = %v",
+		rootVID, accountVID)
+}
+
+func NewErrRevokeCertVidNotEqualToAccountVid(rootVID int32, accountVID int32) error {
+	return errors.Wrapf(ErrCertVidNotEqualAccountVid,
+		"Only a Vendor associated with VID of certificate can revoke certificate: "+
+			"Certificate's VID = %v, Account VID = %v",
+		rootVID, accountVID)
+}
+
 func NewErrCRLSignerCertificateInvalidFormat(description string) error {
 	return errors.Wrapf(
 		ErrCRLSignerCertificateInvalidFormat, "Invalid CRL Signer Certificate format: %v",

x/pki/handler_add_noc_cert_test.go

@@ -224,7 +224,7 @@ func TestHandler_AddXNoc509Cert_WhenNocRootCertIsAbsent(t *testing.T) {
 	addNocX509Cert := types.NewMsgAddNocX509IcaCert(accAddress.String(), testconstants.NocCert1, testconstants.CertSchemaVersion, testconstants.SchemaVersion)
 	_, err := setup.Handler(setup.Ctx, addNocX509Cert)
 
-	require.ErrorIs(t, err, pkitypes.ErrInvalidCertificate)
+	require.ErrorIs(t, err, pkitypes.ErrCertificateDoesNotExist)
 }
 
 func TestHandler_AddNocX509Cert_CertificateExist(t *testing.T) {

x/pki/handler_add_non_root_cert_test.go

@@ -235,7 +235,7 @@ func TestHandler_AddX509Cert_ForAbsentDirectParentCert(t *testing.T) {
 	// add intermediate x509 certificate
 	addX509Cert := types.NewMsgAddX509Cert(vendorAccAddress.String(), testconstants.IntermediateCertPem, testconstants.CertSchemaVersion, testconstants.SchemaVersion)
 	_, err := setup.Handler(setup.Ctx, addX509Cert)
-	require.ErrorIs(t, err, pkitypes.ErrInvalidCertificate)
+	require.ErrorIs(t, err, pkitypes.ErrCertificateDoesNotExist)
 }
 
 func TestHandler_AddX509Cert_ForFailedCertificateVerification(t *testing.T) {

x/pki/handler_remove_noc_ica_cert_test.go

@@ -436,7 +436,7 @@ func TestHandler_RemoveNocX509IcaCert_ByOtherVendor(t *testing.T) {
 		vendorAccAddress2.String(), testconstants.NocCert1Subject, testconstants.NocCert1SubjectKeyID, testconstants.NocCert1SerialNumber)
 	_, err = setup.Handler(setup.Ctx, removeIcaCert)
 	require.Error(t, err)
-	require.True(t, pkitypes.ErrCertificateDoesNotExist.Is(err))
+	require.True(t, pkitypes.ErrCertVidNotEqualAccountVid.Is(err))
 }
 
 func TestHandler_RemoveNocX509IcaCert_SenderNotVendor(t *testing.T) {

x/pki/handler_remove_noc_root_cert_test.go

@@ -491,7 +491,7 @@ func TestHandler_RemoveNocX509RootCert_ByOtherVendor(t *testing.T) {
 		vendorAccAddress2.String(), testconstants.NocRootCert1Subject, testconstants.NocRootCert1SubjectKeyID, testconstants.NocRootCert1SerialNumber)
 	_, err := setup.Handler(setup.Ctx, removeIcaCert)
 	require.Error(t, err)
-	require.True(t, pkitypes.ErrCertificateDoesNotExist.Is(err))
+	require.True(t, pkitypes.ErrCertVidNotEqualAccountVid.Is(err))
 }
 
 func TestHandler_RemoveNocX509RootCert_SenderNotVendor(t *testing.T) {

x/pki/keeper/approved_certificates.go

@@ -129,9 +129,7 @@ func (k Keeper) verifyCertificate(ctx sdk.Context,
 	} else {
 		parentCertificates, found := k.GetApprovedCertificates(ctx, x509Certificate.Issuer, x509Certificate.AuthorityKeyID)
 		if !found {
-			return nil, pkitypes.NewErrInvalidCertificate(
-				fmt.Sprintf("Certificate verification failed for certificate with subject=%v and subjectKeyID=%v",
-					x509Certificate.Subject, x509Certificate.SubjectKeyID))
+			return nil, pkitypes.NewErrRootCertificateDoesNotExist(x509Certificate.Issuer, x509Certificate.AuthorityKeyID)
 		}
 
 		for _, cert := range parentCertificates.Certs {

x/pki/keeper/msg_server_add_noc_x_509_ica_cert.go

@@ -82,7 +82,7 @@ func (k msgServer) AddNocX509IcaCert(goCtx context.Context, msg *types.MsgAddNoc
 	}
 	// Check VID scoping
 	if nocRootCert.Vid != accountVid {
-		return nil, pkitypes.NewErrRootCertVidNotEqualToAccountVid(accountVid, nocRootCert.Vid)
+		return nil, pkitypes.NewErrRootCertVidNotEqualToAccountVid(nocRootCert.Vid, accountVid)
 	}
 
 	// create new certificate

x/pki/keeper/msg_server_approve_revoke_x_509_root_cert.go

@@ -29,7 +29,7 @@ func (k msgServer) ApproveRevokeX509RootCert(goCtx context.Context, msg *types.M
 	// get proposed certificate revocation
 	revocation, found := k.GetProposedCertificateRevocation(ctx, msg.Subject, msg.SubjectKeyId, msg.SerialNumber)
 	if !found {
-		return nil, pkitypes.NewErrProposedCertificateRevocationDoesNotExist(msg.Subject, msg.SubjectKeyId)
+		return nil, pkitypes.NewErrProposedCertificateRevocationDoesNotExist(msg.Subject, msg.SubjectKeyId, msg.SerialNumber)
 	}
 
 	// check if proposed certificate revocation already has approval form signer

x/pki/keeper/msg_server_remove_noc_x_509_ica_cert.go

@@ -26,19 +26,30 @@ func (k msgServer) RemoveNocX509IcaCert(goCtx context.Context, msg *types.MsgRem
 	signerAccount, _ := k.dclauthKeeper.GetAccountO(ctx, signerAddr)
 	accountVid := signerAccount.VendorID
 
-	icaCerts, foundActive := k.GetNocIcaCertificatesBySubjectAndSKID(ctx, accountVid, msg.Subject, msg.SubjectKeyId)
+	icaCerts, foundActive := k.GetApprovedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
 	revCerts, foundRevoked := k.GetRevokedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
 	certificates := icaCerts.Certs
 	certificates = append(certificates, revCerts.Certs...)
 	if len(certificates) == 0 {
 		return nil, pkitypes.NewErrCertificateDoesNotExist(msg.Subject, msg.SubjectKeyId)
 	}
 
+	cert := certificates[0]
+	// Existing certificate must be Root certificate
+	if cert.IsRoot {
+		return nil, pkitypes.NewErrMessageExpectedNonRoot(cert.Subject, cert.SubjectKeyId)
+	}
+
 	// Existing certificate must be NOC certificate
-	if !certificates[0].IsNoc {
+	if !cert.IsNoc {
 		return nil, pkitypes.NewErrProvidedNocCertButExistingNotNoc(msg.Subject, msg.SubjectKeyId)
 	}
 
+	// account VID must be same as VID of existing certificates
+	if accountVid != cert.Vid {
+		return nil, pkitypes.NewErrRevokeCertVidNotEqualToAccountVid(cert.Vid, accountVid)
+	}
+
 	if err = k.EnsureVidMatches(ctx, certificates[0].Owner, msg.Signer); err != nil {
 		return nil, err
 	}
@@ -59,19 +70,18 @@ func (k msgServer) RemoveNocX509IcaCert(goCtx context.Context, msg *types.MsgRem
 
 		if foundActive {
 			// Remove from Approved lists
-			aprCerts, _ := k.GetApprovedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
-			removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &aprCerts.Certs)
-			k.removeApprovedX509Cert(ctx, certID, &aprCerts, msg.SerialNumber)
+			removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &icaCerts.Certs)
+			k.removeApprovedX509Cert(ctx, certID, &icaCerts, msg.SerialNumber)
 
 			// Remove from ICA lists
-			k.RemoveNocIcaCertificateBySerialNumber(ctx, icaCerts.Vid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
+			k.RemoveNocIcaCertificateBySerialNumber(ctx, accountVid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
 		}
 		if foundRevoked {
 			removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &revCerts.Certs)
 			k.removeOrUpdateRevokedX509Cert(ctx, certID, &revCerts)
 		}
 	} else {
-		k.RemoveNocIcaCertificate(ctx, certID.Subject, certID.SubjectKeyId, icaCerts.Vid)
+		k.RemoveNocIcaCertificate(ctx, certID.Subject, certID.SubjectKeyId, accountVid)
 		// remove from approved list
 		k.RemoveApprovedCertificates(ctx, certID.Subject, certID.SubjectKeyId)
 		// remove from subject -> subject key ID map

x/pki/keeper/msg_server_remove_noc_x_509_root_cert.go

@@ -26,14 +26,30 @@ func (k msgServer) RemoveNocX509RootCert(goCtx context.Context, msg *types.MsgRe
 	signerAccount, _ := k.dclauthKeeper.GetAccountO(ctx, signerAddr)
 	accountVid := signerAccount.VendorID
 
-	nocCerts, foundActive := k.GetNocRootCertificatesByVidAndSkid(ctx, accountVid, msg.SubjectKeyId)
+	nocCerts, foundActive := k.GetApprovedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
 	revCerts, foundRevoked := k.GetRevokedNocRootCertificates(ctx, msg.Subject, msg.SubjectKeyId)
 	certificates := nocCerts.Certs
 	certificates = append(certificates, revCerts.Certs...)
 	if len(certificates) == 0 {
 		return nil, pkitypes.NewErrCertificateDoesNotExist(msg.Subject, msg.SubjectKeyId)
 	}
 
+	cert := certificates[0]
+	// Existing certificate must be Root certificate
+	if !cert.IsRoot {
+		return nil, pkitypes.NewErrMessageExistingCertIsNotRoot(cert.Subject, cert.SubjectKeyId)
+	}
+
+	// Existing certificate must be NOC certificate
+	if !cert.IsNoc {
+		return nil, pkitypes.NewErrProvidedNocCertButExistingNotNoc(msg.Subject, msg.SubjectKeyId)
+	}
+
+	// account VID must be same as VID of existing certificates
+	if accountVid != cert.Vid {
+		return nil, pkitypes.NewErrRevokeCertVidNotEqualToAccountVid(cert.Vid, accountVid)
+	}
+
 	certID := types.CertificateIdentifier{
 		Subject:      msg.Subject,
 		SubjectKeyId: msg.SubjectKeyId,
@@ -50,23 +66,22 @@ func (k msgServer) RemoveNocX509RootCert(goCtx context.Context, msg *types.MsgRe
 
 		if foundActive {
 			// Remove from Approved lists
-			aprCerts, _ := k.GetApprovedCertificates(ctx, msg.Subject, msg.SubjectKeyId)
-			removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &aprCerts.Certs)
-			k.removeApprovedX509Cert(ctx, certID, &aprCerts, msg.SerialNumber)
+			removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &nocCerts.Certs)
+			k.removeApprovedX509Cert(ctx, certID, &nocCerts, msg.SerialNumber)
 
 			// Remove from NOC lists
-			k.RemoveNocRootCertificateBySerialNumber(ctx, nocCerts.Vid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
-			k.RemoveNocRootCertificateByVidSubjectSkidAndSerialNumber(ctx, nocCerts.Vid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
+			k.RemoveNocRootCertificateBySerialNumber(ctx, accountVid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
+			k.RemoveNocRootCertificateByVidSubjectSkidAndSerialNumber(ctx, accountVid, certID.Subject, certID.SubjectKeyId, msg.SerialNumber)
 		}
 
 		if foundRevoked {
 			removeCertFromList(certBySerialNumber.Issuer, certBySerialNumber.SerialNumber, &revCerts.Certs)
 			k._removeRevokedNocX509RootCert(ctx, certID, &revCerts)
 		}
 	} else {
-		k.RemoveNocRootCertificate(ctx, nocCerts.Vid, certID.Subject, certID.SubjectKeyId)
+		k.RemoveNocRootCertificate(ctx, accountVid, certID.Subject, certID.SubjectKeyId)
 		// remove from vid, subject key id map
-		k.RemoveNocRootCertificatesByVidAndSkid(ctx, nocCerts.Vid, certID.SubjectKeyId)
+		k.RemoveNocRootCertificatesByVidAndSkid(ctx, accountVid, certID.SubjectKeyId)
 		// remove from revoked noc root certs
 		k.RemoveRevokedNocRootCertificates(ctx, certID.Subject, certID.SubjectKeyId)
 		// remove from revoked list

x/pki/keeper/msg_server_revoke_noc_x_509_ica_cert.go

@@ -40,7 +40,7 @@ func (k msgServer) RevokeNocX509IcaCert(goCtx context.Context, msg *types.MsgRev
 	signerVid := signerAccount.VendorID
 	// signer VID must be same as VID of existing certificates
 	if signerVid != cert.Vid {
-		return nil, pkitypes.NewErrRootCertVidNotEqualToAccountVid(cert.Vid, signerVid)
+		return nil, pkitypes.NewErrRevokeCertVidNotEqualToAccountVid(cert.Vid, signerVid)
 	}
 
 	if msg.SerialNumber != "" {

x/pki/keeper/msg_server_revoke_noc_x_509_root_cert.go

@@ -41,7 +41,7 @@ func (k msgServer) RevokeNocX509RootCert(goCtx context.Context, msg *types.MsgRe
 	signerVid := signerAccount.VendorID
 	// signer VID must be same as VID of existing certificates
 	if signerVid != cert.Vid {
-		return nil, pkitypes.NewErrRootCertVidNotEqualToAccountVid(cert.Vid, signerVid)
+		return nil, pkitypes.NewErrRevokeRootCertVidNotEqualToAccountVid(cert.Vid, signerVid)
 	}
 
 	if msg.SerialNumber != "" {