Fix check for admin not belonging to policy

authority/policy.go

@@ -137,7 +137,7 @@ func (a *Authority) checkAuthorityPolicy(ctx context.Context, currentAdmin *link
 	return a.checkPolicy(ctx, currentAdmin, allAdmins, p)
 }
 
-func (a *Authority) checkProvisionerPolicy(ctx context.Context, currentAdmin *linkedca.Admin, provName string, p *linkedca.Policy) error {
+func (a *Authority) checkProvisionerPolicy(ctx context.Context, provName string, p *linkedca.Policy) error {
 
 	// no policy and thus nothing to evaluate; return early
 	if p == nil {
@@ -147,7 +147,11 @@ func (a *Authority) checkProvisionerPolicy(ctx context.Context, currentAdmin *li
 	// get all admins for the provisioner; ignoring case in which they're not found
 	allProvisionerAdmins, _ := a.admins.LoadByProvisioner(provName)
 
-	return a.checkPolicy(ctx, currentAdmin, allProvisionerAdmins, p)
+	// check the policy; pass in nil as the current admin, as all admins for the
+	// provisioner will be checked by looping through allProvisionerAdmins. Also,
+	// the current admin may be a super admin not belonging to the provisioner, so
+	// can't be blocked, but is not required to be in the policy, either.
+	return a.checkPolicy(ctx, nil, allProvisionerAdmins, p)
 }
 
 // checkPolicy checks if a new or updated policy configuration results in the user
@@ -178,10 +182,13 @@ func (a *Authority) checkPolicy(ctx context.Context, currentAdmin *linkedca.Admi
 
 	// check if the admin user that instructed the authority policy to be
 	// created or updated, would still be allowed when the provided policy
-	// would be applied.
+	// would be applied. This case is skipped when current admin is nil, which
-	sans := []string{currentAdmin.GetSubject()}
+	// is the case when a provisioner policy is checked.
-	if err := isAllowed(engine, sans); err != nil {
+	if currentAdmin != nil {
-		return err
+		sans := []string{currentAdmin.GetSubject()}
+		if err := isAllowed(engine, sans); err != nil {
+			return err
+		}
 	}
 
 	// loop through admins to verify that none of them would be
@@ -189,7 +196,7 @@ func (a *Authority) checkPolicy(ctx context.Context, currentAdmin *linkedca.Admi
 	// an error with a message that includes the admin subject that
 	// would be locked out.
 	for _, adm := range otherAdmins {
-		sans = []string{adm.GetSubject()}
+		sans := []string{adm.GetSubject()}
 		if err := isAllowed(engine, sans); err != nil {
 			return err
 		}

authority/policy_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"gopkg.in/square/go-jose.v2"
 
 	"go.step.sm/linkedca"
 
@@ -917,13 +918,58 @@ func TestAuthority_checkAuthorityPolicy(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "fail/policy",
+			fields: fields{
+				admins: administrator.NewCollection(nil),
+				adminDB: &admin.MockDB{
+					MockGetAdmins: func(ctx context.Context) ([]*linkedca.Admin, error) {
+						return []*linkedca.Admin{
+							{
+								Id:      "adminID1",
+								Subject: "anotherAdmin",
+							},
+							{
+								Id:      "adminID2",
+								Subject: "step",
+							},
+							{
+								Id:      "adminID3",
+								Subject: "otherAdmin",
+							},
+						}, nil
+					},
+				},
+			},
+			args: args{
+				currentAdmin: &linkedca.Admin{Subject: "step"},
+				provName:     "prov",
+				p: &linkedca.Policy{
+					X509: &linkedca.X509Policy{
+						Allow: &linkedca.X509Names{
+							Dns: []string{"step", "otherAdmin"},
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
 		{
 			name: "ok",
 			fields: fields{
 				admins: administrator.NewCollection(nil),
 				adminDB: &admin.MockDB{
 					MockGetAdmins: func(ctx context.Context) ([]*linkedca.Admin, error) {
-						return []*linkedca.Admin{}, nil
+						return []*linkedca.Admin{
+							{
+								Id:      "adminID2",
+								Subject: "step",
+							},
+							{
+								Id:      "adminID3",
+								Subject: "otherAdmin",
+							},
+						}, nil
 					},
 				},
 			},
@@ -957,6 +1003,20 @@ func TestAuthority_checkAuthorityPolicy(t *testing.T) {
 }
 
 func TestAuthority_checkProvisionerPolicy(t *testing.T) {
+	jwkProvisioner := &provisioner.JWK{
+		ID:   "jwkID",
+		Type: "JWK",
+		Name: "jwkProv",
+		Key:  &jose.JSONWebKey{KeyID: "jwkKeyID"},
+	}
+	provisioners := provisioner.NewCollection(testAudiences)
+	provisioners.Store(jwkProvisioner)
+	admins := administrator.NewCollection(provisioners)
+	admins.Store(&linkedca.Admin{
+		Id:            "adminID",
+		Subject:       "step",
+		ProvisionerId: "jwkID",
+	}, jwkProvisioner)
 	type fields struct {
 		provisioners *provisioner.Collection
 		admins       *administrator.Collection
@@ -964,10 +1024,9 @@ func TestAuthority_checkProvisionerPolicy(t *testing.T) {
 		adminDB      admin.DB
 	}
 	type args struct {
-		ctx          context.Context
+		ctx      context.Context
-		currentAdmin *linkedca.Admin
+		provName string
-		provName     string
+		p        *linkedca.Policy
-		p            *linkedca.Policy
 	}
 	tests := []struct {
 		name    string
@@ -979,20 +1038,37 @@ func TestAuthority_checkProvisionerPolicy(t *testing.T) {
 			name:   "no policy",
 			fields: fields{},
 			args: args{
-				currentAdmin: nil,
+				provName: "prov",
-				provName:     "prov",
+				p:        nil,
-				p:            nil,
 			},
 			wantErr: false,
 		},
 		{
-			name: "ok",
+			name: "fail/policy",
 			fields: fields{
-				admins: administrator.NewCollection(nil),
+				provisioners: provisioners,
+				admins:       admins,
 			},
 			args: args{
-				currentAdmin: &linkedca.Admin{Subject: "step"},
+				provName: "jwkProv",
-				provName:     "prov",
+				p: &linkedca.Policy{
+					X509: &linkedca.X509Policy{
+						Allow: &linkedca.X509Names{
+							Dns: []string{"otherAdmin"}, // step not in policy
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "ok",
+			fields: fields{
+				provisioners: provisioners,
+				admins:       admins,
+			},
+			args: args{
+				provName: "jwkProv",
 				p: &linkedca.Policy{
 					X509: &linkedca.X509Policy{
 						Allow: &linkedca.X509Names{
@@ -1012,7 +1088,7 @@ func TestAuthority_checkProvisionerPolicy(t *testing.T) {
 				db:           tt.fields.db,
 				adminDB:      tt.fields.adminDB,
 			}
-			if err := a.checkProvisionerPolicy(tt.args.ctx, tt.args.currentAdmin, tt.args.provName, tt.args.p); (err != nil) != tt.wantErr {
+			if err := a.checkProvisionerPolicy(tt.args.ctx, tt.args.provName, tt.args.p); (err != nil) != tt.wantErr {
 				t.Errorf("Authority.checkProvisionerPolicy() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

authority/provisioners.go

@@ -173,9 +173,7 @@ func (a *Authority) StoreProvisioner(ctx context.Context, prov *linkedca.Provisi
 		return admin.WrapErrorISE(err, "error generating provisioner config")
 	}
 
-	adm := linkedca.MustAdminFromContext(ctx)
+	if err := a.checkProvisionerPolicy(ctx, prov.Name, prov.Policy); err != nil {
-
-	if err := a.checkProvisionerPolicy(ctx, adm, prov.Name, prov.Policy); err != nil {
 		return err
 	}
 
@@ -224,9 +222,7 @@ func (a *Authority) UpdateProvisioner(ctx context.Context, nu *linkedca.Provisio
 		return admin.WrapErrorISE(err, "error generating provisioner config")
 	}
 
-	adm := linkedca.MustAdminFromContext(ctx)
+	if err := a.checkProvisionerPolicy(ctx, nu.Name, nu.Policy); err != nil {
-
-	if err := a.checkProvisionerPolicy(ctx, adm, nu.Name, nu.Policy); err != nil {
 		return err
 	}