Skip to content

Add HTTP request headers to Firebase TestRuleset request. #246

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

Closed
sarvaniv wants to merge 5 commits into from
Closed

Add HTTP request headers to Firebase TestRuleset request. #246

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a22eaa
Add HTTP request headers to Firebase TestRuleset request.
sarvaniv Apr 13, 2017
4031500
Address code review comment. Fix style.
sarvaniv Apr 13, 2017
0c0d0da
Add comment about why the method cannot be pure virtual method.
sarvaniv Apr 13, 2017
6769a20
Move CheckSecurityRules before CheckServiceControl in workflow.
sarvaniv Apr 13, 2017
31c582b
Convert GetHeaders to be pure virtual.
sarvaniv Apr 13, 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/include/api_manager/request.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ class Request {
// Adds a header to backend. If the header exists, overwrite its value
virtual utils::Status AddHeaderToBackend(const std::string &key,
const std::string &value) = 0;

// Gets all HTTP request headers.
virtual void GetHeaders(std::map<std::string, std::string> *headers) {}

@liminw liminw Apr 13, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't this be "= 0"? I suppose we are going to have a follow up change in ESP repo to provide the implementation of GetHeaders()?

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have provided all the information in the PR description. This cannot be 0. If this is 0, then ESP that will point to this proxy's commit will have issues creating a request object since it does not implement a pure virtual function. Having this empty body will fix that issue.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can convert this into a pure virtual function but we have to wait until we merged back to main whenever that is going to happen. Until then this code cannot be changed.

@liminw liminw Apr 13, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Makes sense. Probably you can have a comment here saying that this is a workaround needed until ESP implementation is done, plus proxy and ESP repo are synced.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On a second thought we have to very careful about making this pure virtual since Envoy also implements this class. Let's not be too eager to change this into a pure virtual function.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As discussed with Wayne, making this pure virtual right now can break other's using the latest proxy and ESP code. This is the safest thing to do. I also want to know the full Envoy story. Someone is implementing the request class that can be used on Proxy when used with Envoy. I don't know who implements this class and we use this Request class everywhere in the proxy code. Let's take the safest route and not break anyone. I think its good karma :-)

@qiwzhang qiwzhang Apr 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agreed with Lizan. If you worry about the breakage of ESP, you should immediately make ESP PR with dummy implementation.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You mean in master branch? I don't care about this branch since we are just 2 people on this branch. But do you want me to first do this change on master so that we can merge both the changes together?

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, when we merge into master, we need to remember an additional step that before we do anything on ESP branch, we provide the dummy implementation in ESP and then merge firebase branch into master.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am actually not completely sure what is the big deal since this will become a pure virtual function soon and in a way that will make everyone's lives easy and also without the pressure of having to time the merges for proxy and ESP. Just wondering the reason why this is such a concern?

@lizan lizan Apr 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually having a empty implementation here make it harder to notice some implementation don't implement this, since the build won't break.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The build will break if someone in the team have their ESP point to the latest proxy code as I do a lot during my development/testing. Especially when we merge these changes into master. We cannot be too far off when we merge both proxy and ESP changes into the master and depending on how things go, this is not something we should be too sure about. I must disagree on this. I will make it a pure virtual function once we are in master.

@lizan lizan Apr 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't notice that this is firebase branch. Feel free to merge this but I would block this when you try to merge to master.

This class here is a pure interface, so it shouldn't have any (even if it is dummy) implementations at any time.

Currently the only implementation of this interface is in ESP, and ESP depends on istio/proxy on specific commit. So I don't think making this pure virtual will break anything.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

While ESP depends on a specific commit, the general guideline for merging proxy code is that we point ESP to the local proxy and make sure that ESP tests also pass before we merge changes. But this change is a breaking change if we make it pure virtual right away. Is it OK to block peoples' progress? Instead we will have this temporary fix that will allow people to keep doing their work while we can work on any blocking issues that may come up during the merge. I think the first principle is to never block people and let things go smoothly for everyone. Sometimes perfection is not the best solution :-)

@lizan lizan Apr 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, some refactoring cannot be done without breaking ESP, (e.g. #225 and ESP change). It is OK to merge them, then to mitigate the issue, have a dummy (of course if you have real one it is better) implementation ASAP in ESP too.

This is not a big deal, but comparing 2 solutions, (A) adding a dummy implementation first hand v.s. (B) changing this method to pure virtual are almost same effort (in terms of lines of change, number of PRs), when we do (B), it is hard to track whether the fix is made as nothing is breaking anymore.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If this method does not become a pure virutal function within a few hours of merging ESP into master, please hold me responsible for that. We can convert it into a pure virtual function without breaking builds or trying to keep merges to ESP and proxy very close when there is at least 1-2 hour delay for tests to run. Given that we are merging into main from firebase, this may take longer if we find any issues. I am hoping to make a strong case that we should be able to merge this code into master.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The primary difference between A and B, is that the merges are smooth and no one is blocked since he merge cannot be instantaneous. I think making builds non-breaking is the best thing we can do to our fellow team members and should make this a strong requirement for all merges. I would personally dislike being blocked on build breaks.

@lizan lizan Apr 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the only case you're blocking with A is someone want to update the istio/proxy commit contains your change in ESP, since this is happening in firebase branch, so it doesn't block anyone.

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not talking about firebase branch. I am talking about the case when we merge into master. As I said this will be very temporary change and I will make sure that the method becomes pure virtual ASAP once we merge to master. I still don't know what is the issue here...

@sarvaniv sarvaniv Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright, I will make this pure virutal and before merging into master, I will have a dummy implementation that I will check into ESP branch. Thank you all for the input.

};

} // namespace api_manager
Expand Down
39 changes: 11 additions & 28 deletions contrib/endpoints/src/api_manager/check_security_rules_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,13 @@ static const char kDummyBody[] = R"(
static const char kDummyAudience[] = "test-audience";

const char kFirstRequest[] =
R"({"testSuite":{"testCases":[{"expectation":"ALLOW","request":{"method":"get","path":"/ListShelves","auth":{"token":{"email":"limin-429@appspot.gserviceaccount.com","email_verified":true,"azp":"limin-429@appspot.gserviceaccount.com","aud":"https://myfirebaseapp.appspot.com","sub":"113424383671131376652","iat":1486575396,"iss":"https://accounts.google.com","exp":1486578996}}}}]}})";
R"({"testSuite":{"testCases":[{"expectation":"ALLOW","request":{"headers":{"Pragma":"Custom value"},"path":"/ListShelves","method":"get","auth":{"token":{"email":"limin-429@appspot.gserviceaccount.com","email_verified":true,"azp":"limin-429@appspot.gserviceaccount.com","sub":"113424383671131376652","aud":"https://myfirebaseapp.appspot.com","iat":1486575396,"iss":"https://accounts.google.com","exp":1486578996}}}}]}})";

const char kSecondRequest[] =
R"({"testSuite":{"testCases":[{"expectation":"ALLOW","request":{"auth":{"token":{"email":"limin-429@appspot.gserviceaccount.com","azp":"limin-429@appspot.gserviceaccount.com","aud":"https://myfirebaseapp.appspot.com","sub":"113424383671131376652","iss":"https://accounts.google.com","email_verified":true,"iat":1486575396,"exp":1486578996}},"method":"get","path":"/ListShelves"},"functionMocks":[{"function":"f1","args":[{"exactValue":"http://url1"},{"exactValue":"POST"},{"exactValue":{"key":"value"}},{"exactValue":"test-audience"}],"result":{"value":{"key":"value"}}}]}]}})";
R"({"testSuite":{"testCases":[{"expectation":"ALLOW","request":{"headers":{"Pragma":"Custom value"},"auth":{"token":{"email":"limin-429@appspot.gserviceaccount.com","azp":"limin-429@appspot.gserviceaccount.com","iat":1486575396,"email_verified":true,"aud":"https://myfirebaseapp.appspot.com","sub":"113424383671131376652","exp":1486578996,"iss":"https://accounts.google.com"}},"method":"get","path":"/ListShelves"},"functionMocks":[{"function":"f1","args":[{"exactValue":"http://url1"},{"exactValue":"POST"},{"exactValue":{"key":"value"}},{"exactValue":"test-audience"}],"result":{"value":{"key":"value"}}}]}]}})";

const char kThirdRequest[] =
R"({"testSuite":{"testCases":[{"expectation":"ALLOW","request":{"method":"get","path":"/ListShelves","auth":{"token":{"email":"limin-429@appspot.gserviceaccount.com","iat":1486575396,"azp":"limin-429@appspot.gserviceaccount.com","exp":1486578996,"email_verified":true,"sub":"113424383671131376652","aud":"https://myfirebaseapp.appspot.com","iss":"https://accounts.google.com"}}},"functionMocks":[{"function":"f2","args":[{"exactValue":"http://url2"},{"exactValue":"GET"},{"exactValue":{"key":"value"}},{"exactValue":"test-audience"}],"result":{"value":{"key":"value"}}},{"function":"f3","args":[{"exactValue":"https://url3"},{"exactValue":"GET"},{"exactValue":{"key":"value"}},{"exactValue":"test-audience"}],"result":{"value":{"key":"value"}}},{"function":"f1","args":[{"exactValue":"http://url1"},{"exactValue":"POST"},{"exactValue":{"key":"value"}},{"exactValue":"test-audience"}],"result":{"value":{"key":"value"}}}]}]}})";
R"({"testSuite":{"testCases":[{"expectation":"ALLOW","request":{"headers":{"Pragma":"Custom value"},"auth":{"token":{"email":"limin-429@appspot.gserviceaccount.com","exp":1486578996,"iss":"https://accounts.google.com","iat":1486575396,"sub":"113424383671131376652","aud":"https://myfirebaseapp.appspot.com","azp":"limin-429@appspot.gserviceaccount.com","email_verified":true}},"method":"get","path":"/ListShelves"},"functionMocks":[{"function":"f2","args":[{"exactValue":"http://url2"},{"exactValue":"GET"},{"exactValue":{"key":"value"}},{"exactValue":"test-audience"}],"result":{"value":{"key":"value"}}},{"function":"f3","args":[{"exactValue":"https://url3"},{"exactValue":"GET"},{"exactValue":{"key":"value"}},{"exactValue":"test-audience"}],"result":{"value":{"key":"value"}}},{"function":"f1","args":[{"exactValue":"http://url1"},{"exactValue":"POST"},{"exactValue":{"key":"value"}},{"exactValue":"test-audience"}],"result":{"value":{"key":"value"}}}]}]}})";

::google::protobuf::Value ToValue(const std::string &arg) {
::google::protobuf::Value value;
Expand Down Expand Up @@ -198,31 +198,6 @@ MATCHER_P3(HTTPRequestMatches, url, method, body, "") {
return MessageDifferencer::Equals(actual, expected);
}

FunctionCall BuildCall(const std::string &name, const std::string &url,
const std::string &method, const std::string &body,
const std::string &audience) {
FunctionCall func_call;
func_call.set_function(name);

if (!url.empty()) {
*(func_call.add_args()) = ToValue(url);
}

if (!method.empty()) {
*(func_call.add_args()) = ToValue(method);
}

if (!body.empty()) {
*(func_call.add_args()) = ToValue(body);
}

if (!audience.empty()) {
*(func_call.add_args()) = ToValue(audience);
}

return func_call;
}

// Get a server configuration that has auth disabled. This should disable
// security rules check by default.
std::pair<std::string, std::string> GetConfigWithAuthForceDisabled() {
Expand Down Expand Up @@ -346,6 +321,14 @@ class CheckSecurityRulesTest : public ::testing::Test {
ON_CALL(*raw_request_, GetRequestPath())
.WillByDefault(Return(std::string("/ListShelves")));

ON_CALL(*raw_request_, GetHeaders(_))
.WillByDefault(
testing::Invoke([](std::map<std::string, std::string> *map) {
if (map != nullptr) {
(*map)["Pragma"] = "Custom value";
}
}));

request_context_ = std::make_shared<context::RequestContext>(
service_context_, std::move(request));
release_url_ =
Expand Down
26 changes: 26 additions & 0 deletions contrib/endpoints/src/api_manager/firebase_rules/firebase_request.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const std::string kToken = "token";
const std::string kAuth = "auth";
const std::string kPath = "path";
const std::string kMethod = "method";
const std::string kHeaders = "headers";
const std::string kHttpGetMethod = "GET";
const std::string kHttpPostMethod = "POST";
const std::string kHttpHeadMethod = "HEAD";
Expand Down Expand Up @@ -180,6 +181,7 @@ Status FirebaseRequest::UpdateRulesetRequestBody(
::google::protobuf::Value claims;
::google::protobuf::Value path;
::google::protobuf::Value method;
::google::protobuf::Value headers;

Status status = utils::JsonToProto(context_->auth_claims(), &claims);
if (!status.ok()) {
Expand All @@ -188,6 +190,10 @@ Status FirebaseRequest::UpdateRulesetRequestBody(

auto *variables = test_case->mutable_request()->mutable_struct_value();
auto *fields = variables->mutable_fields();
GetHeaders(&headers);
if (headers.kind_case() != ::google::protobuf::Value::kNullValue) {
(*fields)[kHeaders] = headers;
}

path.set_string_value(context_->request()->GetRequestPath());
(*fields)[kPath] = path;
Expand Down Expand Up @@ -216,6 +222,26 @@ Status FirebaseRequest::UpdateRulesetRequestBody(
return status;
}

void FirebaseRequest::GetHeaders(::google::protobuf::Value *headers) {
if (headers == nullptr) {
return;
}

std::map<std::string, std::string> req_headers;
context_->request()->GetHeaders(&req_headers);
if (req_headers.size() == 0) {
headers->set_null_value(::google::protobuf::NullValue::NULL_VALUE);
return;
}

auto *map = headers->mutable_struct_value()->mutable_fields();
for (const auto &kvp : req_headers) {
::google::protobuf::Value value;
value.set_string_value(kvp.second);
(*map)[kvp.first] = value;
}
}

Status FirebaseRequest::ProcessTestRulesetResponse(const std::string &body) {
Status status = utils::JsonToProto(body, &response_);
if (!status.ok()) {
Expand Down
1 change: 1 addition & 0 deletions contrib/endpoints/src/api_manager/firebase_rules/firebase_request.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ class FirebaseRequest {
std::vector<std::pair<proto::TestRulesetResponse::TestResult::FunctionCall,
std::string>>::const_iterator
Find(const proto::TestRulesetResponse::TestResult::FunctionCall &func_call);
void GetHeaders(::google::protobuf::Value *value);

// The API manager environment. Primarily used for logging.
ApiManagerEnvInterface *env_;
Expand Down
1 change: 1 addition & 0 deletions contrib/endpoints/src/api_manager/mock_request.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class MockRequest : public Request {
MOCK_METHOD0(GetGrpcResponseBytes, int64_t());
MOCK_METHOD0(GetGrpcRequestMessageCounts, int64_t());
MOCK_METHOD0(GetGrpcResponseMessageCounts, int64_t());
MOCK_METHOD1(GetHeaders, void(std::map<std::string, std::string> *));
};

} // namespace api_manager
Expand Down