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

Update the auth check to use service.experimental.authorization.provider #67

Merged
merged 14 commits into from
Feb 8, 2017
Merged

Update the auth check to use service.experimental.authorization.provider #67

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2da31e2
Update the auth check to use service.experimental.authorization.provider
Feb 6, 2017
598d1b1
Address comments and revert accidental change.
Feb 6, 2017
1e3a82d
Merge branch 'firebase' of github.com:wattli/proxy into firebase
Feb 6, 2017
478ae1d
Remove unnecessary added accidentally.
Feb 6, 2017
0eee367
Another patch
Feb 7, 2017
93ed1cc
fix the logic
Feb 7, 2017
b8ca462
fix lint
Feb 7, 2017
1fd53e0
Fix broken test and add unit tests
Feb 7, 2017
bdfe1fc
Fix comments
Feb 7, 2017
05df900
Fix style check
Feb 8, 2017
2e55ff7
revert style for raw string
Feb 8, 2017
519b540
fix small lint
Feb 8, 2017
d33154d
fix small lint
Feb 8, 2017
0d78984
fix small lint
Feb 8, 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
4 changes: 3 additions & 1 deletion contrib/endpoints/repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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,
)
Expand Down
4 changes: 2 additions & 2 deletions contrib/endpoints/src/api_manager/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,14 +516,14 @@ void Config::SetJwksUri(const string &issuer, const string &jwks_uri,
std::string Config::GetFirebaseServer() {
if (server_config_ != nullptr &&
server_config_->has_api_check_security_rules_config() &&
server_config_->api_check_security_rules_config().has_firebase_server()) {
!server_config_->api_check_security_rules_config().firebase_server().empty()) {
// It is the testing environment, use server config.
return server_config_->api_check_security_rules_config().firebase_server();
}

if (service_.has_experimental() &&
service_.experimental().has_authorization() &&
service_.experimental().authorization().has_provider()) {
!service_.experimental().authorization().provider().empty()) {
// In real environment, server_config_ should be null. And we rely on the
// field in service.
return service_.experimental().authorization().provider();
Expand Down
286 changes: 159 additions & 127 deletions contrib/endpoints/src/api_manager/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,17 @@ TEST(Config, CreateFromBinaryProto) {
}

static const char kServerConfig[] = R"(
service_control_config {
check_aggregator_config {
cache_entries: 1000
flush_interval_ms: 10
response_expiration_ms: 20
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
}
}
report_aggregator_config {
cache_entries: 1020
flush_interval_ms: 15
}
}
)";

const char kServiceNameConfig[] = "name: \"service-one\"\n";
Expand All @@ -93,13 +93,13 @@ TEST(Config, ServerConfigProto) {
}

static const char kInvalidServerConfig[] = R"(
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this config invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a new config introduced in the PR.

service_control_config {
type: 1
config {
cache_entries: 1020
flush_interval_ms: 15
service_control_config {
type: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this need any changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To follow google's dev guide: https://screenshot.googleplex.com/eLDuLGnpUNd

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the dev guide only shows the advantage of using raw strings. There is no rules on indentation and here is why: various parsers can treat whitespaces differently. Some may ignore them and some might use them as delimiters. Here is a hint about this in the document:
"
Note that indentation rules, combined with the fact that raw string literals may contain newlines, leave you with an awkward choice on how to indent the first line of your raw string block. Because text protobufs ignore whitespace, this problem can be avoided by throwing in a leading newline (ignored by the parser) in that case, but not all uses of raw strings are so forgiving."

Here is the document link:

https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/64.md?cl=head

Copy link
Contributor

Choose a reason for hiding this comment

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

The safest thing is to not have any leading or trailing white spaces. That way the strings will work with all parsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the raw string style is kinda new to me. And I realized the style even in this file is inconsistent. So I did some search at istio/proxy repo and google3 (https://cs.corp.google.com/search/?q=%22R%5C%22(%22+lang:cc+case:yes) and tried to follow what most people did.

I just kept the existing style as it is. No big deal :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that is because most of google code uses protobufs and they work well with whitespaces.

config {
cache_entries: 1020
flush_interval_ms: 15
}
}
}
)";

TEST(Config, InvalidServerConfigProto) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -751,41 +750,41 @@ TEST(Config, TestHttpOptions) {
MockApiManagerEnvironmentWithLog env;

static const char config_text[] = R"(
name: "Service.Name"
endpoints {
name: "Service.Name"
allow_cors: true
}
http {
rules {
selector: "ListShelves"
get: "/shelves"
}
rules {
selector: "CorsShelves"
custom: {
kind: "OPTIONS"
path: "/shelves"
}
}
rules {
selector: "CreateShelf"
post: "/shelves"
}
rules {
selector: "GetShelf"
get: "/shelves/{shelf}"
}
rules {
selector: "DeleteShelf"
delete: "/shelves/{shelf}"
}
rules {
selector: "GetShelfBook"
get: "/shelves/{shelf}/books"
}
}
)";
name: "Service.Name"
endpoints {
name: "Service.Name"
allow_cors: true
}
http {
rules {
selector: "ListShelves"
get: "/shelves"
}
rules {
selector: "CorsShelves"
custom: {
kind: "OPTIONS"
path: "/shelves"
}
}
rules {
selector: "CreateShelf"
post: "/shelves"
}
rules {
selector: "GetShelf"
Copy link
Contributor

Choose a reason for hiding this comment

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

revert the added spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://screenshot.googleplex.com/eLDuLGnpUNd

Copy link
Contributor

Choose a reason for hiding this comment

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

The document does not say anything about required indentation. Please read my previous comment.

get: "/shelves/{shelf}"
}
rules {
selector: "DeleteShelf"
delete: "/shelves/{shelf}"
}
rules {
selector: "GetShelfBook"
get: "/shelves/{shelf}/books"
}
}
)";

std::unique_ptr<Config> config = Config::Create(&env, config_text, "");
ASSERT_TRUE(config);
Expand Down Expand Up @@ -818,22 +817,22 @@ TEST(Config, TestHttpOptionsSelector) {
MockApiManagerEnvironmentWithLog env;

static const char config_text[] = R"(
name: "Service.Name"
endpoints {
name: "Service.Name"
allow_cors: true
}
http {
rules {
selector: "CORS"
get: "/shelves"
}
rules {
selector: "CORS.1"
get: "/shelves/{shelf}"
}
}
)";
name: "Service.Name"
endpoints {
name: "Service.Name"
allow_cors: true
}
http {
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert the added spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://screenshot.googleplex.com/eLDuLGnpUNd

rules {
selector: "CORS"
get: "/shelves"
}
rules {
selector: "CORS.1"
get: "/shelves/{shelf}"
}
}
)";

std::unique_ptr<Config> config = Config::Create(&env, config_text, "");
ASSERT_TRUE(config);
Expand All @@ -850,18 +849,18 @@ TEST(Config, TestCorsDisabled) {
MockApiManagerEnvironmentWithLog env;

static const char config_text[] = R"(
name: "Service.Name"
http {
rules {
selector: "CORS"
get: "/shelves"
}
rules {
selector: "CORS.1"
get: "/shelves/{shelf}"
}
}
)";
name: "Service.Name"
http {
rules {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you are changing text by just adding spaces. This was done is many places elsewhere. Can you fix this? It is polluting the actual changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The style is recommended by Google dev guide
https://screenshot.googleplex.com/eLDuLGnpUNd

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any such recommendation. It is only an example of a string that can be converted to a proto.

selector: "CORS"
get: "/shelves"
}
rules {
selector: "CORS.1"
get: "/shelves/{shelf}"
}
}
)";

std::unique_ptr<Config> config = Config::Create(&env, config_text, "");
ASSERT_TRUE(config);
Expand All @@ -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: "[email protected]"
}
}
)";

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(), "[email protected]");
}
)";

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

Expand Down