From 911d901d3f007c3b5f1e049378b2251ec4f8a403 Mon Sep 17 00:00:00 2001 From: Arnaud J Le Hors Date: Tue, 30 Jan 2018 11:40:09 +0100 Subject: [PATCH] [FAB-7893] Prevent unforced delete of sub-affiliations This patch adds a couple of unit tests and change code on removal of an affiliation with sub-affiliations to check that this only happens when the force flag is specified. Before the bug fix the new tests fail: $ go test -v -run=TestAffiliationCmd github.com/hyperledger/fabric-ca/cmd/ fabric-ca-client ... Successfully added affiliation: org4.dept1.team Successfully removed affiliation: &{AffiliationInfo:{Name:org3 Affiliation s:[] Identities:[]} CAName:} Successfully modified affiliation: &{AffiliationInfo:{Name:org3 Affiliatio ns:[] Identities:[]} CAName:} Successfully removed affiliation: &{AffiliationInfo:{Name:org4 Affiliation s:[{Name:org4.dept1 Affiliations:[{Name:org4.dept1.team Affiliations:[] Id entities:[]}] Identities:[]}] Identities:[]} CAName:} 2018/01/30 11:31:21 [ERROR] Server has stopped serving: accept tcp [::]:70 90: use of closed network connection --- FAIL: TestAffiliationCmd (4.13s) Error Trace: main_test.go:858 Error: An error is expected but got nil. Messages: Should have failed, no force argument provided and affiliation being deleted had sub-affiliations FAIL exit status 1 FAIL github.com/hyperledger/fabric-ca/cmd/fabric-ca-client 4.147s After the bug fix the new tests pass: $ go test -v -run=TestAffiliationCmd github.com/hyperledger/fabric-ca/cmd/ fabric-ca-client ... Successfully added affiliation: org4.dept1.team Successfully removed affiliation: &{AffiliationInfo:{Name:org3 Affiliation s:[] Identities:[]} CAName:} Successfully modified affiliation: &{AffiliationInfo:{Name:org3 Affiliatio ns:[] Identities:[]} CAName:} Error: Response from server: Error Code: 20 - Authorization failure Successfully removed affiliation: &{AffiliationInfo:{Name:org4 Affiliation s:[{Name:org4.dept1 Affiliations:[{Name:org4.dept1.team Affiliations:[] Id entities:[]}] Identities:[]}] Identities:[]} CAName:} 2018/01/30 11:33:06 [ERROR] Server has stopped serving: accept tcp [::]:70 90: use of closed network connection --- PASS: TestAffiliationCmd (4.12s) PASS ok github.com/hyperledger/fabric-ca/cmd/fabric-ca-client 4.129s Patch-set #2: Improve error messages. Change-Id: Ib5a7acb68a42f251970cce6735173ea296ef5d54 Signed-off-by: Arnaud J Le Hors --- cmd/fabric-ca-client/main_test.go | 10 ++++ lib/dbaccessor.go | 87 ++++++++++++++++++------------- 2 files changed, 60 insertions(+), 37 deletions(-) diff --git a/cmd/fabric-ca-client/main_test.go b/cmd/fabric-ca-client/main_test.go index 2bed18a92..4480984af 100644 --- a/cmd/fabric-ca-client/main_test.go +++ b/cmd/fabric-ca-client/main_test.go @@ -853,6 +853,16 @@ func TestAffiliationCmd(t *testing.T) { _, err = registry.GetAffiliation("org3") assert.NoError(t, err, "Failed to rename 'org1' to 'org3' successfully") + err = RunMain([]string{ + cmdName, "affiliation", "remove", "org4"}) + assert.Error(t, err, "Should have failed, no force argument provided and affiliation being deleted had sub-affiliations") + + // if previous test failed, don't bother with the next one + if err != nil { + err = RunMain([]string{ + cmdName, "affiliation", "remove", "org4", "--force"}) + assert.NoError(t, err, "Failed to remove affiliation with force argument") + } } // Verify the certificate has attribute 'name' with a value of 'val' diff --git a/lib/dbaccessor.go b/lib/dbaccessor.go index ebc468932..289db6463 100644 --- a/lib/dbaccessor.go +++ b/lib/dbaccessor.go @@ -378,43 +378,17 @@ func (d *Accessor) deleteAffiliationTx(tx *sqlx.Tx, args ...interface{}) (interf } idNamesStr := strings.Join(idNames, ",") + // First check that all settings are correct if len(ids) > 0 { if !isRegistar { return nil, newAuthErr(ErrUpdateConfigRemoveAff, "Removing affiliation affects identities, but caller is not a registrar") } - - // Force enabled, delete any associated identities and certificates - if force { - log.Debugf("IDs '%s' to removed based on affiliation '%s' removal", idNamesStr, name) - - if !identityRemoval { - return nil, newAuthErr(ErrUpdateConfigRemoveAff, "Identity removal is not allowed on server") - } - - // Delete all the identities in one database request - query := "DELETE FROM users WHERE (id IN (?))" - inQuery, args, err := sqlx.In(query, idNames) - if err != nil { - return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to construct query '%s': %s", query, err) - } - _, err = tx.Exec(tx.Rebind(inQuery), args...) - if err != nil { - return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to execute query '%s' for multiple identity removal: %s", query, err) - } - - // Revoke all the certificates associated with the removed identities above with reason of "affiliationchange" (3) - query = "UPDATE certificates SET status='revoked', revoked_at=CURRENT_TIMESTAMP, reason = ? WHERE (id IN (?) AND status != 'revoked')" - inQuery, args, err = sqlx.In(query, ocsp.AffiliationChanged, idNames) - if err != nil { - return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to construct query '%s': %s", query, err) - } - _, err = tx.Exec(tx.Rebind(inQuery), args...) - if err != nil { - return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to execute query '%s' for multiple certificate removal: %s", query, err) - } - } else { + if !identityRemoval { + return nil, newAuthErr(ErrUpdateConfigRemoveAff, "Identity removal is not allowed on server") + } + if !force { // If force option is not specified, only delete affiliation if there are no identities that have that affiliation - return nil, newAuthErr(ErrUpdateConfigRemoveAff, "The request to remove affiliation '%s' has the following identities associated: %s. Need to use 'force' to remove identities and affiliation", name, idNamesStr) + return nil, newAuthErr(ErrUpdateConfigRemoveAff, "Cannot delete affiliation '%s'. The affiliation has the following identities associated: %s. Need to use 'force' to remove identities and affiliation", name, idNamesStr) } } @@ -429,7 +403,44 @@ func (d *Accessor) deleteAffiliationTx(tx *sqlx.Tx, args ...interface{}) (interf if err != nil { return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to select sub-affiliations of '%s': %s", allAffs, err) } + + if len(allAffs) > 0 { + if !force { + // If force option is not specified, only delete affiliation if there are no sub-affiliations + return nil, newAuthErr(ErrUpdateConfigRemoveAff, "Cannot delete affiliation '%s'. The affiliation has the following sub-affiliations: %s. Need to use 'force' to remove affiliation and sub-affiliations", name, allAffs) + } + } allAffs = append(allAffs, aff) + + // Now proceed with deletion + + // delete any associated identities and certificates + if len(ids) > 0 { + log.Debugf("IDs '%s' to be removed based on affiliation '%s' removal", idNamesStr, name) + + // Delete all the identities in one database request + query := "DELETE FROM users WHERE (id IN (?))" + inQuery, args, err := sqlx.In(query, idNames) + if err != nil { + return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to construct query '%s': %s", query, err) + } + _, err = tx.Exec(tx.Rebind(inQuery), args...) + if err != nil { + return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to execute query '%s' for multiple identity removal: %s", query, err) + } + + // Revoke all the certificates associated with the removed identities above with reason of "affiliationchange" (3) + query = "UPDATE certificates SET status='revoked', revoked_at=CURRENT_TIMESTAMP, reason = ? WHERE (id IN (?) AND status != 'revoked')" + inQuery, args, err = sqlx.In(query, ocsp.AffiliationChanged, idNames) + if err != nil { + return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to construct query '%s': %s", query, err) + } + _, err = tx.Exec(tx.Rebind(inQuery), args...) + if err != nil { + return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to execute query '%s' for multiple certificate removal: %s", query, err) + } + } + log.Debug("All affiliations to be removed: ", allAffs) // Delete the requested affiliation @@ -437,12 +448,14 @@ func (d *Accessor) deleteAffiliationTx(tx *sqlx.Tx, args ...interface{}) (interf if err != nil { return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to delete affiliation '%s': %s", name, err) } - // Delete all the sub-affiliations - _, err = tx.Exec(tx.Rebind("DELETE FROM affiliations where (name LIKE ?)"), subAffName) - if err != nil { - return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to delete affiliations: %s", err) - } + if len(allAffs) > 1 { + // Delete all the sub-affiliations + _, err = tx.Exec(tx.Rebind("DELETE FROM affiliations where (name LIKE ?)"), subAffName) + if err != nil { + return nil, newHTTPErr(500, ErrRemoveAffDB, "Failed to delete affiliations: %s", err) + } + } // Return the identities and affiliations that were removed result := d.getResult(ids, allAffs)