Enable ESP to invoke Firebase Security rules. by sarvaniv · Pull Request #54 · istio/proxy

sarvaniv · 2017-01-26T23:31:16Z

No description provided.

qiwzhang · 2017-01-27T00:04:03Z

You need to use clang-format to format following files

contrib/endpoints/src/api_manager/check_security_rules.cc contrib/endpoints/src/api_manager/check_security_rules.h contrib/endpoints/src/api_manager/context/request_context.h contrib/endpoints/src/api_manager/context/service_context.cc

qiwzhang · 2017-01-27T00:09:27Z

  std::string auth_authorized_party_;

+  // Auth Claims: This is the decoded payload of the JWT token
+  char *auth_claims_;


We can use std::string to store it. so you don't need to include grpc headers in order to call gpr_free

qiwzhang · 2017-01-27T00:11:18Z

  // See http://openid.net/specs/openid-connect-core-1_0.html#IDToken
  std::string authorized_party;
+  // String of claims
+  char *claims;


do we need this one?

Yes. We do. The UserInfo is where the decoded JWT claims are stored so it can be used in the later part of the work flow. This actually is the same flow as it is used for JWT audience and issuer fields.

Changed it to std::string

qiwzhang · 2017-01-27T00:13:35Z

  } else {
+    env_->LogInfo("Before Parse JWT");
    ParseJwt();
+    env_->LogInfo("After Parse JWT");


remove logging?

Looks like the merge put them back in. I will delete them before the next commit.

qiwzhang · 2017-01-27T00:16:20Z

-  void HttpFetch(const std::string &url, const std::string &request_body,
-                 std::function<void(Status, std::string &&)> continuation);
+  // Fetch the Release attributes.
+  auto pChecker = GetPtr();


Google c++ style guide, not to use camel case in variable name

Hmm. This code is again similar to check_auth.cc below. Let me know if you want me to fix it in both places.

void AuthChecker::DiscoverJwksUri(const std::string &url) {
auto pChecker = GetPtr();
HttpFetch(url, [pChecker](S

qiwzhang · 2017-01-27T00:20:23Z

-  // TODO: Check service config to see if "useSecurityRules" is specified.
-  // If so, call Firebase Rules service TestRuleset API.
+std::pair<Status, std::string> AuthzChecker::ParseReleaseResponse(
+    std::string *json_str) {


pass in const std::string& if you are not modifying json_str

qiwzhang · 2017-01-27T00:22:41Z

-void AuthzChecker::Check() {
-  // TODO: Check service config to see if "useSecurityRules" is specified.
-  // If so, call Firebase Rules service TestRuleset API.
+std::pair<Status, std::string> AuthzChecker::ParseReleaseResponse(


can we pass out ruleset_id in the argument. e.g.
Status Parse(..., string* ruleset_id)

qiwzhang · 2017-01-27T00:25:05Z

+      status = Status(Code::PERMISSION_DENIED,
+              std::string("Unauthorized ")
+              + context->request()->GetRequestHTTPMethod()
+              + " access to resource " + context->request()->GetRequestPath(),


Does response have detail info we can pass back to help users?

No. Unfortunatley, the Test API that we use only provided with a response that looks like this:

{
"testResults": [
{
"state": "SUCCESS"
}
]
}

or

{
"testResults": [
{
"state": "FAILURE"
}
]
}

The response itself is a HTTP 200 OK message.

qiwzhang · 2017-01-27T00:25:50Z

+std::string AuthzChecker::BuildTestRequestBody(
+    std::shared_ptr<context::RequestContext> context) {
+
+  std::shared_ptr<std::ostringstream> ss(new std::ostringstream);


not need to use new. just allocate it from stack.

qiwzhang · 2017-01-27T00:30:58Z

-  std::shared_ptr<AuthzChecker> authzChecker =
-      std::make_shared<AuthzChecker>(context, continuation);
-  authzChecker->Check();
+  std::shared_ptr<AuthzChecker> checker(new AuthzChecker(


why don't you use make_shared?

qiwzhang · 2017-01-27T00:31:54Z

+  std::string BuildTestRequestBody(
+      std::shared_ptr<context::RequestContext> context);
+
+  void AddToBody(const std::string &key,


can some of these function static.

if so, they can be in the cc file, inside anonymous name space

I moved the header file into the .cc file now.

liminw · 2017-01-27T00:20:36Z

+#include "grpc/support/string_util.h"
+#include "grpc/support/sync.h"
+}
+


Do we need all these headers? It looks like we are only using gpr_free() in the file.

Removed them. After changing to std::string based on Wayne's comments, I did not need them.

liminw · 2017-01-27T00:23:23Z

  if (cache_hit) {
    CheckAudience(true);
  } else {
+    env_->LogInfo("Before Parse JWT");


Remember remove these logs before submit.

liminw · 2017-01-27T00:23:37Z

  } else {
+    env_->LogInfo("Before Parse JWT");
    ParseJwt();
+    env_->LogInfo("After Parse JWT");


Remove these logs before submit.

liminw · 2017-01-27T00:26:39Z

  // See http://openid.net/specs/openid-connect-core-1_0.html#IDToken
  std::string authorized_party;
+  // String of claims
+  char *claims;


Can we make it std::string?

liminw · 2017-01-27T00:40:57Z

+
+  // Get the auth token for Firebase service
+  const std::string &GetAuthToken() {
+    return sa_token_->GetAuthToken(


Shall we check if sa_token_ is NULL?

Hmm. This is similar to The Aggregator code in cloud_trace.cc where the ServiceAccountToken is being passed in. There are no null checks there. Let me know if you think this is wrong and we can fix this at book places. However, since we don't throw exceptions, the null check should be done before passing in the value. Otherwise, we will have null checks all over the code.

liminw · 2017-01-27T00:46:24Z

+                 std::shared_ptr<std::ostringstream> ss);
+
+  void AddToBody(const std::string &key, const std::string &value,
+                 bool end, std::shared_ptr<std::ostringstream> ss);


Why do we need shared_ptr here?

liminw · 2017-01-27T00:46:32Z

+      std::shared_ptr<context::RequestContext> context);
+
+  void AddToBody(const std::string &key,
+                 std::shared_ptr<std::ostringstream> ss);


Why do we need shared_ptr here?

liminw · 2017-01-27T00:52:22Z

+
+  const std::string Stringify(const char *s) {
+    return std::string("\"") + s + "\"";
+  }


You are not handing the case that s is NULL pointer.

Converting a "char *" to string is simply:

std::string target = s == nullptr ? "" : s;

liminw · 2017-01-27T01:07:43Z

      .set_url(url)
-      .set_auth_token(GetAuthToken())
-      .set_header("Content-Type", "application/json")
+      .set_auth_token(token);


set_auth_token(GetAuthToken()). Then you don't need "token" variable.

liminw · 2017-01-27T01:17:07Z

-class AuthzChecker : public std::enable_shared_from_this<AuthzChecker> {
- public:
-  AuthzChecker(std::shared_ptr<context::RequestContext> context,
-               std::function<void(Status status)> continuation);


If AuthzChecker class is used only in check_seucurity_rules.cc/h, we prefer to put the class definition in .cc file. This way, any place that "include" check_security_rules.h will only include CheckSecurityRules() function definition (not AuthzChecker class).

qiwzhang · 2017-01-27T21:51:44Z

+  // Get Firebase specific operation Id based on the http Method.
+  std::string GetOperation(std::string httpMethod);
+
+  const std::string Stringify(const char *s) {


no need to be a class function.

This is removed with the new protobuf change.s

Removed this after proto change.

qiwzhang · 2017-01-27T21:52:40Z

+  }
+
+  // Fetch the Release attributes.
+  auto pchecker = GetPtr();


just "checker"

qiwzhang · 2017-01-27T21:55:10Z

+
+  // Fetch the Release attributes.
+  auto pchecker = GetPtr();
+  HttpFetch(GetReleaseUrl(context), std::string("GET"), std::string(""),


no need for std::string("GET"), just "GET", compiler will convert it

qiwzhang · 2017-01-27T21:57:48Z

+    std::function<void(Status status)> continuation) {
+  auto pchecker = GetPtr();
+
+  HttpFetch(std::string(kFirebaseServerStaging) + "v1/" + ruleset_id +


use a separate function for URL

qiwzhang · 2017-01-27T22:00:29Z

+
+  Status status = Status::OK;
+  const char *id = GetStringValue(json, "rulesetName");
+  (*ruleset_id) = (id == nullptr) ? "" : GetStringValue(json, "rulesetName");


last portion is id?

Not sure what you mean ...

*ruleset_id = (id) : id ? "";

qiwzhang · 2017-01-27T22:02:55Z

+  return status;
+}
+
+std::string AuthzChecker::GetOperation(std::string httpMethod) {


need to be a class member function?

Made it static function.

Made it static.

qiwzhang · 2017-01-27T22:05:21Z

+std::string AuthzChecker::BuildTestRequestBody(
+    std::shared_ptr<context::RequestContext> context) {
+
+  std::ostringstream ss;


maybe use a proto, build a proto message first, then convert it to json.

How I wish I could do it! The protos are not open sourced.

You can copy the proto file to our ESP code base. Still prefer to use proto to convert to JSON.

There are multiple proto files and this will come from the firebase codebase. I am not sure if we can just pick up the code and put it in our codebase. I will need to talk to Limin / Wencheng and may be even Firebase guys before doing this.

If it is too much trouble to copy their proto files, you just create your own simple proto message which only fields you used.

I see what you mean. I think I an do that. It is actually a better idea than dealing with strings.

qiwzhang · 2017-01-27T22:06:20Z

+}
+
+void AuthzChecker::AddToBody(const std::string &key,
+                       std::ostringstream &ss) {


no need to be a class member function

The reason I left it inside the class is because the usage of the function is only applicable to objects of this class.

it should be static if the function is not using any class members.

Ah. Sure. I will make them static.

Function is deleted after adding the new protobuf to capture this.

qiwzhang · 2017-01-27T22:08:53Z

+  }
+}
+
+const std::string AuthzChecker::GetReleaseName(


no need to be a class member function

Same reason as above. There is no meaning to this method outside the context of the class.

Changed this to static

liminw · 2017-01-27T20:04:16Z

-  // Pointer to access ESP running environment.
-  ApiManagerEnvInterface *env_;
+  // Get Firebase specific operation Id based on the http Method.
+  std::string GetOperation(std::string httpMethod);


const std::string &httpMethod

liminw · 2017-01-30T19:18:51Z

+#include "contrib/endpoints/include/api_manager/utils/status.h"
+
+#include <string>
+using ::google::api_manager::utils::Status;


No change is needed for this file. The additional "include" and "using" here should be removed.

liminw · 2017-01-30T19:19:49Z

    last_report_time_ = tp;
  }

+  void set_auth_claims(std::string &claims) {


const std::string &

liminw · 2017-01-30T19:25:35Z

+    return auth_claims_;
+  }
+
+  const std::string GetAuthToken();


Why do you need GetAuthToken() here?

liminw · 2017-01-30T19:31:10Z

 namespace api_manager {

-namespace {
-


The whole class should be an anonymous namespace. You should not remove it.

liminw · 2017-01-30T19:31:45Z

    "https://staging-firebaserules.sandbox.googleapis.com/";

-// An AuthzChecker object is created for every incoming request. It does
-// authorizaiton by calling Firebase Rules service.


This comment applies to the class. Why do you remove it?

Added it back. Initially I had code that was creating the object in the service context but this did not work due to circular dependences. I removed it as a part of that change and forgot to add it back.

liminw · 2017-01-30T19:36:00Z

-  void HttpFetch(const std::string &url, const std::string &request_body,
+
+  // Helper method that invokes the test firebase service api.
+  void FirebaseCheck(std::string ruleset_id,


const std::string &ruleset_id

liminw · 2017-01-30T19:37:52Z

    last_report_time_ = tp;
  }

+  void set_auth_claims(std::string &claims) {


const std::string &claims

liminw · 2017-01-30T19:38:45Z

+    auth_claims_ = claims;
+  }
+
+  std::string &auth_claims() {


I think this function can simply return std::string.

Why? We just use this string to create a proto object and this string can be potentially big.

qiwzhang · 2017-02-01T17:49:02Z

+
+  /*for (grpc_json *cur = json->child; cur != nullptr; cur = cur->next) {
+    user_info->claims.emplace(std::string(cur->key), std::string(cur->value));
+  } */


what is this commented out code for? If no needed, remove it

qiwzhang · 2017-02-01T17:56:43Z

+message ApiCheckSecurityRulesConfig {
+  // Allows to disable the API authorization
+  bool force_disable = 1;
+}


you may want to have firebase_server here so "t" test can provide a fake server.

BTW, How do we enable this feature, is it enabled by service_config? Here we force disable it? If so, we are fine

This can be enabled via service config. But the service config changes are not done yet. Tao is working on those changes.

If that is the case, it will be easier for you to integrate with ESP if you default to be disabled, only enable it from server_config.

I don't want to disable this by default via server config and there is a reason for this. By not disabling the feature by default I am able to apply some existing auth based t tests. For example, we have checks to ensure that if auth is disabled, then we don't do any firebase rules checks too. For all those tests, since the code is not disabled by default, we are able to exercise the existing t tests without any more tests that do the same thing.

qiwzhang · 2017-02-01T17:58:00Z

                                  ->api_authentication_config()
-                                  .force_disable()) {
+                                  .force_disable()),
+      is_check_security_rules_disabled_(config_->server_config() &&


is it enabled by default? do we need to check service_config ?

Currently enabled by default. Once the service config changes are done, we will add those checks too.

it is better to be disabled by default for now. and only enable it when server_config has api_check_securty_rules_config and its firebase_server is specified.

we can change it when service config is ready.

qiwzhang · 2017-02-01T18:01:49Z

+
 // An AuthzChecker object is created for every incoming request. It does
 // authorizaiton by calling Firebase Rules service.
 class AuthzChecker : public std::enable_shared_from_this<AuthzChecker> {


Since the file name is called check_security_rules, should we call the class as SecurityRuleChecker

This was named by Limin and so was the file name provided by her. Let me see what she thinks too :-)

qiwzhang · 2017-02-01T18:03:02Z

-  void HttpFetch(const std::string &url, const std::string &request_body,
+  // Helper method that invokes the test firebase service api.
+  void FirebaseCheck(std::string &ruleset_id,
+                     std::shared_ptr<context::RequestContext> context,


how about "CallTest"?

qiwzhang · 2017-02-01T18:10:03Z

+void AuthzChecker::FirebaseCheck(
+    std::string &ruleset_id, std::shared_ptr<context::RequestContext> context,
+    std::function<void(Status status)> continuation) {
+  auto checker = GetPtr();


move this line to 163

qiwzhang · 2017-02-01T18:12:39Z

+    std::shared_ptr<context::RequestContext> context,
+    std::string &result_string) {
+  proto::TestRulesetRequest request;
+  proto::TestRulesetRequest::TestCase *test_case = request.add_test_cases();


you can use "auto"

Yup. Thank you.

qiwzhang · 2017-02-01T18:15:29Z

+
+Status AuthzChecker::BuildTestRequestBody(
+    std::shared_ptr<context::RequestContext> context,
+    std::string &result_string) {


Per style guide, you should use std::string*

qiwzhang · 2017-02-01T18:17:08Z

+
+void AuthzChecker::SetProtoValue(::google::protobuf::Value &head,
+                                 std::string key,
+                                 ::google::protobuf::Value &value) {


per style guide, the prototype should be
(const string& key, const Value& value, Value* head)

qiwzhang · 2017-02-01T18:20:46Z


-  // Pointer to access ESP running environment.
-  ApiManagerEnvInterface *env_;
+  void SetProtoValue(::google::protobuf::Value &head, std::string key,


this can be static.

I think all these 4 static functions can be out of the class.

lizan · 2017-02-01T18:43:17Z

+  if (!context->service_context()->RequireRulesCheck() ||
+      context->method() == nullptr || !context->method()->auth()) {
+    env_->LogDebug(
+        std::string("Autherization and JWT validation was not performed") +


nit: Authorization, no need of std::string, just use literals without +

lizan · 2017-02-01T18:45:32Z

+                              std::string *ruleset_id);
+
+  // Parses the response for the TEST API call
+  Status ParseTestResponse(std::shared_ptr<context::RequestContext> context,


output parameter should be pointers, per our C++ style guide

Yes. Wayne gave me this comment and I have already fixed this. in a later commit.

lizan · 2017-02-01T18:45:36Z

+                           const std::string &json_str);
+
+  // Builds the request body for the TESP API call.
+  Status BuildTestRequestBody(std::shared_ptr<context::RequestContext> context,


same. already fixed in later commit.

lizan · 2017-02-01T18:49:05Z

+    if (result == nullptr) {
+      env_->LogInfo("Result state is empty");
+      status = invalid;
+    } else if (std::string(result) != "SUCCESS") {


"SUCCESS" as constant, and make the constant as std::string.

I have declared a const char[] kTestSuccess with the rest of the cont strings. Let me know if this is not what you had in mind.

lizan · 2017-02-01T18:50:23Z


+  void set_auth_claims(const std::string &claims) { auth_claims_ = claims; }
+
+  std::string &auth_claims() { return auth_claims_; }


liminw · 2017-02-01T19:06:47Z

 #ifndef API_MANAGER_AUTH_H_
 #define API_MANAGER_AUTH_H_

+#include <map>


Why do you include ?

liminw · 2017-02-01T19:42:06Z

-  // Helper function to send a http GET request.
-  void HttpFetch(const std::string &url, const std::string &request_body,
+  // Helper method that invokes the test firebase service api.
+  void CallTest(std::string &ruleset_id,


const std::string &ruleset_id;

liminw · 2017-02-01T19:52:50Z

 #ifndef API_MANAGER_CHECK_SECURITY_RULES_H_
 #define API_MANAGER_CHECK_SECURITY_RULES_H_

+#include <string>


remove "include".

liminw · 2017-02-01T19:57:07Z

+// Server config for API Authorization via Firebase Rules
+message ApiCheckSecurityRulesConfig {
+  // Firebase server to use.
+  string firebase_server = 1;


When do we use this configuration?

We will use this configuration with t tests. I will update this in another change once I understand t tests better. For now this is only a place holder.

lizan · 2017-02-01T20:39:16Z

  // Optional field.
  const grpc_json *grpc_json = grpc_jwt_claims_json(claims_);
+
+  char *json_str =


json_str is leaked

Thank you. Missed this while addressing review comments. I was freeing this in the first commit :-)

qiwzhang · 2017-02-01T21:42:30Z

+const char kInvalidResponse[] = "Invalid JSON response from Firebase Service";
+const char kTestSuccess[] = "SUCCESS";
+
+void SetProtoValue(std::string key, ::google::protobuf::Value &value,


const std::string&
const Value&

qiwzhang · 2017-02-01T21:45:03Z

+}
+
+const std::string GetReleaseName(
+    std::shared_ptr<context::RequestContext> request_context) {


Not to use shared_ptr if you are not keep a ref_count.
use
const context::RequestContext&

The CheckSecurityRules method is the entry point and it receives a share_ptr. Do you mean that I should extract the pointer from the shared pointer? I thought it is considered bad to extract a pointer out of a smart pointer. Lets discuss more.

Pass a shared_ptr is bad for performance. it needs to lock in order to increase ref_count.

If your function don't need to keep a ref_count, pass in the object.

qiwzhang · 2017-02-01T21:45:23Z

+}
+
+const std::string GetReleaseUrl(
+    std::shared_ptr<context::RequestContext> request_context) {


not to use shared_prt

qiwzhang · 2017-02-01T21:48:33Z

+  if (!context->service_context()->RequireRulesCheck() ||
+      context->method() == nullptr || !context->method()->auth()) {
+    env_->LogDebug(
+        "Autherization and JWT validation was not performed"


is logging message correct?

should it be firebase ruleset validation?

qiwzhang · 2017-02-01T21:51:11Z

+  Status status = Status::OK;
+  const char *id = GetStringValue(json, "rulesetName");
+  (*ruleset_id) = (id == nullptr) ? "" : id;
+


why do we need () in (*ruleset_id)?

qiwzhang · 2017-02-01T21:52:52Z

+  SetProtoValue("token", claims, &token);
+  SetProtoValue("auth", token, &auth);
+
+  Map<std::string, google::protobuf::Value> *variables =


qiwzhang · 2017-02-01T21:53:17Z

+    const std::string &request_body,
    std::function<void(Status, std::string &&)> continuation) {
+  env_->LogInfo(std::string("Issue HTTP Request to url :") + url +
+                " method : " + method + " body: " + request_body);


qiwzhang · 2017-02-01T21:57:07Z

                                  ->api_authentication_config()
-                                  .force_disable()) {
+                                  .force_disable()),
+      is_check_security_rules_disabled_(config_->server_config() &&


it is better to be disabled by default for now. and only enable it when server_config has api_check_securty_rules_config and its firebase_server is specified.

we can change it when service config is ready.

qiwzhang · 2017-02-01T23:49:57Z

-    std::shared_ptr<context::RequestContext> request_context) {
-  return request_context->service_context()->service_name() + ":" +
-         request_context->service_context()->service().apis(0).version();
+const std::string GetReleaseName(context::RequestContext &context) {


do we need const here? same as line 65

and line 70

I already tried that and I get the following compilation error. I think this is because the service_context() method is not marked as const. Let me know if you are OK with marking that method as const - I think it can be made into a const but I don't know why it was not marked const.

contrib/endpoints/src/api_manager/check_security_rules.cc: In function 'const string google::api_manager::{anonymous}::GetReleaseName(const google::api_manager::context::RequestContext&)':
contrib/endpoints/src/api_manager/check_security_rules.cc:61:34: error: passing 'const google::api_manager::context::RequestContext' as 'this' argument of 'google::api_manager::context::ServiceContext* google::api_manager::context::RequestContext::service_context()' discards qualifiers [-fpermissive]
return context.service_context()->service_name() + ":" +
^
contrib/endpoints/src/api_manager/check_security_rules.cc:62:34: error: passing 'const google::api_manager::context::RequestContext' as 'this' argument of 'google::api_manager::context::ServiceContext* google::api_manager::context::RequestContext::service_context()' discards qualifiers [-fpermissive]
context.service_context()->service().apis(0).version();

qiwzhang · 2017-02-01T23:51:51Z

  auto checker = GetPtr();
-  HttpFetch(GetReleaseUrl(context), "GET", "",
+  HttpFetch(GetReleaseUrl(*context.get()), "GET", "",
            [context, final_continuation, checker](Status status,


could we just do "*context" to convert shared_ptr to const T&?

qiwzhang · 2017-02-01T23:55:59Z

+    return RequireAuth() && config_->server_config() &&
+           !config_->server_config()
+                ->api_check_security_rules_config()
+                .firebase_server()


you may need to call has_ first before you access its members.

Added it. For some reason this is not being done in other parts of the code. I am adding this check in one place I know.

lizan · 2017-02-02T01:17:17Z

+  char *json_str =
+      grpc_json_dump_to_string(const_cast<::grpc_json *>(grpc_json), 0);
+  user_info->claims = json_str == nullptr ? "" : json_str;
+  gpr_free(json_str);


you probably want to check if json_str is nullptr

lizan · 2017-02-02T01:40:05Z

 const char kFirebaseServerStaging[] =
    "https://staging-firebaserules.sandbox.googleapis.com/";

+const char kFirebaseService[] =


where are these two constants used?

I will delete them once i have done more testing.

lizan · 2017-02-02T01:41:12Z

+
+std::string GetOperation(std::string &httpMethod) {
+  if (httpMethod == "POST") {
+    return std::string("create");


make these constant

Done. Some parts of the code were using them as I have done. I did not know this was a strict rule.

lizan · 2017-02-02T01:49:24Z

+  request->set_method(method).set_url(url).set_auth_token(GetAuthToken());
+
+  if (method != "GET") {
+    request->set_header("Content-Type", "application/json")


make these constant too

I have removed as many string constants from code as I could. Let me know if you think there are more that need to be changed.

qiwzhang

Just add a simple config_test for firebase_server for now.
In a separate PR, add check_security_rules_test.cc, similar to auth_check_test.cc

qiwzhang · 2017-02-02T18:40:47Z

+
+const std::string GetFirebaseServer(const context::RequestContext &context) {
+  return context.service_context()
+      ->config()


make a GetFireBaseServer() function in Config class. Later when service_config is ready, get it from there. Only need to change that function.

qiwzhang · 2017-02-02T18:41:35Z

+const char kContentType[] = "Content-Type";
+const char kApplication[] = "application/json";
+
+const std::string GetFirebaseServer(const context::RequestContext &context) {


I still don't understand the purpose of "const" in

const std::string foo()

what is this const for?

qiwzhang · 2017-02-02T18:43:01Z

+
+const std::string GetReleaseName(const context::RequestContext &context) {
+  return context.service_context()->service_name() + ":" +
+         context.service_context()->service().apis(0).version();


when enabling firebase, need to make sure apis.size() == 1.

lizan · 2017-02-02T20:46:18Z

+      grpc_json_dump_to_string(const_cast<::grpc_json *>(grpc_json), 0);
+  user_info->claims = json_str == nullptr ? "" : json_str;
+  if (json_str != nullptr) {
+    gpr_free(json_str);


move the line above in here? member std::string is default to "", so you just need to assign once.

qiwzhang · 2017-02-02T21:35:49Z

  bool IsRulesCheckEnabled() const {
-    return RequireAuth() && config_->server_config() &&
+    return RequireAuth() && service().apis_size() > 0 &&
+           config_->server_config() &&


can we call config->GEtFirebaseServer()?

qiwzhang · 2017-02-02T21:36:34Z

+
+  auto server = config->GetFirebaseServer();
+  ASSERT_EQ(server, "https://myfirebaseserver.com/");
+}


could you add a test case with a non-empty server_config, but no api_check_security_rules_config.

Enable ESP to invoke Firebase Security rules.

348f061

sarvaniv requested review from liminw, qiwzhang and wattli January 26, 2017 23:31

qiwzhang reviewed Jan 27, 2017

View reviewed changes

liminw reviewed Jan 27, 2017

View reviewed changes

sarvaniv added 3 commits January 27, 2017 11:42

Address code review comments.

6339697

Address code review comments

13caf68

Remove some debug logs

b0e48c7

qiwzhang reviewed Jan 27, 2017

View reviewed changes

liminw reviewed Jan 30, 2017

View reviewed changes

sarvaniv added 5 commits January 31, 2017 14:32

Add proto file to capture TestRulesetRequest.

6f10a20

clang-format files

c6ae0fa

Resolve a merge issue with previous commit

01b50bf

Allow security rules to disabled via serverconfig

1a6704c

format file

6351cfa

qiwzhang reviewed Feb 1, 2017

View reviewed changes

lizan reviewed Feb 1, 2017

View reviewed changes

sarvaniv added 3 commits February 1, 2017 10:56

Addressed Wayne's review comments.

32d0023

Add firebase server to Server Config.

75d6e7a

Address Lizan's review comments

b573770

liminw reviewed Feb 1, 2017

View reviewed changes

Address review comments.

9887846

liminw approved these changes Feb 1, 2017

View reviewed changes

lizan suggested changes Feb 1, 2017

View reviewed changes

qiwzhang reviewed Feb 1, 2017

View reviewed changes

Disable check rules service by default.

7a2abe7

qiwzhang reviewed Feb 2, 2017

View reviewed changes

lizan reviewed Feb 2, 2017

View reviewed changes

sarvaniv added 3 commits February 2, 2017 09:32

Address more review comments.

b871e40

Fix a check.

71c0d92

Delete unwanted constant.

50955e3

qiwzhang reviewed Feb 2, 2017

View reviewed changes

Address Wayne's comments and add a simple config test.

35a61df

lizan reviewed Feb 2, 2017

View reviewed changes

Address a review comment.

4ef505a

qiwzhang reviewed Feb 2, 2017

View reviewed changes

sarvaniv added 3 commits February 2, 2017 13:44

Add negative test case for config

9dddcac

Address code review

c35348d

Remove unwanted const std::string

4ccc606

qiwzhang approved these changes Feb 2, 2017

View reviewed changes

sarvaniv merged commit df8c69c into istio:firebase Feb 2, 2017


		void set_auth_claims(const std::string &claims) { auth_claims_ = claims; }

		std::string &auth_claims() { return auth_claims_; }

Uh oh!

Conversation

sarvaniv commented Jan 26, 2017

Uh oh!

qiwzhang commented Jan 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!