Update conditional matcher ctors and include NotMatcher. (#26256)

src/core/lib/security/authorization/matchers.cc

@@ -22,30 +22,39 @@ namespace grpc_core {
 std::unique_ptr<AuthorizationMatcher> AuthorizationMatcher::Create(
     Rbac::Permission permission) {
   switch (permission.type) {
-    case Rbac::Permission::RuleType::kAnd:
+    case Rbac::Permission::RuleType::kAnd: {
-      return absl::make_unique<AndAuthorizationMatcher>(
+      std::vector<std::unique_ptr<AuthorizationMatcher>> matchers;
-          std::move(permission.permissions), permission.not_rule);
+      for (const auto& rule : permission.permissions) {
-    case Rbac::Permission::RuleType::kOr:
+        matchers.push_back(AuthorizationMatcher::Create(std::move(*rule)));
-      return absl::make_unique<OrAuthorizationMatcher>(
+      }
-          std::move(permission.permissions), permission.not_rule);
+      return absl::make_unique<AndAuthorizationMatcher>(std::move(matchers));
+    }
+    case Rbac::Permission::RuleType::kOr: {
+      std::vector<std::unique_ptr<AuthorizationMatcher>> matchers;
+      for (const auto& rule : permission.permissions) {
+        matchers.push_back(AuthorizationMatcher::Create(std::move(*rule)));
+      }
+      return absl::make_unique<OrAuthorizationMatcher>(std::move(matchers));
+    }
+    case Rbac::Permission::RuleType::kNot:
+      return absl::make_unique<NotAuthorizationMatcher>(
+          AuthorizationMatcher::Create(std::move(*permission.permissions[0])));
     case Rbac::Permission::RuleType::kAny:
-      return absl::make_unique<AlwaysAuthorizationMatcher>(permission.not_rule);
+      return absl::make_unique<AlwaysAuthorizationMatcher>();
     case Rbac::Permission::RuleType::kHeader:
       return absl::make_unique<HeaderAuthorizationMatcher>(
-          std::move(permission.header_matcher), permission.not_rule);
+          std::move(permission.header_matcher));
     case Rbac::Permission::RuleType::kPath:
       return absl::make_unique<PathAuthorizationMatcher>(
-          std::move(permission.string_matcher), permission.not_rule);
+          std::move(permission.string_matcher));
     case Rbac::Permission::RuleType::kDestIp:
       return absl::make_unique<IpAuthorizationMatcher>(
-          IpAuthorizationMatcher::Type::kDestIp, std::move(permission.ip),
+          IpAuthorizationMatcher::Type::kDestIp, std::move(permission.ip));
-          permission.not_rule);
     case Rbac::Permission::RuleType::kDestPort:
-      return absl::make_unique<PortAuthorizationMatcher>(permission.port,
+      return absl::make_unique<PortAuthorizationMatcher>(permission.port);
-                                                         permission.not_rule);
     case Rbac::Permission::RuleType::kReqServerName:
       return absl::make_unique<ReqServerNameAuthorizationMatcher>(
-          std::move(permission.string_matcher), permission.not_rule);
+          std::move(permission.string_matcher));
   }
   return nullptr;
 }
@@ -53,103 +62,78 @@ std::unique_ptr<AuthorizationMatcher> AuthorizationMatcher::Create(
 std::unique_ptr<AuthorizationMatcher> AuthorizationMatcher::Create(
     Rbac::Principal principal) {
   switch (principal.type) {
-    case Rbac::Principal::RuleType::kAnd:
+    case Rbac::Principal::RuleType::kAnd: {
-      return absl::make_unique<AndAuthorizationMatcher>(
+      std::vector<std::unique_ptr<AuthorizationMatcher>> matchers;
-          std::move(principal.principals), principal.not_rule);
+      for (const auto& id : principal.principals) {
-    case Rbac::Principal::RuleType::kOr:
+        matchers.push_back(AuthorizationMatcher::Create(std::move(*id)));
-      return absl::make_unique<OrAuthorizationMatcher>(
+      }
-          std::move(principal.principals), principal.not_rule);
+      return absl::make_unique<AndAuthorizationMatcher>(std::move(matchers));
+    }
+    case Rbac::Principal::RuleType::kOr: {
+      std::vector<std::unique_ptr<AuthorizationMatcher>> matchers;
+      for (const auto& id : principal.principals) {
+        matchers.push_back(AuthorizationMatcher::Create(std::move(*id)));
+      }
+      return absl::make_unique<OrAuthorizationMatcher>(std::move(matchers));
+    }
+    case Rbac::Principal::RuleType::kNot:
+      return absl::make_unique<NotAuthorizationMatcher>(
+          AuthorizationMatcher::Create(std::move(*principal.principals[0])));
     case Rbac::Principal::RuleType::kAny:
-      return absl::make_unique<AlwaysAuthorizationMatcher>(principal.not_rule);
+      return absl::make_unique<AlwaysAuthorizationMatcher>();
     case Rbac::Principal::RuleType::kPrincipalName:
       return absl::make_unique<AuthenticatedAuthorizationMatcher>(
-          std::move(principal.string_matcher), principal.not_rule);
+          std::move(principal.string_matcher));
     case Rbac::Principal::RuleType::kSourceIp:
       return absl::make_unique<IpAuthorizationMatcher>(
-          IpAuthorizationMatcher::Type::kSourceIp, std::move(principal.ip),
+          IpAuthorizationMatcher::Type::kSourceIp, std::move(principal.ip));
-          principal.not_rule);
     case Rbac::Principal::RuleType::kDirectRemoteIp:
       return absl::make_unique<IpAuthorizationMatcher>(
           IpAuthorizationMatcher::Type::kDirectRemoteIp,
-          std::move(principal.ip), principal.not_rule);
+          std::move(principal.ip));
     case Rbac::Principal::RuleType::kRemoteIp:
       return absl::make_unique<IpAuthorizationMatcher>(
-          IpAuthorizationMatcher::Type::kRemoteIp, std::move(principal.ip),
+          IpAuthorizationMatcher::Type::kRemoteIp, std::move(principal.ip));
-          principal.not_rule);
     case Rbac::Principal::RuleType::kHeader:
       return absl::make_unique<HeaderAuthorizationMatcher>(
-          std::move(principal.header_matcher), principal.not_rule);
+          std::move(principal.header_matcher));
     case Rbac::Principal::RuleType::kPath:
       return absl::make_unique<PathAuthorizationMatcher>(
-          std::move(principal.string_matcher), principal.not_rule);
+          std::move(principal.string_matcher));
   }
   return nullptr;
 }
 
-AndAuthorizationMatcher::AndAuthorizationMatcher(
-    std::vector<std::unique_ptr<Rbac::Permission>> rules, bool not_rule)
-    : not_rule_(not_rule) {
-  for (auto& rule : rules) {
-    matchers_.push_back(AuthorizationMatcher::Create(std::move(*rule)));
-  }
-}
-
-AndAuthorizationMatcher::AndAuthorizationMatcher(
-    std::vector<std::unique_ptr<Rbac::Principal>> ids, bool not_rule)
-    : not_rule_(not_rule) {
-  for (const auto& id : ids) {
-    matchers_.push_back(AuthorizationMatcher::Create(std::move(*id)));
-  }
-}
-
 bool AndAuthorizationMatcher::Matches(const EvaluateArgs& args) const {
-  bool matches = true;
   for (const auto& matcher : matchers_) {
     if (!matcher->Matches(args)) {
-      matches = false;
+      return false;
-      break;
     }
   }
-  return matches != not_rule_;
+  return true;
-}
-
-OrAuthorizationMatcher::OrAuthorizationMatcher(
-    std::vector<std::unique_ptr<Rbac::Permission>> rules, bool not_rule)
-    : not_rule_(not_rule) {
-  for (const auto& rule : rules) {
-    matchers_.push_back(AuthorizationMatcher::Create(std::move(*rule)));
-  }
-}
-
-OrAuthorizationMatcher::OrAuthorizationMatcher(
-    std::vector<std::unique_ptr<Rbac::Principal>> ids, bool not_rule)
-    : not_rule_(not_rule) {
-  for (const auto& id : ids) {
-    matchers_.push_back(AuthorizationMatcher::Create(std::move(*id)));
-  }
 }
 
 bool OrAuthorizationMatcher::Matches(const EvaluateArgs& args) const {
-  bool matches = false;
   for (const auto& matcher : matchers_) {
     if (matcher->Matches(args)) {
-      matches = true;
+      return true;
-      break;
     }
   }
-  return matches != not_rule_;
+  return false;
+}
+
+bool NotAuthorizationMatcher::Matches(const EvaluateArgs& args) const {
+  return !matcher_->Matches(args);
 }
 
 bool HeaderAuthorizationMatcher::Matches(const EvaluateArgs& args) const {
   std::string concatenated_value;
-  bool matches =
+  return matcher_.Match(
-      matcher_.Match(args.GetHeaderValue(matcher_.name(), &concatenated_value));
+      args.GetHeaderValue(matcher_.name(), &concatenated_value));
-  return matches != not_rule_;
 }
 
-IpAuthorizationMatcher::IpAuthorizationMatcher(Type type, Rbac::CidrRange range,
+IpAuthorizationMatcher::IpAuthorizationMatcher(Type type, Rbac::CidrRange range)
-                                               bool not_rule)
+    : type_(type), prefix_len_(range.prefix_len) {
-    : type_(type), prefix_len_(range.prefix_len), not_rule_(not_rule) {
   grpc_error_handle error =
       grpc_string_to_sockaddr(&subnet_address_, range.address_prefix.c_str(),
                               /*port does not matter here*/ 0);
@@ -176,59 +160,54 @@ bool IpAuthorizationMatcher::Matches(const EvaluateArgs& args) const {
     }
     default: {
       // Currently we do not support matching rules containing "remote_ip".
-      return not_rule_;
+      return false;
     }
   }
-  bool matches =
+  return grpc_sockaddr_match_subnet(&address, &subnet_address_, prefix_len_);
-      grpc_sockaddr_match_subnet(&address, &subnet_address_, prefix_len_);
-  return matches != not_rule_;
 }
 
 bool PortAuthorizationMatcher::Matches(const EvaluateArgs& args) const {
-  bool matches = (port_ == args.GetLocalPort());
+  return port_ == args.GetLocalPort();
-  return matches != not_rule_;
 }
 
 bool AuthenticatedAuthorizationMatcher::Matches(
     const EvaluateArgs& args) const {
   if (args.GetTransportSecurityType() != GRPC_SSL_TRANSPORT_SECURITY_TYPE) {
     // Connection is not authenticated.
-    return not_rule_;
+    return false;
   }
   if (matcher_.string_matcher().empty()) {
     // Allows any authenticated user.
-    return !not_rule_;
+    return true;
   }
   absl::string_view spiffe_id = args.GetSpiffeId();
   if (!spiffe_id.empty()) {
-    return matcher_.Match(spiffe_id) != not_rule_;
+    return matcher_.Match(spiffe_id);
   }
   std::vector<absl::string_view> dns_sans = args.GetDnsSans();
   if (!dns_sans.empty()) {
     for (const auto& dns : dns_sans) {
       if (matcher_.Match(dns)) {
-        return !not_rule_;
+        return true;
       }
     }
   }
   // TODO(ashithasantosh): Check Subject field from certificate.
-  return not_rule_;
+  return false;
 }
 
 bool ReqServerNameAuthorizationMatcher::Matches(const EvaluateArgs&) const {
   // Currently we do not support matching rules containing
   // "requested_server_name".
-  bool matches = false;
+  return false;
-  return matches != not_rule_;
 }
 
 bool PathAuthorizationMatcher::Matches(const EvaluateArgs& args) const {
-  bool matches = false;
   absl::string_view path = args.GetPath();
   if (!path.empty()) {
-    matches = matcher_.Match(path);
+    return matcher_.Match(path);
   }
-  return matches != not_rule_;
+  return false;
 }
 
 bool PolicyAuthorizationMatcher::Matches(const EvaluateArgs& args) const {

src/core/lib/security/authorization/matchers.h

@@ -47,46 +47,46 @@ class AuthorizationMatcher {
 
 class AlwaysAuthorizationMatcher : public AuthorizationMatcher {
  public:
-  explicit AlwaysAuthorizationMatcher(bool not_rule = false)
+  explicit AlwaysAuthorizationMatcher() = default;
-      : not_rule_(not_rule) {}
 
-  bool Matches(const EvaluateArgs&) const override { return !not_rule_; }
+  bool Matches(const EvaluateArgs&) const override { return true; }
-
- private:
-  // Negates matching the provided permission/principal.
-  const bool not_rule_;
 };
 
 class AndAuthorizationMatcher : public AuthorizationMatcher {
  public:
   explicit AndAuthorizationMatcher(
-      std::vector<std::unique_ptr<Rbac::Permission>> rules,
+      std::vector<std::unique_ptr<AuthorizationMatcher>> matchers)
-      bool not_rule = false);
+      : matchers_(std::move(matchers)) {}
-  explicit AndAuthorizationMatcher(
-      std::vector<std::unique_ptr<Rbac::Principal>> ids, bool not_rule = false);
 
   bool Matches(const EvaluateArgs& args) const override;
 
  private:
   std::vector<std::unique_ptr<AuthorizationMatcher>> matchers_;
-  // Negates matching the provided permission/principal.
-  const bool not_rule_;
 };
 
 class OrAuthorizationMatcher : public AuthorizationMatcher {
  public:
   explicit OrAuthorizationMatcher(
-      std::vector<std::unique_ptr<Rbac::Permission>> rules,
+      std::vector<std::unique_ptr<AuthorizationMatcher>> matchers)
-      bool not_rule = false);
+      : matchers_(std::move(matchers)) {}
-  explicit OrAuthorizationMatcher(
-      std::vector<std::unique_ptr<Rbac::Principal>> ids, bool not_rule = false);
 
   bool Matches(const EvaluateArgs& args) const override;
 
  private:
   std::vector<std::unique_ptr<AuthorizationMatcher>> matchers_;
-  // Negates matching the provided permission/principal.
+};
-  const bool not_rule_;
+
+// Negates matching the provided permission/principal.
+class NotAuthorizationMatcher : public AuthorizationMatcher {
+ public:
+  explicit NotAuthorizationMatcher(
+      std::unique_ptr<AuthorizationMatcher> matcher)
+      : matcher_(std::move(matcher)) {}
+
+  bool Matches(const EvaluateArgs& args) const override;
+
+ private:
+  std::unique_ptr<AuthorizationMatcher> matcher_;
 };
 
 // TODO(ashithasantosh): Add matcher implementation for metadata field.
@@ -94,16 +94,13 @@ class OrAuthorizationMatcher : public AuthorizationMatcher {
 // Perform a match against HTTP headers.
 class HeaderAuthorizationMatcher : public AuthorizationMatcher {
  public:
-  explicit HeaderAuthorizationMatcher(HeaderMatcher matcher,
+  explicit HeaderAuthorizationMatcher(HeaderMatcher matcher)
-                                      bool not_rule = false)
+      : matcher_(std::move(matcher)) {}
-      : matcher_(std::move(matcher)), not_rule_(not_rule) {}
 
   bool Matches(const EvaluateArgs& args) const override;
 
  private:
   const HeaderMatcher matcher_;
-  // Negates matching the provided permission/principal.
-  const bool not_rule_;
 };
 
 // Perform a match against IP Cidr Range.
@@ -116,8 +113,7 @@ class IpAuthorizationMatcher : public AuthorizationMatcher {
     kRemoteIp,
   };
 
-  IpAuthorizationMatcher(Type type, Rbac::CidrRange range,
+  IpAuthorizationMatcher(Type type, Rbac::CidrRange range);
-                         bool not_rule = false);
 
   bool Matches(const EvaluateArgs& args) const override;
 
@@ -126,38 +122,30 @@ class IpAuthorizationMatcher : public AuthorizationMatcher {
   // Subnet masked address.
   grpc_resolved_address subnet_address_;
   const uint32_t prefix_len_;
-  // Negates matching the provided permission/principal.
-  const bool not_rule_;
 };
 
 // Perform a match against port number of the destination (local) address.
 class PortAuthorizationMatcher : public AuthorizationMatcher {
  public:
-  explicit PortAuthorizationMatcher(int port, bool not_rule = false)
+  explicit PortAuthorizationMatcher(int port) : port_(port) {}
-      : port_(port), not_rule_(not_rule) {}
 
   bool Matches(const EvaluateArgs& args) const override;
 
  private:
   const int port_;
-  // Negates matching the provided permission/principal.
-  const bool not_rule_;
 };
 
 // Matches the principal name as described in the peer certificate. Uses URI SAN
 // or DNS SAN in that order, otherwise uses subject field.
 class AuthenticatedAuthorizationMatcher : public AuthorizationMatcher {
  public:
-  explicit AuthenticatedAuthorizationMatcher(StringMatcher auth,
+  explicit AuthenticatedAuthorizationMatcher(StringMatcher auth)
-                                             bool not_rule = false)
+      : matcher_(std::move(auth)) {}
-      : matcher_(std::move(auth)), not_rule_(not_rule) {}
 
   bool Matches(const EvaluateArgs& args) const override;
 
  private:
   const StringMatcher matcher_;
-  // Negates matching the provided permission/principal.
-  const bool not_rule_;
 };
 
 // Perform a match against the request server from the client's connection
@@ -165,29 +153,25 @@ class AuthenticatedAuthorizationMatcher : public AuthorizationMatcher {
 class ReqServerNameAuthorizationMatcher : public AuthorizationMatcher {
  public:
   explicit ReqServerNameAuthorizationMatcher(
-      StringMatcher requested_server_name, bool not_rule = false)
+      StringMatcher requested_server_name)
-      : matcher_(std::move(requested_server_name)), not_rule_(not_rule) {}
+      : matcher_(std::move(requested_server_name)) {}
 
   bool Matches(const EvaluateArgs&) const override;
 
  private:
   const StringMatcher matcher_;
-  // Negates matching the provided permission/principal.
-  const bool not_rule_;
 };
 
 // Perform a match against the path header of HTTP request.
 class PathAuthorizationMatcher : public AuthorizationMatcher {
  public:
-  explicit PathAuthorizationMatcher(StringMatcher path, bool not_rule = false)
+  explicit PathAuthorizationMatcher(StringMatcher path)
-      : matcher_(std::move(path)), not_rule_(not_rule) {}
+      : matcher_(std::move(path)) {}
 
   bool Matches(const EvaluateArgs& args) const override;
 
  private:
   const StringMatcher matcher_;
-  // Negates matching the provided permission/principal.
-  const bool not_rule_;
 };
 
 // Performs a match for policy field in RBAC, which is a collection of

src/core/lib/security/authorization/rbac_policy.cc

@@ -77,31 +77,31 @@ std::string Rbac::CidrRange::ToString() const {
 
 Rbac::Permission::Permission(
     Permission::RuleType type,
-    std::vector<std::unique_ptr<Permission>> permissions, bool not_rule)
+    std::vector<std::unique_ptr<Permission>> permissions)
-    : type(type), permissions(std::move(permissions)), not_rule(not_rule) {}
+    : type(type), permissions(std::move(permissions)) {}
-Rbac::Permission::Permission(Permission::RuleType type, bool not_rule)
+Rbac::Permission::Permission(Permission::RuleType type, Permission permission)
-    : type(type), not_rule(not_rule) {}
+    : type(type) {
+  permissions.push_back(
+      absl::make_unique<Rbac::Permission>(std::move(permission)));
+}
+Rbac::Permission::Permission(Permission::RuleType type) : type(type) {}
 Rbac::Permission::Permission(Permission::RuleType type,
-                             HeaderMatcher header_matcher, bool not_rule)
+                             HeaderMatcher header_matcher)
-    : type(type),
+    : type(type), header_matcher(std::move(header_matcher)) {}
-      header_matcher(std::move(header_matcher)),
-      not_rule(not_rule) {}
 Rbac::Permission::Permission(Permission::RuleType type,
-                             StringMatcher string_matcher, bool not_rule)
+                             StringMatcher string_matcher)
-    : type(type),
+    : type(type), string_matcher(std::move(string_matcher)) {}
-      string_matcher(std::move(string_matcher)),
+Rbac::Permission::Permission(Permission::RuleType type, CidrRange ip)
-      not_rule(not_rule) {}
+    : type(type), ip(std::move(ip)) {}
-Rbac::Permission::Permission(Permission::RuleType type, CidrRange ip,
+Rbac::Permission::Permission(Permission::RuleType type, int port)
-                             bool not_rule)
+    : type(type), port(port) {}
-    : type(type), ip(std::move(ip)), not_rule(not_rule) {}
-Rbac::Permission::Permission(Permission::RuleType type, int port, bool not_rule)
-    : type(type), port(port), not_rule(not_rule) {}
 
 Rbac::Permission::Permission(Rbac::Permission&& other) noexcept
-    : type(other.type), not_rule(other.not_rule) {
+    : type(other.type) {
   switch (type) {
     case RuleType::kAnd:
     case RuleType::kOr:
+    case RuleType::kNot:
       permissions = std::move(other.permissions);
       break;
     case RuleType::kAny:
@@ -124,10 +124,10 @@ Rbac::Permission::Permission(Rbac::Permission&& other) noexcept
 Rbac::Permission& Rbac::Permission::operator=(
     Rbac::Permission&& other) noexcept {
   type = other.type;
-  not_rule = other.not_rule;
   switch (type) {
     case RuleType::kAnd:
     case RuleType::kOr:
+    case RuleType::kNot:
       permissions = std::move(other.permissions);
       break;
     case RuleType::kAny:
@@ -156,8 +156,7 @@ std::string Rbac::Permission::ToString() const {
       for (const auto& permission : permissions) {
         contents.push_back(permission->ToString());
       }
-      return absl::StrFormat("%sand=[%s]", not_rule ? "not " : "",
+      return absl::StrFormat("and=[%s]", absl::StrJoin(contents, ","));
-                             absl::StrJoin(contents, ","));
     }
     case RuleType::kOr: {
       std::vector<std::string> contents;
@@ -165,25 +164,23 @@ std::string Rbac::Permission::ToString() const {
       for (const auto& permission : permissions) {
         contents.push_back(permission->ToString());
       }
-      return absl::StrFormat("%sor=[%s]", not_rule ? "not " : "",
+      return absl::StrFormat("or=[%s]", absl::StrJoin(contents, ","));
-                             absl::StrJoin(contents, ","));
     }
+    case RuleType::kNot:
+      return absl::StrFormat("not %s", permissions[0]->ToString());
     case RuleType::kAny:
-      return absl::StrFormat("%sany", not_rule ? "not " : "");
+      return "any";
     case RuleType::kHeader:
-      return absl::StrFormat("%sheader=%s", not_rule ? "not " : "",
+      return absl::StrFormat("header=%s", header_matcher.ToString());
-                             header_matcher.ToString());
     case RuleType::kPath:
-      return absl::StrFormat("%spath=%s", not_rule ? "not " : "",
+      return absl::StrFormat("path=%s", string_matcher.ToString());
-                             string_matcher.ToString());
     case RuleType::kDestIp:
-      return absl::StrFormat("%sdest_ip=%s", not_rule ? "not " : "",
+      return absl::StrFormat("dest_ip=%s", ip.ToString());
-                             ip.ToString());
     case RuleType::kDestPort:
-      return absl::StrFormat("%sdest_port=%d", not_rule ? "not " : "", port);
+      return absl::StrFormat("dest_port=%d", port);
     case RuleType::kReqServerName:
-      return absl::StrFormat("%srequested_server_name=%s",
+      return absl::StrFormat("requested_server_name=%s",
-                             not_rule ? "not " : "", string_matcher.ToString());
+                             string_matcher.ToString());
     default:
       return "";
   }
@@ -194,30 +191,29 @@ std::string Rbac::Permission::ToString() const {
 //
 
 Rbac::Principal::Principal(Principal::RuleType type,
-                           std::vector<std::unique_ptr<Principal>> principals,
+                           std::vector<std::unique_ptr<Principal>> principals)
-                           bool not_rule)
+    : type(type), principals(std::move(principals)) {}
-    : type(type), principals(std::move(principals)), not_rule(not_rule) {}
+Rbac::Principal::Principal(Principal::RuleType type, Principal principal)
-Rbac::Principal::Principal(Principal::RuleType type, bool not_rule)
+    : type(type) {
-    : type(type), not_rule(not_rule) {}
+  principals.push_back(
+      absl::make_unique<Rbac::Principal>(std::move(principal)));
+}
+Rbac::Principal::Principal(Principal::RuleType type) : type(type) {}
 Rbac::Principal::Principal(Principal::RuleType type,
-                           StringMatcher string_matcher, bool not_rule)
+                           StringMatcher string_matcher)
-    : type(type),
+    : type(type), string_matcher(std::move(string_matcher)) {}
-      string_matcher(std::move(string_matcher)),
+Rbac::Principal::Principal(Principal::RuleType type, CidrRange ip)
-      not_rule(not_rule) {}
+    : type(type), ip(std::move(ip)) {}
-Rbac::Principal::Principal(Principal::RuleType type, CidrRange ip,
-                           bool not_rule)
-    : type(type), ip(std::move(ip)), not_rule(not_rule) {}
 Rbac::Principal::Principal(Principal::RuleType type,
-                           HeaderMatcher header_matcher, bool not_rule)
+                           HeaderMatcher header_matcher)
-    : type(type),
+    : type(type), header_matcher(std::move(header_matcher)) {}
-      header_matcher(std::move(header_matcher)),
-      not_rule(not_rule) {}
 
 Rbac::Principal::Principal(Rbac::Principal&& other) noexcept
-    : type(other.type), not_rule(other.not_rule) {
+    : type(other.type) {
   switch (type) {
     case RuleType::kAnd:
     case RuleType::kOr:
+    case RuleType::kNot:
       principals = std::move(other.principals);
       break;
     case RuleType::kAny:
@@ -236,10 +232,10 @@ Rbac::Principal::Principal(Rbac::Principal&& other) noexcept
 
 Rbac::Principal& Rbac::Principal::operator=(Rbac::Principal&& other) noexcept {
   type = other.type;
-  not_rule = other.not_rule;
   switch (type) {
     case RuleType::kAnd:
     case RuleType::kOr:
+    case RuleType::kNot:
       principals = std::move(other.principals);
       break;
     case RuleType::kAny:
@@ -265,8 +261,7 @@ std::string Rbac::Principal::ToString() const {
       for (const auto& principal : principals) {
         contents.push_back(principal->ToString());
       }
-      return absl::StrFormat("%sand=[%s]", not_rule ? "not " : "",
+      return absl::StrFormat("and=[%s]", absl::StrJoin(contents, ","));
-                             absl::StrJoin(contents, ","));
     }
     case RuleType::kOr: {
       std::vector<std::string> contents;
@@ -274,29 +269,24 @@ std::string Rbac::Principal::ToString() const {
       for (const auto& principal : principals) {
         contents.push_back(principal->ToString());
       }
-      return absl::StrFormat("%sor=[%s]", not_rule ? "not " : "",
+      return absl::StrFormat("or=[%s]", absl::StrJoin(contents, ","));
-                             absl::StrJoin(contents, ","));
     }
+    case RuleType::kNot:
+      return absl::StrFormat("not %s", principals[0]->ToString());
     case RuleType::kAny:
-      return absl::StrFormat("%sany", not_rule ? "not " : "");
+      return "any";
     case RuleType::kPrincipalName:
-      return absl::StrFormat("%sprincipal_name=%s", not_rule ? "not " : "",
+      return absl::StrFormat("principal_name=%s", string_matcher.ToString());
-                             string_matcher.ToString());
     case RuleType::kSourceIp:
-      return absl::StrFormat("%ssource_ip=%s", not_rule ? "not " : "",
+      return absl::StrFormat("source_ip=%s", ip.ToString());
-                             ip.ToString());
     case RuleType::kDirectRemoteIp:
-      return absl::StrFormat("%sdirect_remote_ip=%s", not_rule ? "not " : "",
+      return absl::StrFormat("direct_remote_ip=%s", ip.ToString());
-                             ip.ToString());
     case RuleType::kRemoteIp:
-      return absl::StrFormat("%sremote_ip=%s", not_rule ? "not " : "",
+      return absl::StrFormat("remote_ip=%s", ip.ToString());
-                             ip.ToString());
     case RuleType::kHeader:
-      return absl::StrFormat("%sheader=%s", not_rule ? "not " : "",
+      return absl::StrFormat("header=%s", header_matcher.ToString());
-                             header_matcher.ToString());
     case RuleType::kPath:
-      return absl::StrFormat("%spath=%s", not_rule ? "not " : "",
+      return absl::StrFormat("path=%s", string_matcher.ToString());
-                             string_matcher.ToString());
     default:
       return "";
   }

src/core/lib/security/authorization/rbac_policy.h

@@ -49,6 +49,7 @@ struct Rbac {
     enum class RuleType {
       kAnd,
       kOr,
+      kNot,
       kAny,
       kHeader,
       kPath,
@@ -60,20 +61,19 @@ struct Rbac {
     Permission() = default;
     // For kAnd/kOr RuleType.
     Permission(Permission::RuleType type,
-               std::vector<std::unique_ptr<Permission>> permissions,
+               std::vector<std::unique_ptr<Permission>> permissions);
-               bool not_rule = false);
+    // For kNot RuleType.
+    Permission(Permission::RuleType type, Permission permission);
     // For kAny RuleType.
-    explicit Permission(Permission::RuleType type, bool not_rule = false);
+    explicit Permission(Permission::RuleType type);
     // For kHeader RuleType.
-    Permission(Permission::RuleType type, HeaderMatcher header_matcher,
+    Permission(Permission::RuleType type, HeaderMatcher header_matcher);
-               bool not_rule = false);
     // For kPath/kReqServerName RuleType.
-    Permission(Permission::RuleType type, StringMatcher string_matcher,
+    Permission(Permission::RuleType type, StringMatcher string_matcher);
-               bool not_rule = false);
     // For kDestIp RuleType.
-    Permission(Permission::RuleType type, CidrRange ip, bool not_rule = false);
+    Permission(Permission::RuleType type, CidrRange ip);
     // For kDestPort RuleType.
-    Permission(Permission::RuleType type, int port, bool not_rule = false);
+    Permission(Permission::RuleType type, int port);
 
     Permission(Permission&& other) noexcept;
     Permission& operator=(Permission&& other) noexcept;
@@ -85,15 +85,16 @@ struct Rbac {
     StringMatcher string_matcher;
     CidrRange ip;
     int port;
-    // For type kAnd/kOr.
+    // For type kAnd/kOr/kNot. For kNot type, the vector will have only one
+    // element.
     std::vector<std::unique_ptr<Permission>> permissions;
-    bool not_rule = false;
   };
 
   struct Principal {
     enum class RuleType {
       kAnd,
       kOr,
+      kNot,
       kAny,
       kPrincipalName,
       kSourceIp,
@@ -106,18 +107,17 @@ struct Rbac {
     Principal() = default;
     // For kAnd/kOr RuleType.
     Principal(Principal::RuleType type,
-              std::vector<std::unique_ptr<Principal>> principals,
+              std::vector<std::unique_ptr<Principal>> principals);
-              bool not_rule = false);
+    // For kNot RuleType.
+    Principal(Principal::RuleType type, Principal principal);
     // For kAny RuleType.
-    explicit Principal(Principal::RuleType type, bool not_rule = false);
+    explicit Principal(Principal::RuleType type);
     // For kPrincipalName/kPath RuleType.
-    Principal(Principal::RuleType type, StringMatcher string_matcher,
+    Principal(Principal::RuleType type, StringMatcher string_matcher);
-              bool not_rule = false);
     // For kSourceIp/kDirectRemoteIp/kRemoteIp RuleType.
-    Principal(Principal::RuleType type, CidrRange ip, bool not_rule = false);
+    Principal(Principal::RuleType type, CidrRange ip);
     // For kHeader RuleType.
-    Principal(Principal::RuleType type, HeaderMatcher header_matcher,
+    Principal(Principal::RuleType type, HeaderMatcher header_matcher);
-              bool not_rule = false);
 
     Principal(Principal&& other) noexcept;
     Principal& operator=(Principal&& other) noexcept;
@@ -128,9 +128,9 @@ struct Rbac {
     HeaderMatcher header_matcher;
     StringMatcher string_matcher;
     CidrRange ip;
-    // For type kAnd/kOr.
+    // For type kAnd/kOr/kNot. For kNot type, the vector will have only one
+    // element.
     std::vector<std::unique_ptr<Principal>> principals;
-    bool not_rule = false;
   };
 
   struct Policy {

test/core/security/authorization_matchers_test.cc

@@ -36,12 +36,6 @@ TEST_F(AuthorizationMatchersTest, AlwaysAuthorizationMatcher) {
   EXPECT_TRUE(matcher.Matches(args));
 }
 
-TEST_F(AuthorizationMatchersTest, NotAlwaysAuthorizationMatcher) {
-  EvaluateArgs args = args_.MakeEvaluateArgs();
-  AlwaysAuthorizationMatcher matcher(/*not_rule=*/true);
-  EXPECT_FALSE(matcher.Matches(args));
-}
-
 TEST_F(AuthorizationMatchersTest, AndAuthorizationMatcherSuccessfulMatch) {
   args_.AddPairToMetadata("foo", "bar");
   args_.SetLocalEndpoint("ipv4:255.255.255.255:123");
@@ -54,8 +48,9 @@ TEST_F(AuthorizationMatchersTest, AndAuthorizationMatcherSuccessfulMatch) {
           .value()));
   rules.push_back(absl::make_unique<Rbac::Permission>(
       Rbac::Permission::RuleType::kDestPort, /*port=*/123));
-  AndAuthorizationMatcher matcher(std::move(rules));
+  auto matcher = AuthorizationMatcher::Create(
-  EXPECT_TRUE(matcher.Matches(args));
+      Rbac::Permission(Rbac::Permission::RuleType::kAnd, std::move(rules)));
+  EXPECT_TRUE(matcher->Matches(args));
 }
 
 TEST_F(AuthorizationMatchersTest, AndAuthorizationMatcherFailedMatch) {
@@ -70,23 +65,10 @@ TEST_F(AuthorizationMatchersTest, AndAuthorizationMatcherFailedMatch) {
           .value()));
   rules.push_back(absl::make_unique<Rbac::Permission>(
       Rbac::Permission::RuleType::kDestPort, /*port=*/123));
-  AndAuthorizationMatcher matcher(std::move(rules));
+  auto matcher = AuthorizationMatcher::Create(
+      Rbac::Permission(Rbac::Permission::RuleType::kAnd, std::move(rules)));
   // Header rule fails. Expected value "bar", got "not_bar" for key "foo".
-  EXPECT_FALSE(matcher.Matches(args));
+  EXPECT_FALSE(matcher->Matches(args));
-}
-
-TEST_F(AuthorizationMatchersTest, NotAndAuthorizationMatcher) {
-  args_.AddPairToMetadata(":path", "/expected/foo");
-  EvaluateArgs args = args_.MakeEvaluateArgs();
-  std::vector<std::unique_ptr<Rbac::Permission>> ids;
-  ids.push_back(absl::make_unique<Rbac::Permission>(
-      Rbac::Permission::RuleType::kPath,
-      StringMatcher::Create(StringMatcher::Type::kExact,
-                            /*matcher=*/"/expected/foo",
-                            /*case_sensitive=*/false)
-          .value()));
-  AndAuthorizationMatcher matcher(std::move(ids), /*not_rule=*/true);
-  EXPECT_FALSE(matcher.Matches(args));
 }
 
 TEST_F(AuthorizationMatchersTest, OrAuthorizationMatcherSuccessfulMatch) {
@@ -101,9 +83,10 @@ TEST_F(AuthorizationMatchersTest, OrAuthorizationMatcherSuccessfulMatch) {
           .value()));
   rules.push_back(absl::make_unique<Rbac::Permission>(
       Rbac::Permission::RuleType::kDestPort, /*port=*/456));
-  OrAuthorizationMatcher matcher(std::move(rules));
+  auto matcher = AuthorizationMatcher::Create(
+      Rbac::Permission(Rbac::Permission::RuleType::kOr, std::move(rules)));
   // Matches as header rule matches even though port rule fails.
-  EXPECT_TRUE(matcher.Matches(args));
+  EXPECT_TRUE(matcher->Matches(args));
 }
 
 TEST_F(AuthorizationMatchersTest, OrAuthorizationMatcherFailedMatch) {
@@ -115,22 +98,36 @@ TEST_F(AuthorizationMatchersTest, OrAuthorizationMatcherFailedMatch) {
       HeaderMatcher::Create(/*name=*/"foo", HeaderMatcher::Type::kExact,
                             /*matcher=*/"bar")
           .value()));
-  OrAuthorizationMatcher matcher(std::move(rules));
+  auto matcher = AuthorizationMatcher::Create(
+      Rbac::Permission(Rbac::Permission::RuleType::kOr, std::move(rules)));
   // Header rule fails. Expected value "bar", got "not_bar" for key "foo".
-  EXPECT_FALSE(matcher.Matches(args));
+  EXPECT_FALSE(matcher->Matches(args));
 }
 
-TEST_F(AuthorizationMatchersTest, NotOrAuthorizationMatcher) {
+TEST_F(AuthorizationMatchersTest, NotAuthorizationMatcherSuccessfulMatch) {
-  args_.AddPairToMetadata("foo", "not_bar");
+  args_.AddPairToMetadata(":path", "/different/foo");
   EvaluateArgs args = args_.MakeEvaluateArgs();
-  std::vector<std::unique_ptr<Rbac::Permission>> rules;
+  auto matcher = AuthorizationMatcher::Create(Rbac::Principal(
-  rules.push_back(absl::make_unique<Rbac::Permission>(
+      Rbac::Principal::RuleType::kNot,
-      Rbac::Permission::RuleType::kHeader,
+      Rbac::Principal(Rbac::Principal::RuleType::kPath,
-      HeaderMatcher::Create(/*name=*/"foo", HeaderMatcher::Type::kExact,
+                      StringMatcher::Create(StringMatcher::Type::kExact,
-                            /*matcher=*/"bar")
+                                            /*matcher=*/"/expected/foo",
-          .value()));
+                                            /*case_sensitive=*/false)
-  OrAuthorizationMatcher matcher(std::move(rules), /*not_rule=*/true);
+                          .value())));
-  EXPECT_TRUE(matcher.Matches(args));
+  EXPECT_TRUE(matcher->Matches(args));
+}
+
+TEST_F(AuthorizationMatchersTest, NotAuthorizationMatcherFailedMatch) {
+  args_.AddPairToMetadata(":path", "/expected/foo");
+  EvaluateArgs args = args_.MakeEvaluateArgs();
+  auto matcher = AuthorizationMatcher::Create(Rbac::Principal(
+      Rbac::Principal::RuleType::kNot,
+      Rbac::Principal(Rbac::Principal::RuleType::kPath,
+                      StringMatcher::Create(StringMatcher::Type::kExact,
+                                            /*matcher=*/"/expected/foo",
+                                            /*case_sensitive=*/false)
+                          .value())));
+  EXPECT_FALSE(matcher->Matches(args));
 }
 
 TEST_F(AuthorizationMatchersTest, HybridAuthorizationMatcherSuccessfulMatch) {
@@ -151,8 +148,9 @@ TEST_F(AuthorizationMatchersTest, HybridAuthorizationMatcherSuccessfulMatch) {
       Rbac::Permission::RuleType::kAnd, std::move(sub_and_rules)));
   and_rules.push_back(absl::make_unique<Rbac::Permission>(
       Rbac::Permission::RuleType::kOr, std::move(std::move(sub_or_rules))));
-  AndAuthorizationMatcher matcher(std::move(and_rules));
+  auto matcher = AuthorizationMatcher::Create(
-  EXPECT_TRUE(matcher.Matches(args));
+      Rbac::Permission(Rbac::Permission::RuleType::kAnd, std::move(and_rules)));
+  EXPECT_TRUE(matcher->Matches(args));
 }
 
 TEST_F(AuthorizationMatchersTest, HybridAuthorizationMatcherFailedMatch) {
@@ -178,9 +176,10 @@ TEST_F(AuthorizationMatchersTest, HybridAuthorizationMatcherFailedMatch) {
       Rbac::Permission::RuleType::kAnd, std::move(sub_and_rules)));
   and_rules.push_back(absl::make_unique<Rbac::Permission>(
       Rbac::Permission::RuleType::kOr, std::move(std::move(sub_or_rules))));
-  AndAuthorizationMatcher matcher(std::move(and_rules));
+  auto matcher = AuthorizationMatcher::Create(
+      Rbac::Permission(Rbac::Permission::RuleType::kAnd, std::move(and_rules)));
   // Fails as "absent_key" header was not present.
-  EXPECT_FALSE(matcher.Matches(args));
+  EXPECT_FALSE(matcher->Matches(args));
 }
 
 TEST_F(AuthorizationMatchersTest, PathAuthorizationMatcherSuccessfulMatch) {
@@ -205,18 +204,6 @@ TEST_F(AuthorizationMatchersTest, PathAuthorizationMatcherFailedMatch) {
   EXPECT_FALSE(matcher.Matches(args));
 }
 
-TEST_F(AuthorizationMatchersTest, NotPathAuthorizationMatcher) {
-  args_.AddPairToMetadata(":path", "expected/path");
-  EvaluateArgs args = args_.MakeEvaluateArgs();
-  PathAuthorizationMatcher matcher(
-      StringMatcher::Create(StringMatcher::Type::kExact,
-                            /*matcher=*/"expected/path",
-                            /*case_sensitive=*/false)
-          .value(),
-      /*not_rule=*/true);
-  EXPECT_FALSE(matcher.Matches(args));
-}
-
 TEST_F(AuthorizationMatchersTest,
        PathAuthorizationMatcherFailedMatchMissingPath) {
   EvaluateArgs args = args_.MakeEvaluateArgs();
@@ -270,17 +257,6 @@ TEST_F(AuthorizationMatchersTest,
   EXPECT_FALSE(matcher.Matches(args));
 }
 
-TEST_F(AuthorizationMatchersTest, NotHeaderAuthorizationMatcher) {
-  args_.AddPairToMetadata("key123", "foo");
-  EvaluateArgs args = args_.MakeEvaluateArgs();
-  HeaderAuthorizationMatcher matcher(
-      HeaderMatcher::Create(/*name=*/"key123", HeaderMatcher::Type::kExact,
-                            /*matcher=*/"bar")
-          .value(),
-      /*not_rule=*/true);
-  EXPECT_TRUE(matcher.Matches(args));
-}
-
 TEST_F(AuthorizationMatchersTest,
        IpAuthorizationMatcherLocalIpSuccessfulMatch) {
   args_.SetLocalEndpoint("ipv4:1.2.3.4:123");
@@ -325,16 +301,6 @@ TEST_F(AuthorizationMatchersTest,
   EXPECT_FALSE(matcher.Matches(args));
 }
 
-TEST_F(AuthorizationMatchersTest, NotIpAuthorizationMatcherSuccessfulMatch) {
-  args_.SetLocalEndpoint("ipv4:1.2.3.4:123");
-  EvaluateArgs args = args_.MakeEvaluateArgs();
-  IpAuthorizationMatcher matcher(
-      IpAuthorizationMatcher::Type::kDestIp,
-      Rbac::CidrRange(/*address_prefix=*/"1.7.8.9", /*prefix_len=*/8),
-      /*not_rule=*/true);
-  EXPECT_FALSE(matcher.Matches(args));
-}
-
 TEST_F(AuthorizationMatchersTest, PortAuthorizationMatcherSuccessfulMatch) {
   args_.SetLocalEndpoint("ipv4:255.255.255.255:123");
   EvaluateArgs args = args_.MakeEvaluateArgs();
@@ -349,13 +315,6 @@ TEST_F(AuthorizationMatchersTest, PortAuthorizationMatcherFailedMatch) {
   EXPECT_FALSE(matcher.Matches(args));
 }
 
-TEST_F(AuthorizationMatchersTest, NotPortAuthorizationMatcher) {
-  args_.SetLocalEndpoint("ipv4:255.255.255.255:123");
-  EvaluateArgs args = args_.MakeEvaluateArgs();
-  PortAuthorizationMatcher matcher(/*port=*/123, /*not_rule=*/true);
-  EXPECT_FALSE(matcher.Matches(args));
-}
-
 TEST_F(AuthorizationMatchersTest,
        AuthenticatedMatcherUnAuthenticatedConnection) {
   EvaluateArgs args = args_.MakeEvaluateArgs();
@@ -451,18 +410,6 @@ TEST_F(AuthorizationMatchersTest, AuthenticatedMatcherFailedNothingMatches) {
   EXPECT_FALSE(matcher.Matches(args));
 }
 
-TEST_F(AuthorizationMatchersTest, NotAuthenticatedMatcher) {
-  args_.AddPropertyToAuthContext(GRPC_TRANSPORT_SECURITY_TYPE_PROPERTY_NAME,
-                                 GRPC_SSL_TRANSPORT_SECURITY_TYPE);
-  EvaluateArgs args = args_.MakeEvaluateArgs();
-  AuthenticatedAuthorizationMatcher matcher(
-      StringMatcher::Create(StringMatcher::Type::kExact, /*matcher=*/"foo",
-                            /*case_sensitive=*/false)
-          .value(),
-      /*not_rule=*/true);
-  EXPECT_TRUE(matcher.Matches(args));
-}
-
 TEST_F(AuthorizationMatchersTest, PolicyAuthorizationMatcherSuccessfulMatch) {
   args_.AddPairToMetadata("key123", "foo");
   EvaluateArgs args = args_.MakeEvaluateArgs();

test/core/security/grpc_authorization_engine_test.cc

@@ -23,8 +23,10 @@ namespace grpc_core {
 
 TEST(GrpcAuthorizationEngineTest, AllowEngineWithMatchingPolicy) {
   Rbac::Policy policy1(
-      Rbac::Permission(Rbac::Permission::RuleType::kAny, /*not_rule=*/true),
+      Rbac::Permission(Rbac::Permission::RuleType::kNot,
-      Rbac::Principal(Rbac::Principal::RuleType::kAny, /*not_rule=*/true));
+                       Rbac::Permission(Rbac::Permission::RuleType::kAny)),
+      Rbac::Principal(Rbac::Principal::RuleType::kNot,
+                      Rbac::Principal(Rbac::Principal::RuleType::kAny)));
   Rbac::Policy policy2((Rbac::Permission(Rbac::Permission::RuleType::kAny)),
                        (Rbac::Principal(Rbac::Principal::RuleType::kAny)));
   std::map<std::string, Rbac::Policy> policies;
@@ -40,8 +42,10 @@ TEST(GrpcAuthorizationEngineTest, AllowEngineWithMatchingPolicy) {
 
 TEST(GrpcAuthorizationEngineTest, AllowEngineWithNoMatchingPolicy) {
   Rbac::Policy policy1(
-      Rbac::Permission(Rbac::Permission::RuleType::kAny, /*not_rule=*/true),
+      Rbac::Permission(Rbac::Permission::RuleType::kNot,
-      Rbac::Principal(Rbac::Principal::RuleType::kAny, /*not_rule=*/true));
+                       Rbac::Permission(Rbac::Permission::RuleType::kAny)),
+      Rbac::Principal(Rbac::Principal::RuleType::kNot,
+                      Rbac::Principal(Rbac::Principal::RuleType::kAny)));
   std::map<std::string, Rbac::Policy> policies;
   policies["policy1"] = std::move(policy1);
   Rbac rbac(Rbac::Action::kAllow, std::move(policies));
@@ -62,8 +66,10 @@ TEST(GrpcAuthorizationEngineTest, AllowEngineWithEmptyPolicies) {
 
 TEST(GrpcAuthorizationEngineTest, DenyEngineWithMatchingPolicy) {
   Rbac::Policy policy1(
-      Rbac::Permission(Rbac::Permission::RuleType::kAny, /*not_rule=*/true),
+      Rbac::Permission(Rbac::Permission::RuleType::kNot,
-      Rbac::Principal(Rbac::Principal::RuleType::kAny, /*not_rule=*/true));
+                       Rbac::Permission(Rbac::Permission::RuleType::kAny)),
+      Rbac::Principal(Rbac::Principal::RuleType::kNot,
+                      Rbac::Principal(Rbac::Principal::RuleType::kAny)));
   Rbac::Policy policy2((Rbac::Permission(Rbac::Permission::RuleType::kAny)),
                        (Rbac::Principal(Rbac::Principal::RuleType::kAny)));
   std::map<std::string, Rbac::Policy> policies;
@@ -79,8 +85,10 @@ TEST(GrpcAuthorizationEngineTest, DenyEngineWithMatchingPolicy) {
 
 TEST(GrpcAuthorizationEngineTest, DenyEngineWithNoMatchingPolicy) {
   Rbac::Policy policy1(
-      Rbac::Permission(Rbac::Permission::RuleType::kAny, /*not_rule=*/true),
+      Rbac::Permission(Rbac::Permission::RuleType::kNot,
-      Rbac::Principal(Rbac::Principal::RuleType::kAny, /*not_rule=*/true));
+                       Rbac::Permission(Rbac::Permission::RuleType::kAny)),
+      Rbac::Principal(Rbac::Principal::RuleType::kNot,
+                      Rbac::Principal(Rbac::Principal::RuleType::kAny)));
   std::map<std::string, Rbac::Policy> policies;
   policies["policy1"] = std::move(policy1);
   Rbac rbac(Rbac::Action::kDeny, std::move(policies));