Merge pull request #6711 from TheThingsNetwork/fix/membership-chains-…

…batch-rights Fix universal rights effect on batch gateway rights assertions
TheThingsNetwork · Nov 16, 2023 · fc765de · fc765de
2 parents 0c91d4b + 3001ded
commit fc765de
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 13 deletions.
diff --git a/pkg/identityserver/gateway_access_test.go b/pkg/identityserver/gateway_access_test.go
@@ -590,15 +590,14 @@ func TestGatewayBatchAccess(t *testing.T) {
 	locUserCreds := rpcCreds(locUserKey)
 
 	testWithIdentityServer(t, func(is *IdentityServer, cc *grpc.ClientConn) {
-		is.config.AdminRights.All = true
-
 		reg := ttnpb.NewGatewayBatchAccessClient(cc)
 
 		for _, tc := range []struct {
-			Name           string
-			Credentials    grpc.CallOption
-			Request        *ttnpb.AssertGatewayRightsRequest
-			ErrorAssertion func(error) bool
+			Name               string
+			Credentials        grpc.CallOption
+			Request            *ttnpb.AssertGatewayRightsRequest
+			ErrorAssertion     func(error) bool
+			LimitedAdminRights bool
 		}{
 			{
 				Name:           "Empty request",
@@ -771,6 +770,20 @@ func TestGatewayBatchAccess(t *testing.T) {
 				Credentials:    collaboratorCreds,
 				ErrorAssertion: errors.IsPermissionDenied,
 			},
+			{
+				Name: "Universal rights for admin",
+				Request: &ttnpb.AssertGatewayRightsRequest{
+					GatewayIds: []*ttnpb.GatewayIdentifiers{
+						gtw3.GetIds(),
+					},
+					Required: &ttnpb.Rights{
+						Rights: []ttnpb.Right{
+							ttnpb.Right_RIGHT_GATEWAY_SETTINGS_API_KEYS,
+						},
+					},
+				},
+				Credentials: adminCreds,
+			},
 			{
 				Name: "Limited rights for admin",
 				Request: &ttnpb.AssertGatewayRightsRequest{
@@ -783,8 +796,9 @@ func TestGatewayBatchAccess(t *testing.T) {
 						},
 					},
 				},
-				Credentials:    adminCreds,
-				ErrorAssertion: errors.IsNotFound,
+				Credentials:        adminCreds,
+				ErrorAssertion:     errors.IsPermissionDenied,
+				LimitedAdminRights: true,
 			},
 			{
 				Name: "Read Stats Failure",
@@ -945,6 +959,8 @@ func TestGatewayBatchAccess(t *testing.T) {
 		} {
 			tc := tc
 			t.Run(tc.Name, func(t *testing.T) {
+				is.config.AdminRights.All = !tc.LimitedAdminRights
+
 				_, err := reg.AssertRights(ctx, tc.Request, tc.Credentials)
 				if err != nil {
 					if tc.ErrorAssertion == nil || !a.So(tc.ErrorAssertion(err), should.BeTrue) {

diff --git a/pkg/identityserver/rights.go b/pkg/identityserver/rights.go
@@ -261,7 +261,7 @@ func (is *IdentityServer) assertGatewayRights( // nolint:gocyclo
 			return err
 		}
 		if len(gtws) != len(gtwIDs) {
-			if is.IsAdmin(ctx) {
+			if authInfo.IsAdmin {
 				// Return the cause only to the admin.
 				// This follows the same logic as in ListRights.
 				return errSomeGatewaysNotFound.New()
@@ -322,6 +322,11 @@ func (is *IdentityServer) assertGatewayRights( // nolint:gocyclo
 		if len(entityIDs) == 0 {
 			return nil
 		}
+		if authInfo.IsAdmin {
+			if authInfo.GetUniversalRights().IncludesAll(requiredGatewayRights.GetRights()...) {
+				return nil
+			}
+		}
 		membershipChains, err := st.FindAccountMembershipChains(
 			ctx,
 			ouID,
@@ -332,10 +337,6 @@ func (is *IdentityServer) assertGatewayRights( // nolint:gocyclo
 			return err
 		}
 		if len(membershipChains) != len(entityIDs) {
-			// Some memberships were not found.
-			if is.IsAdmin(ctx) {
-				return errSomeGatewaysNotFound.New()
-			}
 			return errInsufficientRights.New()
 		}
 		for _, chain := range membershipChains {