Update the auth check to use service.experimental.authorization.provi…

…der (#67) * Update the auth check to use service.experimental.authorization.provider * Address comments and revert accidental change. * Remove unnecessary added accidentally. * Another patch * fix the logic * fix lint * Fix broken test and add unit tests * Fix comments * Fix style check * revert style for raw string * fix small lint * fix small lint * fix small lint
istio · Feb 8, 2017 · 0c001df · 0c001df
1 parent dee3881
commit 0c001df
Show file tree

Hide file tree

Showing 10 changed files with 100 additions and 456 deletions.
diff --git a/.idea/misc.xml b/.idea/misc.xml
diff --git a/.idea/modules.xml b/.idea/modules.xml
diff --git a/.idea/proxy.iml b/.idea/proxy.iml
diff --git a/.idea/vcs.xml b/.idea/vcs.xml
diff --git a/.idea/workspace.xml b/.idea/workspace.xml
diff --git a/contrib/endpoints/repositories.bzl b/contrib/endpoints/repositories.bzl
@@ -254,6 +254,8 @@ cc_proto_library(
         "google/api/control.proto",
         "google/api/documentation.proto",
         "google/api/endpoint.proto",
+        "google/api/experimental/authorization_config.proto",
+        "google/api/experimental/experimental.proto",
         "google/api/http.proto",
         "google/api/label.proto",
         "google/api/log.proto",
@@ -293,7 +295,7 @@ cc_proto_library(
 
     native.new_git_repository(
         name = "googleapis_git",
-        commit = "db1d4547dc56a798915e0eb2c795585385922165",
+        commit = "412867fb105722fb9d2cd9af90af1f8f120de238",
         remote = "https://github.com/googleapis/googleapis.git",
         build_file_content = BUILD,
     )

diff --git a/contrib/endpoints/src/api_manager/config.cc b/contrib/endpoints/src/api_manager/config.cc
@@ -514,11 +514,21 @@ void Config::SetJwksUri(const string &issuer, const string &jwks_uri,
 }
 
 std::string Config::GetFirebaseServer() {
-  if (server_config_ == nullptr) {
-    return "";
+  // Server config overwrites service config.
+  if (server_config_ != nullptr &&
+      server_config_->has_api_check_security_rules_config() &&
+      !server_config_->api_check_security_rules_config()
+           .firebase_server()
+           .empty()) {
+    return server_config_->api_check_security_rules_config().firebase_server();
   }
 
-  return server_config_->api_check_security_rules_config().firebase_server();
+  if (service_.has_experimental() &&
+      service_.experimental().has_authorization() &&
+      !service_.experimental().authorization().provider().empty()) {
+    return service_.experimental().authorization().provider();
+  }
+  return "";
 }
 
 }  // namespace api_manager

diff --git a/contrib/endpoints/src/api_manager/config.h b/contrib/endpoints/src/api_manager/config.h
@@ -64,9 +64,7 @@ class Config {
   // TODO: Remove in favor of service().
   const std::string &service_name() const { return service_.name(); }
 
-  bool HasAuth() const { return service_.has_experimental() &&
-      service_.experimental().has_authorization() &&
-      service_.experimental().authorization().has_provider(); }
+  bool HasAuth() const { return service_.has_authentication(); }
 
   // Returns true if the caller should try openId discovery to fetch jwksUri.
   // url is set to the openId discovery link in this case. Returns false

diff --git a/contrib/endpoints/src/api_manager/config_test.cc b/contrib/endpoints/src/api_manager/config_test.cc
@@ -501,35 +501,34 @@ TEST(Config, LoadBackends) {
 TEST(Config, RpcMethodsWithHttpRules) {
   MockApiManagerEnvironmentWithLog env;
 
-  const char config_text[] =
-      R"(
-      name : "BookstoreApi"
-      apis {
-        name: "Bookstore"
-        methods {
-          name: "ListShelves"
-          request_type_url: "types.googleapis.com/google.protobuf.Empty"
-          response_type_url: "types.googleapis.com/Bookstore.ListShelvesResponse"
-        }
-        methods {
-          name: "CreateShelves"
-          request_streaming: true
-          request_type_url: "types.googleapis.com/Bookstore.Shelf"
-          response_streaming: true
-          response_type_url: "types.googleapis.com/Bookstore.Shelf"
-        }
+  const char config_text[] = R"(
+    name : "BookstoreApi"
+    apis {
+      name: "Bookstore"
+      methods {
+        name: "ListShelves"
+        request_type_url: "types.googleapis.com/google.protobuf.Empty"
+        response_type_url: "types.googleapis.com/Bookstore.ListShelvesResponse"
       }
-      http {
-        rules {
-          selector: "Bookstore.ListShelves"
-          get: "/shelves"
-        }
-        rules {
-          selector: "Bookstore.CreateShelves"
-          post: "/shelves"
-        }
+      methods {
+        name: "CreateShelves"
+        request_streaming: true
+        request_type_url: "types.googleapis.com/Bookstore.Shelf"
+        response_streaming: true
+        response_type_url: "types.googleapis.com/Bookstore.Shelf"
       }
-    )";
+    }
+    http {
+      rules {
+        selector: "Bookstore.ListShelves"
+        get: "/shelves"
+      }
+      rules {
+        selector: "Bookstore.CreateShelves"
+        post: "/shelves"
+      }
+    }
+  )";
 
   std::unique_ptr<Config> config = Config::Create(&env, config_text, "");
   ASSERT_TRUE(config);
@@ -764,8 +763,8 @@ TEST(Config, TestHttpOptions) {
    rules {
      selector: "CorsShelves"
      custom: {
-        kind: "OPTIONS"
-        path: "/shelves"
+       kind: "OPTIONS"
+       path: "/shelves"
      }
    }
    rules {
@@ -870,44 +869,77 @@ TEST(Config, TestCorsDisabled) {
   ASSERT_EQ(nullptr, method1);
 }
 
-TEST(Config, TestFirebaseServerCheck) {
+static const char kServiceConfigWithoutAuthz[] = R"(
+  name: "Service.Name"
+)";
+
+static const char kServiceConfigWithAuthz[] = R"(
+  name: "Service.Name"
+  experimental {
+    authorization {
+      provider: "authz@firebase.com"
+    }
+  }
+)";
+
+static const char kServerConfigWithoutAuthz[] = R"(
+  service_control_config {
+    check_aggregator_config {
+      cache_entries: 1000
+      flush_interval_ms: 10
+      response_expiration_ms: 20
+    }
+    report_aggregator_config {
+      cache_entries: 1020
+      flush_interval_ms: 15
+    }
+  }
+)";
+
+static const char kServerConfigWithAuthz[] = R"(
+  api_check_security_rules_config {
+    firebase_server: "https://myfirebaseserver.com/"
+  }
+)";
+
+TEST(Config, TestFirebaseServerCheckWithServiceAuthzWithoutServerAuthz) {
   MockApiManagerEnvironmentWithLog env;
 
-  static const char server_config[] = R"(
-api_check_security_rules_config {
-  firebase_server: "https://myfirebaseserver.com/"
+  std::unique_ptr<Config> config =
+      Config::Create(&env, kServiceConfigWithAuthz, kServerConfigWithoutAuthz);
+  ASSERT_TRUE(config);
+
+  ASSERT_EQ(config->GetFirebaseServer(), "authz@firebase.com");
 }
-)";
+
+TEST(Config, TestFirebaseServerCheckWithServiceAuthzWithServerAuthz) {
+  MockApiManagerEnvironmentWithLog env;
 
   std::unique_ptr<Config> config =
-      Config::Create(&env, kServiceNameConfig, server_config);
+      Config::Create(&env, kServiceConfigWithAuthz, kServerConfigWithAuthz);
   ASSERT_TRUE(config);
 
   ASSERT_EQ(config->GetFirebaseServer(), "https://myfirebaseserver.com/");
 }
 
-TEST(Config, TestEmptyFirebaseServerCheck) {
+TEST(Config, TestFirebaseServerCheckWithoutServiceAuthzWithoutServerAuthz) {
   MockApiManagerEnvironmentWithLog env;
 
-  static const char server_config[] = R"(
-service_control_config {
-  check_aggregator_config {
-    cache_entries: 1000
-    flush_interval_ms: 10
-    response_expiration_ms: 20
-  }
-  report_aggregator_config {
-    cache_entries: 1020
-    flush_interval_ms: 15
-  }
+  std::unique_ptr<Config> config = Config::Create(
+      &env, kServiceConfigWithoutAuthz, kServerConfigWithoutAuthz);
+  ASSERT_TRUE(config);
+
+  ASSERT_EQ(config->GetFirebaseServer(), "");
 }
-)";
+
+TEST(Config, TestFirebaseServerCheckWithoutServiceConfigWithServerConfig) {
+  MockApiManagerEnvironmentWithLog env;
 
   std::unique_ptr<Config> config =
-      Config::Create(&env, kServiceNameConfig, server_config);
+      Config::Create(&env, kServiceConfigWithoutAuthz, kServerConfigWithAuthz);
   ASSERT_TRUE(config);
 
-  ASSERT_TRUE(config->GetFirebaseServer().empty());
+  ASSERT_EQ(config->GetFirebaseServer(), "https://myfirebaseserver.com/");
 }
 }  // namespace
 

diff --git a/contrib/endpoints/src/api_manager/context/service_context.h b/contrib/endpoints/src/api_manager/context/service_context.h
@@ -67,10 +67,7 @@ class ServiceContext {
 
   bool IsRulesCheckEnabled() const {
     return RequireAuth() && service().apis_size() > 0 &&
-        (!config_->GetFirebaseServer().empty() ||
-            (service().has_experimental() &&
-                service().experimental().has_authorization() &&
-                service().experimental().authorization().has_provider()));
+           !config_->GetFirebaseServer().empty();
   }
 
   auth::Certs &certs() { return certs_; }