Merge "[FAB-2408] Fix policies absolute paths"

common/configtx/update.go

@@ -18,7 +18,6 @@ package configtx
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/hyperledger/fabric/common/policies"
 	cb "github.com/hyperledger/fabric/protos/common"
@@ -123,10 +122,6 @@ func (cm *configManager) authorizeUpdate(configUpdateEnv *cb.ConfigUpdateEnvelop
 }
 
 func (cm *configManager) policyForItem(item comparable) (policies.Policy, bool) {
-	if strings.HasPrefix(item.modPolicy(), PathSeparator) {
-		return cm.PolicyManager().GetPolicy(item.modPolicy()[1:])
-	}
-
 	// path is always at least of length 1
 	manager, ok := cm.PolicyManager().Manager(item.path[1:])
 	if !ok {

common/configtx/update_test.go

@@ -72,13 +72,4 @@ func TestPolicyForItem(t *testing.T) {
 	})
 	assert.True(t, ok)
 	assert.Equal(t, policy, fooPolicy, "Should not have found relative foo policy the foo manager")
-
-	policy, ok = cm.policyForItem(comparable{
-		path: []string{"root", "foo"},
-		ConfigValue: &cb.ConfigValue{
-			ModPolicy: "/rootPolicy",
-		},
-	})
-	assert.True(t, ok)
-	assert.Equal(t, policy, rootPolicy, "Should not have found absolute root policy from the foo path position")
 }

common/policies/policy.go

@@ -18,15 +18,25 @@ package policies
 
 import (
 	"fmt"
+	"strings"
 
 	cb "github.com/hyperledger/fabric/protos/common"
 
 	logging "github.com/op/go-logging"
 )
 
 const (
+	// Path separator is used to separate policy names in paths
+	PathSeparator = "/"
+
+	// ChannelPrefix is used in the path of standard channel policy managers
+	ChannelPrefix = "Channel"
+
+	// ApplicationPrefix is used in the path of standard application policy paths
+	ApplicationPrefix = "Application"
+
 	// ChannelApplicationReaders is the label for the channel's application readers policy
-	ChannelApplicationReaders = "/channel/Application/Readers"
+	ChannelApplicationReaders = "/" + ChannelPrefix + "/" + ApplicationPrefix + "/Readers"
 )
 
 var logger = logging.MustGetLogger("common/policies")
@@ -45,7 +55,7 @@ type Manager interface {
 	// Manager returns the sub-policy manager for a given path and whether it exists
 	Manager(path []string) (Manager, bool)
 
-	// Basepath returns the basePath the manager was instnatiated with
+	// Basepath returns the basePath the manager was instantiated with
 	BasePath() string
 
 	// Policies returns all policy names defined in the manager
@@ -85,7 +95,9 @@ type policyConfig struct {
 // ManagerImpl is an implementation of Manager and configtx.ConfigHandler
 // In general, it should only be referenced as an Impl for the configtx.ConfigManager
 type ManagerImpl struct {
+	parent        *ManagerImpl
 	basePath      string
+	fqPrefix      string
 	providers     map[int32]Provider
 	config        *policyConfig
 	pendingConfig *policyConfig
@@ -100,6 +112,7 @@ func NewManagerImpl(basePath string, providers map[int32]Provider) *ManagerImpl
 
 	return &ManagerImpl{
 		basePath:  basePath,
+		fqPrefix:  PathSeparator + basePath + PathSeparator,
 		providers: providers,
 		config: &policyConfig{
 			policies: make(map[string]Policy),
@@ -145,15 +158,36 @@ func (pm *ManagerImpl) Manager(path []string) (Manager, bool) {
 
 // GetPolicy returns a policy and true if it was the policy requested, or false if it is the default reject policy
 func (pm *ManagerImpl) GetPolicy(id string) (Policy, bool) {
-	policy, ok := pm.config.policies[id]
+	if id == "" {
+		logger.Errorf("Returning dummy reject all policy because no policy ID supplied")
+		return rejectPolicy(id), false
+	}
+	var relpath string
+
+	if strings.HasPrefix(id, PathSeparator) {
+		if pm.parent != nil {
+			return pm.parent.GetPolicy(id)
+		}
+		if !strings.HasPrefix(id, pm.fqPrefix) {
+			if logger.IsEnabledFor(logging.DEBUG) {
+				logger.Debugf("Requested policy from root manager with wrong basePath: %s, returning rejectAll", id)
+			}
+			return rejectPolicy(id), false
+		}
+		relpath = id[len(pm.fqPrefix):]
+	} else {
+		relpath = id
+	}
+
+	policy, ok := pm.config.policies[relpath]
 	if !ok {
 		if logger.IsEnabledFor(logging.DEBUG) {
-			logger.Debugf("Returning dummy reject all policy because %s could not be found in %s", id, pm.basePath)
+			logger.Debugf("Returning dummy reject all policy because %s could not be found in /%s/%s", id, pm.basePath, relpath)
 		}
-		return rejectPolicy(id), false
+		return rejectPolicy(relpath), false
 	}
 	if logger.IsEnabledFor(logging.DEBUG) {
-		logger.Debugf("Returning policy %s for evaluation", id)
+		logger.Debugf("Returning policy %s for evaluation", relpath)
 	}
 	return policy, true
 }
@@ -172,6 +206,7 @@ func (pm *ManagerImpl) BeginPolicyProposals(groups []string) ([]Proposer, error)
 	managers := make([]Proposer, len(groups))
 	for i, group := range groups {
 		newManager := NewManagerImpl(group, pm.providers)
+		newManager.parent = pm
 		pm.pendingConfig.managers[group] = newManager
 		managers[i] = newManager
 	}
@@ -196,7 +231,7 @@ func (pm *ManagerImpl) CommitProposals() {
 
 	for managerPath, m := range pm.pendingConfig.managers {
 		for _, policyName := range m.PolicyNames() {
-			fqKey := managerPath + "/" + policyName
+			fqKey := managerPath + PathSeparator + policyName
 			pm.pendingConfig.policies[fqKey], _ = m.GetPolicy(policyName)
 			logger.Debugf("In commit adding relative sub-policy %s to %s", fqKey, pm.basePath)
 		}
@@ -209,6 +244,20 @@ func (pm *ManagerImpl) CommitProposals() {
 
 	pm.config = pm.pendingConfig
 	pm.pendingConfig = nil
+
+	if pm.parent == nil && pm.basePath == ChannelPrefix {
+		if _, ok := pm.config.managers[ApplicationPrefix]; ok {
+			// Check for default application policies if the application component is defined
+			for _, policyName := range []string{ChannelApplicationReaders} {
+				_, ok := pm.GetPolicy(policyName)
+				if !ok {
+					logger.Warningf("Current configuration has no policy '%s', this will likely cause problems in production systems", policyName)
+				} else {
+					logger.Debugf("As expected, current configuration has policy '%s'", policyName)
+				}
+			}
+		}
+	}
 }
 
 // ProposePolicy takes key, path, and ConfigPolicy and registers it in the proposed PolicyManager, or errors

common/policies/policy_test.go

@@ -74,6 +74,7 @@ func TestUnnestedManager(t *testing.T) {
 
 func TestNestedManager(t *testing.T) {
 	m := NewManagerImpl("test", defaultProviders())
+	absPrefix := "/test/"
 	nesting1, err := m.BeginPolicyProposals([]string{"nest1"})
 	assert.NoError(t, err)
 	assert.Len(t, nesting1, 1, "Should not have returned exactly one additional manager")
@@ -137,6 +138,10 @@ func TestNestedManager(t *testing.T) {
 	for _, policyName := range policyNames {
 		_, ok := m.GetPolicy(policyName)
 		assert.True(t, ok, "Should have found policy %s", policyName)
+
+		absName := absPrefix + policyName
+		_, ok = m.GetPolicy(absName)
+		assert.True(t, ok, "Should have found absolute policy %s", absName)
 	}
 
 	for _, policyName := range n1PolicyNames {
@@ -145,6 +150,12 @@ func TestNestedManager(t *testing.T) {
 
 		_, ok = m.GetPolicy(n1.BasePath() + "/" + policyName)
 		assert.True(t, ok, "Should have found policy %s", policyName)
+
+		for i, abs := range []Manager{n1, m} {
+			absName := absPrefix + n1.BasePath() + "/" + policyName
+			_, ok = abs.GetPolicy(absName)
+			assert.True(t, ok, "Should have found absolutely policy for manager %d", i)
+		}
 	}
 
 	for _, policyName := range n2aPolicyNames {
@@ -156,6 +167,12 @@ func TestNestedManager(t *testing.T) {
 
 		_, ok = m.GetPolicy(n1.BasePath() + "/" + n2a.BasePath() + "/" + policyName)
 		assert.True(t, ok, "Should have found policy %s", policyName)
+
+		for i, abs := range []Manager{n2a, n1, m} {
+			absName := absPrefix + n1.BasePath() + "/" + n2a.BasePath() + "/" + policyName
+			_, ok = abs.GetPolicy(absName)
+			assert.True(t, ok, "Should have found absolutely policy for manager %d", i)
+		}
 	}
 
 	for _, policyName := range n2bPolicyNames {
@@ -167,5 +184,11 @@ func TestNestedManager(t *testing.T) {
 
 		_, ok = m.GetPolicy(n1.BasePath() + "/" + n2b.BasePath() + "/" + policyName)
 		assert.True(t, ok, "Should have found policy %s", policyName)
+
+		for i, abs := range []Manager{n2b, n1, m} {
+			absName := absPrefix + n1.BasePath() + "/" + n2b.BasePath() + "/" + policyName
+			_, ok = abs.GetPolicy(absName)
+			assert.True(t, ok, "Should have found absolutely policy for manager %d", i)
+		}
 	}
 }