From fbaf7c55821070944bb0ce342ba3c54cc521c6fe Mon Sep 17 00:00:00 2001
From: Ashitha Santhosh <55257063+ashithasantosh@users.noreply.github.com>
Date: Tue, 28 Dec 2021 15:07:12 -0800
Subject: [PATCH] authz: update representation of allow authenticated in SDK
 (#5052)

* remove empty principals logic

* Update test

* minor formatting

* resolving comments
---
 authz/rbac_translator.go      |  8 +-------
 authz/rbac_translator_test.go | 34 ++++++++++++++++++++++++----------
 authz/sdk_end2end_test.go     | 34 +++++-----------------------------
 3 files changed, 30 insertions(+), 46 deletions(-)

diff --git a/authz/rbac_translator.go b/authz/rbac_translator.go
index 010fc89a6e22..75bbdb44d497 100644
--- a/authz/rbac_translator.go
+++ b/authz/rbac_translator.go
@@ -157,19 +157,13 @@ func parsePrincipalNames(principalNames []string) []*v3rbacpb.Principal {
 }
 
 func parsePeer(source peer) *v3rbacpb.Principal {
-	if source.Principals == nil {
+	if len(source.Principals) == 0 {
 		return &v3rbacpb.Principal{
 			Identifier: &v3rbacpb.Principal_Any{
 				Any: true,
 			},
 		}
 	}
-	if len(source.Principals) == 0 {
-		return &v3rbacpb.Principal{
-			Identifier: &v3rbacpb.Principal_Authenticated_{
-				Authenticated: &v3rbacpb.Principal_Authenticated{},
-			}}
-	}
 	return principalOr(parsePrincipalNames(source.Principals))
 }
 
diff --git a/authz/rbac_translator_test.go b/authz/rbac_translator_test.go
index e22ab62ce26b..e8e2f76b5ed8 100644
--- a/authz/rbac_translator_test.go
+++ b/authz/rbac_translator_test.go
@@ -205,23 +205,37 @@ func TestTranslatePolicy(t *testing.T) {
 				},
 			},
 		},
-		"empty principal field": {
+		"allow authenticated": {
 			authzPolicy: `{
-				"name": "authz",
-				"allow_rules": [{
-					"name": "allow_authenticated",
-					"source": {"principals":[]}
-				}]
-			}`,
+						"name": "authz",
+						"allow_rules": [
+						{
+							"name": "allow_authenticated",
+							"source": {
+								"principals":["*", ""]
+							}
+						}]
+					}`,
 			wantPolicies: []*v3rbacpb.RBAC{
 				{
 					Action: v3rbacpb.RBAC_ALLOW,
 					Policies: map[string]*v3rbacpb.Policy{
 						"authz_allow_authenticated": {
 							Principals: []*v3rbacpb.Principal{
-								{Identifier: &v3rbacpb.Principal_Authenticated_{
-									Authenticated: &v3rbacpb.Principal_Authenticated{},
-								}},
+								{Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{
+									Ids: []*v3rbacpb.Principal{
+										{Identifier: &v3rbacpb.Principal_Authenticated_{
+											Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{
+												MatchPattern: &v3matcherpb.StringMatcher_SafeRegex{SafeRegex: &v3matcherpb.RegexMatcher{Regex: ".+"}},
+											}},
+										}},
+										{Identifier: &v3rbacpb.Principal_Authenticated_{
+											Authenticated: &v3rbacpb.Principal_Authenticated{PrincipalName: &v3matcherpb.StringMatcher{
+												MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: ""},
+											}},
+										}},
+									},
+								}}},
 							},
 							Permissions: []*v3rbacpb.Permission{
 								{Rule: &v3rbacpb.Permission_Any{Any: true}},
diff --git a/authz/sdk_end2end_test.go b/authz/sdk_end2end_test.go
index 839faaa76081..b3d449d5dfbd 100644
--- a/authz/sdk_end2end_test.go
+++ b/authz/sdk_end2end_test.go
@@ -261,30 +261,6 @@ var sdkTests = map[string]struct {
 		wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"),
 	},
 	"DeniesRPCRequestWithPrincipalsFieldOnUnauthenticatedConnection": {
-		authzPolicy: `{
-				"name": "authz",
-				"allow_rules":
-				[
-					{
-						"name": "allow_TestServiceCalls",
-						"source": {
-							"principals":
-							[
-								"foo"
-							]
-						},
-						"request": {
-							"paths":
-							[
-								"/grpc.testing.TestService/*"
-							]
-						}
-					}
-				]
-			}`,
-		wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"),
-	},
-	"DeniesRPCRequestWithEmptyPrincipalsOnUnauthenticatedConnection": {
 		authzPolicy: `{
 				"name": "authz",
 				"allow_rules":
@@ -292,7 +268,7 @@ var sdkTests = map[string]struct {
 					{
 						"name": "allow_authenticated",
 						"source": {
-							"principals": []
+							"principals": ["*", ""]
 						}
 					}
 				]
@@ -386,7 +362,7 @@ func (s) TestSDKStaticPolicyEnd2End(t *testing.T) {
 	}
 }
 
-func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnTLSAuthenticatedConnection(t *testing.T) {
+func (s) TestSDKAllowsRPCRequestWithPrincipalsFieldOnTLSAuthenticatedConnection(t *testing.T) {
 	authzPolicy := `{
 				"name": "authz",
 				"allow_rules":
@@ -394,7 +370,7 @@ func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnTLSAuthenticatedConnection(
 					{
 						"name": "allow_authenticated",
 						"source": {
-							"principals": []
+							"principals": ["*", ""]
 						}
 					}
 				]
@@ -438,7 +414,7 @@ func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnTLSAuthenticatedConnection(
 	}
 }
 
-func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnMTLSAuthenticatedConnection(t *testing.T) {
+func (s) TestSDKAllowsRPCRequestWithPrincipalsFieldOnMTLSAuthenticatedConnection(t *testing.T) {
 	authzPolicy := `{
 				"name": "authz",
 				"allow_rules":
@@ -446,7 +422,7 @@ func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnMTLSAuthenticatedConnection
 					{
 						"name": "allow_authenticated",
 						"source": {
-							"principals": []
+							"principals": ["*", ""]
 						}
 					}
 				]