Skip to content

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

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

Merged
wattli 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 4 commits
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
14 changes: 0 additions & 14 deletions .idea/misc.xml
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions .idea/modules.xml
View file
Open in desktop

This file was deleted.

13 changes: 0 additions & 13 deletions .idea/proxy.iml
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions .idea/vcs.xml
View file
Open in desktop

This file was deleted.

354 changes: 0 additions & 354 deletions .idea/workspace.xml
View file
Open in desktop

This file was deleted.

8 changes: 8 additions & 0 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 @@ -515,9 +515,17 @@ void Config::SetJwksUri(const string &issuer, const string &jwks_uri,

std::string Config::GetFirebaseServer() {
if (server_config_ == nullptr) {

@sarvaniv sarvaniv Feb 7, 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.

This check is not sufficient. You need to check the following:

if (server_config_ && !server_config_->api_check_security_rules_config()->firebase_server().empty) {
return server_config_->api_check_security_rules_config().firebase_server();
}

Then you can add the code you have added.

@sarvaniv sarvaniv Feb 7, 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.

You will also need to update the config unit tests to check the behavior.

@wattli wattli Feb 7, 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.

Again, this review is based on an old version. Updated anyway. Is the following check enough? What if server_config_ is not null, but server_config_->api_check_security_rules_config() is null?

if (server_config_ && !server_config_->api_check_security_rules_config()->firebase_server().empty)

if (service_.has_experimental() &&
service_.experimental().has_authorization() &&
service_.experimental().authorization().has_provider()) {
// In real environment, server_config_ should be null. And we rely on the
// field in service.
return service_.experimental().authorization().provider();
}
return "";
}

// It is the testing environment, use server config.

@qiwzhang qiwzhang Feb 6, 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.

By design, server_config over-ride service_config. Please check server_config first.

Server_config may have other fields, only if it doesn't have firebase server field, check service_config.

@wattli wattli Feb 7, 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.

Done

return server_config_->api_check_security_rules_config().firebase_server();
}

Expand Down
4 changes: 1 addition & 3 deletions contrib/endpoints/src/api_manager/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ class Config {
// TODO: Remove in favor of service().
const std::string &service_name() const { return service_.name(); }

bool HasAuth() const { return service_.has_experimental() &&
service_.experimental().has_authorization() &&
service_.experimental().authorization().has_provider(); }
bool HasAuth() const { return service_.has_authentication(); }

// Returns true if the caller should try openId discovery to fetch jwksUri.
// url is set to the openId discovery link in this case. Returns false
Expand Down