Skip to content

Commit

Permalink
Apply review suggestions
Browse files Browse the repository at this point in the history 
Signed-off-by: Daniel Weiße <[email protected]>
  • Loading branch information
@daniel-weisse
daniel-weisse committed Jan 13, 2025
1 parent 455ea05 commit 9818283
Show file tree
Hide file tree
Showing 2 changed files with 166 additions and 42 deletions.
7 changes: 4 additions & 3 deletions coordinator/manifest/manifest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -499,16 +499,17 @@ func (m Manifest) Check(zaplogger *zap.Logger) error {
}
}

var manifestUpdaters int
var manifestUpdaters uint
for _, mrUser := range m.Users {
for _, roleName := range mrUser.Roles {
if m.Roles[roleName].ResourceType == "Manifest" && strings.ToLower(m.Roles[roleName].Actions[0]) == user.PermissionUpdateManifest {
if m.Roles[roleName].ResourceType == "Manifest" && len(m.Roles[roleName].Actions) > 0 &&
strings.ToLower(m.Roles[roleName].Actions[0]) == user.PermissionUpdateManifest {
manifestUpdaters++
break // Avoid counting the same user multiple times if they are assigned more than one role with update permission
}
}
}
if manifestUpdaters < int(m.Config.UpdateThreshold) {
if manifestUpdaters < m.Config.UpdateThreshold {
return fmt.Errorf("not enough users with manifest update permissions (%d) to meet the threshold of %d", manifestUpdaters, m.Config.UpdateThreshold)
}

Expand Down
201 changes: 162 additions & 39 deletions coordinator/manifest/manifest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,50 +370,173 @@ func TestTemplateDryRun(t *testing.T) {
}

func TestManifestCheck(t *testing.T) {
assert := assert.New(t)
require := require.New(t)
testCases := map[string]struct {
manifest func(*require.Assertions) Manifest
wantErr bool
}{
"valid default manifest": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.ManifestJSON), &manifest))
return manifest
},
},
"valid manifest with recovery key": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest))
return manifest
},
},
"valid integration test manifest": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.IntegrationManifestJSON), &manifest))
return manifest
},
},
"update threshold 0 with one update-manifest user": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest))

var manifest Manifest
err := json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest)
require.NoError(err)
manifest.Roles["updateManifest"] = Role{
ResourceType: "Manifest",
ResourceNames: []string{},
Actions: []string{user.PermissionUpdateManifest},
}
adminUser := manifest.Users["admin"]
adminUser.Roles = append(adminUser.Roles, "updateManifest")
manifest.Users["admin"] = adminUser

log := zaptest.NewLogger(t)
err = manifest.Check(log)
assert.NoError(err)
manifest.Config.UpdateThreshold = 0
return manifest
},
},
"update threshold equal to allowed users": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest))

manifest.Roles["updateManifest"] = Role{
ResourceType: "Manifest",
ResourceNames: []string{},
Actions: []string{user.PermissionUpdateManifest},
}
adminUser := manifest.Users["admin"]
adminUser.Roles = append(adminUser.Roles, "updateManifest")
manifest.Users["admin"] = adminUser

manifest.Config.UpdateThreshold = 1
return manifest
},
},
"update threshold lower than allowed users": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest))

manifest.Roles["updateManifest"] = Role{
ResourceType: "Manifest",
ResourceNames: []string{},
Actions: []string{user.PermissionUpdateManifest},
}
adminUser := manifest.Users["admin"]
adminUser.Roles = append(adminUser.Roles, "updateManifest")
manifest.Users["admin"] = adminUser

// Set up a second user from a copy of the first
// The certificate is not a valid x509 cert, but that is not checked for in this test
adminUser2 := adminUser
adminUser2.Certificate = strings.ReplaceAll(adminUser.Certificate, "q", "b")
manifest.Users["admin2"] = adminUser2

manifest.Config.UpdateThreshold = 1
return manifest
},
},
"update threshold higher than allowed users": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest))

manifest.Roles["updateManifest"] = Role{
ResourceType: "Manifest",
ResourceNames: []string{},
Actions: []string{user.PermissionUpdateManifest},
}
adminUser := manifest.Users["admin"]
adminUser.Roles = append(adminUser.Roles, "updateManifest")
manifest.Users["admin"] = adminUser

// Set up a second user from a copy of the first
// The certificate is not a valid x509 cert, but that is not checked for in this test
adminUser2 := adminUser
adminUser2.Certificate = strings.ReplaceAll(adminUser.Certificate, "q", "b")
manifest.Users["admin2"] = adminUser2

manifest.Roles["updateManifest"] = Role{
ResourceType: "Manifest",
ResourceNames: []string{},
Actions: []string{user.PermissionUpdateManifest},
manifest.Config.UpdateThreshold = 3
return manifest
},
wantErr: true,
},
"user with multiple update manifest roles is only counted once for the threshold": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest))

manifest.Roles["updateManifest"] = Role{
ResourceType: "Manifest",
ResourceNames: []string{},
Actions: []string{user.PermissionUpdateManifest},
}
manifest.Roles["updateManifest2"] = Role{
ResourceType: "Manifest",
ResourceNames: []string{},
Actions: []string{user.PermissionUpdateManifest},
}
manifest.Roles["updateManifest3"] = Role{
ResourceType: "Manifest",
ResourceNames: []string{},
Actions: []string{user.PermissionUpdateManifest},
}

adminUser := manifest.Users["admin"]
adminUser.Roles = append(adminUser.Roles, "updateManifest", "updateManifest2", "updateManifest3")
manifest.Users["admin"] = adminUser

manifest.Config.UpdateThreshold = 3
return manifest
},
wantErr: true,
},
"users without unique certificates": {
manifest: func(require *require.Assertions) Manifest {
var manifest Manifest
require.NoError(json.Unmarshal([]byte(test.ManifestJSONWithRecoveryKey), &manifest))

manifest.Users["anotherUser"] = User{
Certificate: manifest.Users["admin"].Certificate,
}
return manifest
},
wantErr: true,
},
}
adminUser := manifest.Users["admin"]
adminUser.Roles = append(adminUser.Roles, "updateManifest")
manifest.Users["admin"] = adminUser

manifest.Config.UpdateThreshold = 0
err = manifest.Check(log)
assert.NoError(err, "update threshold 0 should be accepted")

manifest.Config.UpdateThreshold = 1
err = manifest.Check(log)
assert.NoError(err, "update threshold equal to allowed users should be accepted")

adminUser2 := adminUser
adminUser2.Certificate = strings.ReplaceAll(adminUser.Certificate, "q", "b")
manifest.Users["admin2"] = adminUser2
err = manifest.Check(log)
assert.NoError(err, "update threshold lower than allowed users should be accepted")

manifest.Config.UpdateThreshold = 3
err = manifest.Check(log)
assert.Error(err, "update threshold higher than allowed users should be rejected")
manifest.Config.UpdateThreshold = 0

manifest.Users["anotherUser"] = User{
Certificate: manifest.Users["admin"].Certificate,

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
assert := assert.New(t)
require := require.New(t)

err := tc.manifest(require).Check(zaptest.NewLogger(t))
if tc.wantErr {
assert.Error(err)
} else {
assert.NoError(err)
}
})
}
err = manifest.Check(log)
assert.Error(err, "users without unique certificates should be rejected")
}

func TestCertificate(t *testing.T) {
Expand Down

0 comments on commit 9818283

Please sign in to comment.