Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow for HTTP based function from Firebase rules #202

Merged
merged 8 commits into from
Mar 29, 2017
Merged

Allow for HTTP based function from Firebase rules #202

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b8c48bc
Allow for HTTP based function from Firebase rules
sarvaniv Mar 24, 2017
a2e6768
Fix code style check
sarvaniv Mar 24, 2017
d33f083
Added more comments.
sarvaniv Mar 24, 2017
a5d8603
Fix style issues.
sarvaniv Mar 24, 2017
8a3eddf
Address code review comments from Limin and Lizan.
sarvaniv Mar 24, 2017
a509913
Add more comments and address CR comments.
sarvaniv Mar 27, 2017
54fe47d
Fix a typo.
sarvaniv Mar 28, 2017
93e0936
Address Wayne's CR comments.
sarvaniv Mar 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions contrib/endpoints/src/api_manager/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ cc_library(
"//contrib/endpoints/src/api_manager/context",
"//contrib/endpoints/src/api_manager/service_control",
"//contrib/endpoints/src/api_manager/utils",
"//contrib/endpoints/src/api_manager/firebase_rules",
"//external:cc_wkt_protos",
"//external:cloud_trace",
"//external:googletest_prod",
Expand Down Expand Up @@ -291,6 +292,8 @@ cc_test(
deps = [
":api_manager",
":mock_api_manager_environment",
":security_rules_proto",
"//external:cc_wkt_protos",
"//external:googletest_main",
],
)
3 changes: 3 additions & 0 deletions contrib/endpoints/src/api_manager/auth/service_account_token.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ class ServiceAccountToken {
JWT_TOKEN_FOR_SERVICE_CONTROL = 0,
JWT_TOKEN_FOR_CLOUD_TRACING,
JWT_TOKEN_FOR_FIREBASE,

// JWT token for accessing the http endpoints defined in Firebase Rules.
JWT_TOKEN_FOR_AUTHORIZATION_SERVICE,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment for this one, like "JWT token for accessing the http endpoints that user defines in Firebase Rules".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

JWT_TOKEN_TYPE_MAX,
};
// Set audience. Only calcualtes JWT token with specified audience.
Expand Down
190 changes: 31 additions & 159 deletions contrib/endpoints/src/api_manager/check_security_rules.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@
#include <iostream>
#include <sstream>
#include "contrib/endpoints/src/api_manager/auth/lib/json_util.h"
#include "contrib/endpoints/src/api_manager/proto/security_rules.pb.h"
#include "contrib/endpoints/src/api_manager/firebase_rules/firebase_request.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does the new file have to be on its own folder? Can it be part of auth folder? Or should we create a new folder "authz" for all future authorization files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially started off with authz but then decided to firebase_rules since this is very explicit as to what is being used.

  1. I don't think auth is the right place for this since authentication and authorization are two different things.
  2. Here are few ways things can go:
    a) If we continue using firebase rules for authorization, then this is directory is still good.
    b) If we decide to support another form of authz along with firebase rules, then I think we can create a directory called authz and move the firebase_rules under that directory and add another directory that appropriately names the authorization mechanism we will support.
    c) We decide to discontinue using firebase rules, then we delete this directory.

I think keeping this as firebase_rules makes a lot of sense since that is what we are doing.

#include "contrib/endpoints/src/api_manager/utils/marshalling.h"

using ::google::api_manager::auth::GetProperty;
using ::google::api_manager::auth::GetStringValue;
using ::google::api_manager::firebase_rules::FirebaseRequest;
using ::google::api_manager::utils::Status;
using ::google::protobuf::Map;
using ::google::protobuf::util::error::Code;

namespace google {
namespace api_manager {
Expand All @@ -33,71 +31,26 @@ namespace {
const char kFailedFirebaseReleaseFetch[] = "Failed to fetch Firebase Release";
const char kFailedFirebaseTest[] = "Failed to execute Firebase Test";
const char kInvalidResponse[] = "Invalid JSON response from Firebase Service";
const char kTestSuccess[] = "SUCCESS";
const char kHttpGetMethod[] = "GET";
const char kHttpPostMethod[] = "POST";
const char kHttpHeadMethod[] = "HEAD";
const char kHttpOptionsMethod[] = "OPTIONS";
const char kHttpDeleteMethod[] = "DELETE";
const char kFirebaseCreateMethod[] = "create";
const char kFirebaseGetMethod[] = "get";
const char kFirebaseDeleteMethod[] = "delete";
const char kFirebaseUpdateMethod[] = "update";
const char kV1[] = "/v1";
const char kTestQuery[] = ":test?alt=json";
const char kHttpGetMethod[] = "GET";
const char kProjects[] = "/projects";
const char kReleases[] = "/releases";
const char kRulesetName[] = "rulesetName";
const char kTestResults[] = "testResults";
const char kState[] = "state";
const char kToken[] = "token";
const char kAuth[] = "auth";
const char kRequest[] = "request";
const char kContentType[] = "Content-Type";
const char kApplication[] = "application/json";

void SetProtoValue(const std::string &key,
const ::google::protobuf::Value &value,
::google::protobuf::Value *head) {
::google::protobuf::Struct *s = head->mutable_struct_value();
Map<std::string, google::protobuf::Value> *fields = s->mutable_fields();
(*fields)[key] = value;
}

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

std::string GetRulesetTestUri(const context::RequestContext &context,
const std::string &ruleset_id) {
return context.service_context()->config()->GetFirebaseServer() + kV1 + "/" +
ruleset_id + kTestQuery;
}

std::string GetReleaseUrl(const context::RequestContext &context) {
return context.service_context()->config()->GetFirebaseServer() + kV1 +
kProjects + "/" + context.service_context()->project_id() + kReleases +
"/" + GetReleaseName(context);
}

std::string GetOperation(const std::string &httpMethod) {
if (httpMethod == kHttpPostMethod) {
return kFirebaseCreateMethod;
}

if (httpMethod == kHttpGetMethod || httpMethod == kHttpHeadMethod ||
httpMethod == kHttpOptionsMethod) {
return kFirebaseGetMethod;
}

if (httpMethod == kHttpDeleteMethod) {
return kFirebaseDeleteMethod;
}

return kFirebaseUpdateMethod;
}

// 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> {
Expand All @@ -111,38 +64,31 @@ class AuthzChecker : public std::enable_shared_from_this<AuthzChecker> {
std::function<void(Status status)> continuation);

private:
// Helper method that invokes the test firebase service api.
void CallTest(const std::string &ruleset_id,
std::shared_ptr<context::RequestContext> context,
std::function<void(Status status)> continuation);
// This method invokes the Firebase TestRuleset API endpoint as well as user
// defined endpoints provided by the TestRulesetResponse.
void CallNextRequest(std::function<void(Status status)> continuation);

// Parse the respose for GET RELEASE API call
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: respose -> response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Status ParseReleaseResponse(const std::string &json_str,
std::string *ruleset_id);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use std::string & instead of std::string *.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the Google syle guide link that says output variables should be pointers. The ParseReleaseResponse will set the ruleset_id and making it a pointer is a way of saying that it will be modified. It should also be the last parameter.

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#Function_Parameter_Ordering


// Parses the response for the TEST API call
Status ParseTestResponse(context::RequestContext &context,
const std::string &json_str);

// Builds the request body for the TESP API call.
Status BuildTestRequestBody(context::RequestContext &context,
std::string *result_string);

// Invoke the HTTP call
void HttpFetch(const std::string &url, const std::string &method,
const std::string &request_body,
auth::ServiceAccountToken::JWT_TOKEN_TYPE token_type,
std::function<void(Status, std::string &&)> continuation);

// Get the auth token for Firebase service
const std::string &GetAuthToken() {
return sa_token_->GetAuthToken(
auth::ServiceAccountToken::JWT_TOKEN_FOR_FIREBASE);
const std::string &GetAuthToken(
auth::ServiceAccountToken::JWT_TOKEN_TYPE token_type) {
return sa_token_->GetAuthToken(token_type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this function necessary? it did not do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.

}

std::shared_ptr<AuthzChecker> GetPtr() { return shared_from_this(); }

ApiManagerEnvInterface *env_;
auth::ServiceAccountToken *sa_token_;
std::unique_ptr<FirebaseRequest> request_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this object handle single request? or it is a state machine, handling a complex logic. If the latter, should it be call FirebaseHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

};

AuthzChecker::AuthzChecker(ApiManagerEnvInterface *env,
Expand All @@ -165,6 +111,7 @@ void AuthzChecker::Check(
// Fetch the Release attributes.
auto checker = GetPtr();
HttpFetch(GetReleaseUrl(*context), kHttpGetMethod, "",
auth::ServiceAccountToken::JWT_TOKEN_FOR_FIREBASE,
[context, final_continuation, checker](Status status,
std::string &&body) {
std::string ruleset_id;
Expand All @@ -182,39 +129,38 @@ void AuthzChecker::Check(
// If the parsing of the release body is successful, then call the
// Test Api for firebase rules service.
if (status.ok()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if-else clause can be combined with the if-else clause above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is another way but here is how that code will look.

if (status.ok()) {
checker->env_->LogDebug(
std::string("GetReleasName succeeded with ") + body);
status = checker->ParseReleaseResponse(body, &ruleset_id);
if (status.ok()) {
checker->request_handler_ = std::unique_ptr(
new FirebaseRequest(ruleset_id, checker->env_, context));
checker->CallNextRequest(final_continuation);
} else { // I cannot remove this else statement since this will not go into the else part below
final_continuation(status)
}
} else {
checker->env_->LogError(std::string("GetReleaseName for ") +
GetReleaseUrl(*context.get()) +
" with status " + status.ToString());
status = Status(Code::INTERNAL, kFailedFirebaseReleaseFetch);
}

The code does not look as good with status checks within status checks. Which is why I have written code in the way it is.

checker->CallTest(ruleset_id, context, final_continuation);
checker->request_ = std::unique_ptr<FirebaseRequest>(
new FirebaseRequest(ruleset_id, checker->env_, context));
checker->CallNextRequest(final_continuation);
} else {
final_continuation(status);
}
});
}

void AuthzChecker::CallTest(const std::string &ruleset_id,
std::shared_ptr<context::RequestContext> context,
std::function<void(Status status)> continuation) {
std::string body;
Status status = BuildTestRequestBody(*context.get(), &body);
if (!status.ok()) {
continuation(status);
void AuthzChecker::CallNextRequest(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment for what the function is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment that already exists in the class declaration. look at line 69. Let me know if you need me to add anything more to it.

std::function<void(Status status)> continuation) {
if (request_->is_done()) {
continuation(request_->RequestStatus());
return;
}

auto checker = GetPtr();
HttpFetch(GetRulesetTestUri(*context, ruleset_id), kHttpPostMethod, body,
[context, continuation, checker, ruleset_id](Status status,
std::string &&body) {
auto httpRequest = request_->GetHttpRequest();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style, it should be http_request.
prefer to list the type here instead of auto. I like to know which http request type is used since there are so many http request type now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

HttpFetch(httpRequest.url, httpRequest.method, httpRequest.body,
httpRequest.token_type,
[continuation, checker](Status status, std::string &&body) {

checker->env_->LogError(std::string("Response Body = ") + body);
if (status.ok()) {
checker->env_->LogDebug(
std::string("Test API succeeded with ") + body);
status = checker->ParseTestResponse(*context.get(), body);
checker->request_->UpdateResponse(body);
checker->CallNextRequest(continuation);
} else {
checker->env_->LogError(std::string("Test API failed with ") +
status.ToString());
status = Status(Code::INTERNAL, kFailedFirebaseTest);
continuation(status);
}

continuation(status);
});
}

Expand Down Expand Up @@ -242,85 +188,10 @@ Status AuthzChecker::ParseReleaseResponse(const std::string &json_str,
return status;
}

Status AuthzChecker::ParseTestResponse(context::RequestContext &context,
const std::string &json_str) {
grpc_json *json = grpc_json_parse_string_with_len(
const_cast<char *>(json_str.data()), json_str.length());

if (!json) {
return Status(Code::INVALID_ARGUMENT,
"Invalid JSON response from Firebase Service");
}

Status status = Status::OK;
Status invalid = Status(Code::INTERNAL, kInvalidResponse);

const grpc_json *testResults = GetProperty(json, kTestResults);
if (testResults == nullptr) {
env_->LogError("TestResults are null");
status = invalid;
} else {
const char *result = GetStringValue(testResults->child, kState);
if (result == nullptr) {
env_->LogInfo("Result state is empty");
status = invalid;
} else if (std::string(result) != kTestSuccess) {
status = Status(Code::PERMISSION_DENIED,
std::string("Unauthorized ") +
context.request()->GetRequestHTTPMethod() +
" access to resource " +
context.request()->GetRequestPath(),
Status::AUTH);
}
}

grpc_json_destroy(json);
return status;
}

Status AuthzChecker::BuildTestRequestBody(context::RequestContext &context,
std::string *result_string) {
proto::TestRulesetRequest request;
auto *test_case = request.add_test_cases();
auto httpMethod = context.request()->GetRequestHTTPMethod();

test_case->set_service_name(context.service_context()->service_name());
test_case->set_resource_path(context.request()->GetRequestPath());
test_case->set_operation(GetOperation(httpMethod));
test_case->set_expectation(proto::TestRulesetRequest::TestCase::ALLOW);

::google::protobuf::Value auth;
::google::protobuf::Value token;
::google::protobuf::Value claims;

Status status = utils::JsonToProto(context.auth_claims(), &claims);
if (!status.ok()) {
env_->LogError(std::string("Error creating Protobuf from claims") +
status.ToString());
return status;
}

SetProtoValue(kToken, claims, &token);
SetProtoValue(kAuth, token, &auth);

auto *variables = test_case->mutable_variables();
(*variables)[kRequest] = auth;

status =
utils::ProtoToJson(request, result_string, utils::JsonOptions::DEFAULT);
if (status.ok()) {
env_->LogDebug(std::string("PRotobuf to JSON string = ") + *result_string);
} else {
env_->LogError(std::string("Error creating TestRulesetRequest") +
status.ToString());
}

return status;
}

void AuthzChecker::HttpFetch(
const std::string &url, const std::string &method,
const std::string &request_body,
auth::ServiceAccountToken::JWT_TOKEN_TYPE token_type,
std::function<void(Status, std::string &&)> continuation) {
env_->LogDebug(std::string("Issue HTTP Request to url :") + url +
" method : " + method + " body: " + request_body);
Expand All @@ -334,9 +205,10 @@ void AuthzChecker::HttpFetch(
return;
}

request->set_method(method).set_url(url).set_auth_token(GetAuthToken());
request->set_method(method).set_url(url).set_auth_token(
GetAuthToken(token_type));

if (method != kHttpGetMethod) {
if (!request_body.empty()) {
request->set_header(kContentType, kApplication).set_body(request_body);
}

Expand Down
Loading