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 HTTP functions in firebase rules to specify audience #244

Merged
merged 5 commits into from
Apr 12, 2017
Merged

Allow HTTP functions in firebase rules to specify audience #244

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c472a9
Allow HTTP functions in firebase rules to specify audience
sarvaniv Apr 12, 2017
f78d039
Allow GetAuthToken to ignore cache and fix style checks.
sarvaniv Apr 12, 2017
46bb6cc
Fix GetAuthToken
sarvaniv Apr 12, 2017
e47d0f2
Address Wayne's comment
sarvaniv Apr 12, 2017
aa1ddc8
Check for empty response body
sarvaniv Apr 12, 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
9 changes: 5 additions & 4 deletions contrib/endpoints/src/api_manager/auth/service_account_token.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,12 @@ void ServiceAccountToken::SetAudience(JWT_TOKEN_TYPE type,
jwt_tokens_[type].set_audience(audience);
}

const std::string& ServiceAccountToken::GetAuthToken(JWT_TOKEN_TYPE type) {
const std::string& ServiceAccountToken::GetAuthToken(JWT_TOKEN_TYPE type,
bool ignore_cache) {
// Uses authentication secret if available.
if (!client_auth_secret_.empty()) {
GOOGLE_CHECK(type >= 0 && type < JWT_TOKEN_TYPE_MAX);
if (!jwt_tokens_[type].is_valid(0)) {
if (ignore_cache || !jwt_tokens_[type].is_valid(0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to compare the new audience with old audience, if diff, wipe out the cache.

Status status = jwt_tokens_[type].GenerateJwtToken(client_auth_secret_);
if (!status.ok()) {
if (env_) {
Expand All @@ -79,9 +80,9 @@ const std::string& ServiceAccountToken::GetAuthToken(JWT_TOKEN_TYPE type) {
}

const std::string& ServiceAccountToken::GetAuthToken(
JWT_TOKEN_TYPE type, const std::string& audience) {
JWT_TOKEN_TYPE type, const std::string& audience, bool ignore_cache) {
SetAudience(type, audience);
Copy link
Contributor

Choose a reason for hiding this comment

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

Token is cached, you have to clear the cache if audience is different.

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 see. Thank you for pointing this out.

return GetAuthToken(type);
return GetAuthToken(type, ignore_cache);
}

Status ServiceAccountToken::JwtTokenInfo::GenerateJwtToken(
Expand Down
8 changes: 6 additions & 2 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 @@ -77,14 +77,18 @@ class ServiceAccountToken {
// Gets the auth token to access Google services.
// If client auth secret is specified, use it to calcualte JWT token.
// Otherwise, use the access token fetched from metadata server.
const std::string& GetAuthToken(JWT_TOKEN_TYPE type);
// If ignore_cache is true then a JWT token is regenerated even if the current
// cached JWT token is valid.
const std::string& GetAuthToken(JWT_TOKEN_TYPE type,
bool ignore_cache = false);

// Gets the auth token to access Google services. This method accepts an
Copy link

Choose a reason for hiding this comment

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

The target service does not have to be Google services. Correct?

// audience parameter to set when generating JWT token.
// If client auth secret is specified, use it to calcualte JWT token.
// Otherwise, use the access token fetched from metadata server.
const std::string& GetAuthToken(JWT_TOKEN_TYPE type,
const std::string& audience);
const std::string& audience,
bool ignore_cache = false);

private:
// Stores base token info. Used for both OAuth and JWT tokens.
Expand Down
5 changes: 2 additions & 3 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 @@ -111,9 +111,8 @@ void AuthzChecker::Check(
auto checker = GetPtr();
HttpFetch(GetReleaseUrl(*context), kHttpGetMethod, "",
auth::ServiceAccountToken::JWT_TOKEN_FOR_FIREBASE,
kFirebaseAudience,
[context, final_continuation, checker](Status status,
std::string &&body) {
kFirebaseAudience, [context, final_continuation, checker](
Status status, std::string &&body) {
std::string ruleset_id;
if (status.ok()) {
checker->env_->LogDebug(
Expand Down
59 changes: 31 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 @@ -537,26 +537,30 @@ class CheckSecurityRulesFunctions : public CheckSecurityRulesTest,
InSequence s;

ExpectCall(release_url_, "GET", "", kRelease);
ExpectCall(
ruleset_test_url_, "POST", kFirstRequest,
BuildTestRulesetResponse(
false, {std::make_tuple("f1", "http://url1", "POST", kDummyBody, kDummyAudience)}));
ExpectCall(ruleset_test_url_, "POST", kFirstRequest,
BuildTestRulesetResponse(
false, {std::make_tuple("f1", "http://url1", "POST",
kDummyBody, kDummyAudience)}));

ExpectCall("http://url1", "POST", kDummyBody, kDummyBody);
ExpectCall(
ruleset_test_url_, "POST", kSecondRequest,
BuildTestRulesetResponse(
false, {std::make_tuple("f2", "http://url2", "GET", kDummyBody, kDummyAudience),
std::make_tuple("f3", "https://url3", "GET", kDummyBody, kDummyAudience),
std::make_tuple("f1", "http://url1", "POST", kDummyBody, kDummyAudience)}));
ExpectCall(ruleset_test_url_, "POST", kSecondRequest,
BuildTestRulesetResponse(
false, {std::make_tuple("f2", "http://url2", "GET",
kDummyBody, kDummyAudience),
std::make_tuple("f3", "https://url3", "GET",
kDummyBody, kDummyAudience),
std::make_tuple("f1", "http://url1", "POST",
kDummyBody, kDummyAudience)}));
ExpectCall("http://url2", "GET", kDummyBody, kDummyBody);
ExpectCall("https://url3", "GET", kDummyBody, kDummyBody);
ExpectCall(ruleset_test_url_, "POST", kThirdRequest,
BuildTestRulesetResponse(
GetParam(),
{std::make_tuple("f2", "http://url2", "GET", kDummyBody, kDummyAudience),
std::make_tuple("f3", "https://url3", "GET", kDummyBody, kDummyAudience),
std::make_tuple("f1", "http://url1", "POST", kDummyBody, kDummyAudience)}));
GetParam(), {std::make_tuple("f2", "http://url2", "GET",
kDummyBody, kDummyAudience),
std::make_tuple("f3", "https://url3", "GET",
kDummyBody, kDummyAudience),
std::make_tuple("f1", "http://url1", "POST",
kDummyBody, kDummyAudience)}));
}
};

Expand Down Expand Up @@ -622,20 +626,19 @@ TEST_P(CheckSecurityRulesBadFunctions, CheckBadFunctionArguments) {
});
}

INSTANTIATE_TEST_CASE_P(CheckSecurityRulesBadFunctionArguments,
CheckSecurityRulesBadFunctions,
::testing::Values(
// Empty function name
std::make_tuple("", "http://url1", "POST",
kDummyBody, kDummyAudience),
// Argument count less than 3
std::make_tuple("f1", "http://url1", "", "", kDummyAudience),
// The url is not set
std::make_tuple("f1", "", "POST", kDummyBody, kDummyAudience),
// The url is not a http or https protocol
std::make_tuple("f1", "ftp://url1", "POST", kDummyBody, kDummyAudience),
// The audience is not present
std::make_tuple("f1", "http://url1", "GET", kDummyBody, "")));
INSTANTIATE_TEST_CASE_P(
CheckSecurityRulesBadFunctionArguments, CheckSecurityRulesBadFunctions,
::testing::Values(
// Empty function name
std::make_tuple("", "http://url1", "POST", kDummyBody, kDummyAudience),
// Argument count less than 3
std::make_tuple("f1", "http://url1", "", "", kDummyAudience),
// The url is not set
std::make_tuple("f1", "", "POST", kDummyBody, kDummyAudience),
// The url is not a http or https protocol
std::make_tuple("f1", "ftp://url1", "POST", kDummyBody, kDummyAudience),
// The audience is not present
std::make_tuple("f1", "http://url1", "GET", kDummyBody, "")));
}
} // namespace api_manager
} // namespace google
8 changes: 5 additions & 3 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 @@ -310,7 +310,8 @@ Status FirebaseRequest::SetNextRequest() {
auto call = *func_call_iter_;
external_http_request_.url = call.args(0).string_value();
external_http_request_.method = call.args(1).string_value();
external_http_request_.audience = call.args(call.args_size()-1).string_value();
external_http_request_.audience =
call.args(call.args_size() - 1).string_value();
Copy link

Choose a reason for hiding this comment

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

"audience" is an optional field here. For datastore, we provide a default audience if not specified.

How do you handle the case that "audience" field is provided by in the rules for datastore? Do you overwrite the audience field with the default datastore audience?

How do you handle the case that "audience" is not provided? What is the default audience?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not optional right now since there is no default behavior implemented right now. There is no default audience being specified at all.

std::string body;
status =
utils::ProtoToJson(call.args(2), &body, utils::JsonOptions::DEFAULT);
Expand Down Expand Up @@ -354,11 +355,12 @@ Status FirebaseRequest::CheckFuncCallArgs(const FunctionCall &func) {
std::string(func.function() + " Arguments 1 and 2 should be strings"));
}

if (func.args(func.args_size()-1).kind_case() != google::protobuf::Value::kStringValue) {
if (func.args(func.args_size() - 1).kind_case() !=
google::protobuf::Value::kStringValue) {
return Status(
Code::INVALID_ARGUMENT,
std::string(func.function() + "The last argument should be a string"
"that specifies audience"));
"that specifies audience"));
}

if (!utils::IsHttpRequest(func.args(0).string_value())) {
Expand Down