From f777ba7005a4ea6d153798d8d8624f7e3dfea8c5 Mon Sep 17 00:00:00 2001 From: Jason Yellick Date: Tue, 21 Feb 2017 11:40:20 -0500 Subject: [PATCH] [FAB-2408] Fix policies absolute paths https://jira.hyperledger.org/browse/FAB-2408 While the policy manager was still in development, the absolute path resolution for policies was handled in the configuration manager. This worked fine for config updates, but for other components of the system such as those depending on the 'ChannelApplicationReaders', the absolute paths did not work. This CR pushes the absolute policy path resolution back into the policy manager where it belongs. Change-Id: Idcfa2ca030115b3ff9e3034f8c54767af1c2ea49 Signed-off-by: Jason Yellick --- common/configtx/update.go | 5 --- common/configtx/update_test.go | 9 ----- common/policies/policy.go | 63 ++++++++++++++++++++++++++++++---- common/policies/policy_test.go | 23 +++++++++++++ 4 files changed, 79 insertions(+), 21 deletions(-) diff --git a/common/configtx/update.go b/common/configtx/update.go index 9a840b70ca5..a7ca1cae730 100644 --- a/common/configtx/update.go +++ b/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 { diff --git a/common/configtx/update_test.go b/common/configtx/update_test.go index dc7f0ac5997..0031baad2b8 100644 --- a/common/configtx/update_test.go +++ b/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") } diff --git a/common/policies/policy.go b/common/policies/policy.go index d20dad36521..c1ca05a691b 100644 --- a/common/policies/policy.go +++ b/common/policies/policy.go @@ -18,6 +18,7 @@ package policies import ( "fmt" + "strings" cb "github.com/hyperledger/fabric/protos/common" @@ -25,8 +26,17 @@ import ( ) 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 @@ -78,7 +88,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 @@ -93,6 +105,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), @@ -138,15 +151,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 } @@ -165,6 +199,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 } @@ -184,7 +219,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) } @@ -197,6 +232,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 diff --git a/common/policies/policy_test.go b/common/policies/policy_test.go index e43f09940e4..7de47971f7c 100644 --- a/common/policies/policy_test.go +++ b/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) + } } }