Skip to content

Commit

Permalink
[Signin] Record Oauth2 response regardless of the response code
Browse files Browse the repository at this point in the history 
This CL moves from `Gaia.BadRequestTypeForOAuth2AccessToken` histogram
that is only recorded if Oauth2 response code is 400 (bad request) to a
more generic histogram recorded for any response.

Note: Some of the buckets like `OAUTH2_ACCESS_ERROR_INVALID_CLIENT` were
never recorded as the server sends a response code 401 (Unauthorized)
for this error not 400.

`OAuth2ErrorCodes` will be used in a future CL to fire the right
`GoogleServiceAuthError` as the response code is not sufficient.

Bug: 1336627
Change-Id: If5b813479a9828dd6a45ce7a1295cebb59c3cf0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3710093
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Weilun Shi <sweilun@chromium.org>
Commit-Queue: Monica Basta <msalama@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1019574}
  • Loading branch information
Monica Basta authored and Chromium LUCI CQ committed Jun 30, 2022
1 parent fa238a0 commit 900a36d
Show file tree
Hide file tree
Showing 7 changed files with 265 additions and 138 deletions.
18 changes: 12 additions & 6 deletions google_apis/gaia/gaia_access_token_fetcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@
#include "google_apis/gaia/gaia_urls.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"

// static
const char GaiaAccessTokenFetcher::kOAuth2NetResponseCodeHistogramName[] =
"Gaia.ResponseCodesForOAuth2AccessToken";

// static
const char GaiaAccessTokenFetcher::kOAuth2ResponseHistogramName[] =
"Gaia.ResponseForOAuth2AccessToken";

// static
std::unique_ptr<GaiaAccessTokenFetcher>
GaiaAccessTokenFetcher::CreateExchangeRefreshTokenForAccessTokenInstance(
Expand Down Expand Up @@ -48,14 +56,12 @@ GaiaAccessTokenFetcher::GaiaAccessTokenFetcher(
GaiaAccessTokenFetcher::~GaiaAccessTokenFetcher() = default;

void GaiaAccessTokenFetcher::RecordResponseCodeUma(int error_value) const {
base::UmaHistogramSparse("Gaia.ResponseCodesForOAuth2AccessToken",
error_value);
base::UmaHistogramSparse(kOAuth2NetResponseCodeHistogramName, error_value);
}

void GaiaAccessTokenFetcher::RecordBadRequestTypeUma(
OAuth2ErrorCodesForHistogram access_error) const {
UMA_HISTOGRAM_ENUMERATION("Gaia.BadRequestTypeForOAuth2AccessToken",
access_error, OAUTH2_ACCESS_ERROR_COUNT);
void GaiaAccessTokenFetcher::RecordOAuth2Response(
OAuth2Response response) const {
base::UmaHistogramEnumeration(kOAuth2ResponseHistogramName, response);
}

GURL GaiaAccessTokenFetcher::GetAccessTokenURL() const {
Expand Down
6 changes: 4 additions & 2 deletions google_apis/gaia/gaia_access_token_fetcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ class SharedURLLoaderFactory;
// https://developers.google.com/identity/protocols/oauth2/web-server?csw=1#obtainingaccesstokens
class GaiaAccessTokenFetcher : public OAuth2AccessTokenFetcherImpl {
public:
static const char kOAuth2NetResponseCodeHistogramName[];
static const char kOAuth2ResponseHistogramName[];

static std::unique_ptr<GaiaAccessTokenFetcher>
CreateExchangeRefreshTokenForAccessTokenInstance(
OAuth2AccessTokenConsumer* consumer,
Expand All @@ -45,8 +48,7 @@ class GaiaAccessTokenFetcher : public OAuth2AccessTokenFetcherImpl {

// OAuth2AccessTokenFetcherImpl:
void RecordResponseCodeUma(int error_value) const override;
void RecordBadRequestTypeUma(
OAuth2ErrorCodesForHistogram access_error) const override;
void RecordOAuth2Response(OAuth2Response response) const override;
GURL GetAccessTokenURL() const override;
net::NetworkTrafficAnnotationTag GetTrafficAnnotationTag() const override;
};
Expand Down
184 changes: 96 additions & 88 deletions google_apis/gaia/oauth2_access_token_fetcher_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,30 +49,39 @@ constexpr char kExpiresInKey[] = "expires_in";
constexpr char kIdTokenKey[] = "id_token";
constexpr char kErrorKey[] = "error";

OAuth2AccessTokenFetcherImpl::OAuth2ErrorCodesForHistogram
OAuth2ErrorToHistogramValue(const std::string& error) {
OAuth2AccessTokenFetcherImpl::OAuth2Response
OAuth2ResponseErrorToOAuth2Response(const std::string& error) {
if (error.empty())
return OAuth2AccessTokenFetcherImpl::kErrorUnexpectedFormat;

if (error == "invalid_request")
return OAuth2AccessTokenFetcherImpl::OAUTH2_ACCESS_ERROR_INVALID_REQUEST;
else if (error == "invalid_client")
return OAuth2AccessTokenFetcherImpl::OAUTH2_ACCESS_ERROR_INVALID_CLIENT;
else if (error == "invalid_grant")
return OAuth2AccessTokenFetcherImpl::OAUTH2_ACCESS_ERROR_INVALID_GRANT;
else if (error == "unauthorized_client")
return OAuth2AccessTokenFetcherImpl::
OAUTH2_ACCESS_ERROR_UNAUTHORIZED_CLIENT;
else if (error == "unsupported_grant_type")
return OAuth2AccessTokenFetcherImpl::
OAUTH2_ACCESS_ERROR_UNSUPPORTED_GRANT_TYPE;
else if (error == "invalid_scope")
return OAuth2AccessTokenFetcherImpl::OAUTH2_ACCESS_ERROR_INVALID_SCOPE;

return OAuth2AccessTokenFetcherImpl::OAUTH2_ACCESS_ERROR_UNKNOWN;
}
return OAuth2AccessTokenFetcherImpl::kInvalidRequest;

if (error == "invalid_client")
return OAuth2AccessTokenFetcherImpl::kInvalidClient;

if (error == "invalid_grant")
return OAuth2AccessTokenFetcherImpl::kInvalidGrant;

if (error == "unauthorized_client")
return OAuth2AccessTokenFetcherImpl::kUnauthorizedClient;

if (error == "unsupported_grant_type")
return OAuth2AccessTokenFetcherImpl::kUnsuportedGrantType;

if (error == "invalid_scope")
return OAuth2AccessTokenFetcherImpl::kInvalidScope;

if (error == "restricted_client")
return OAuth2AccessTokenFetcherImpl::kRestrictedClient;

if (error == "rate_limit_exceeded")
return OAuth2AccessTokenFetcherImpl::kRateLimitExceeded;

if (error == "internal_failure")
return OAuth2AccessTokenFetcherImpl::kInternalFailure;

static GoogleServiceAuthError CreateAuthError(int net_error) {
CHECK_NE(net_error, net::OK);
DLOG(WARNING) << "Could not reach Authorization servers: errno " << net_error;
return GoogleServiceAuthError::FromConnectionError(net_error);
return OAuth2AccessTokenFetcherImpl::kUnknownError;
}

static std::unique_ptr<network::SimpleURLLoader> CreateURLLoader(
Expand Down Expand Up @@ -105,20 +114,6 @@ static std::unique_ptr<network::SimpleURLLoader> CreateURLLoader(

return url_loader;
}

std::unique_ptr<base::DictionaryValue> ParseGetAccessTokenResponse(
std::unique_ptr<std::string> data) {
if (!data)
return nullptr;

std::unique_ptr<base::Value> value = base::JSONReader::ReadDeprecated(*data);
if (!value.get() || value->type() != base::Value::Type::DICTIONARY)
value.reset();

return std::unique_ptr<base::DictionaryValue>(
static_cast<base::DictionaryValue*>(value.release()));
}

} // namespace

OAuth2AccessTokenFetcherImpl::OAuth2AccessTokenFetcherImpl(
Expand Down Expand Up @@ -174,25 +169,46 @@ void OAuth2AccessTokenFetcherImpl::EndGetAccessToken(
CHECK_EQ(GET_ACCESS_TOKEN_STARTED, state_);
state_ = GET_ACCESS_TOKEN_DONE;

bool net_failure = false;
int histogram_value;
if (url_loader_->NetError() == net::OK && url_loader_->ResponseInfo() &&
url_loader_->ResponseInfo()->headers) {
histogram_value = url_loader_->ResponseInfo()->headers->response_code();
} else {
histogram_value = url_loader_->NetError();
net_failure = true;
}
RecordResponseCodeUma(histogram_value);
bool net_failure = url_loader_->NetError() != net::OK ||
!url_loader_->ResponseInfo() ||
!url_loader_->ResponseInfo()->headers;

if (net_failure) {
OnGetTokenFailure(CreateAuthError(histogram_value));
int net_error = url_loader_->NetError();
DLOG(WARNING) << "Could not reach Authorization servers: errno "
<< net_error;
RecordResponseCodeUma(net_error);
OnGetTokenFailure(GoogleServiceAuthError::FromConnectionError(net_error));
return;
}

int response_code = url_loader_->ResponseInfo()->headers->response_code();
RecordResponseCodeUma(response_code);

std::string response_str = response_body ? *response_body : "";
if (response_code == net::HTTP_OK) {
OAuth2AccessTokenConsumer::TokenResponse token_response;
if (ParseGetAccessTokenSuccessResponse(response_str, &token_response)) {
RecordOAuth2Response(OAuth2Response::kOk);
OnGetTokenSuccess(token_response);
} else {
DLOG(WARNING) << "Response doesn't match expected format";
RecordOAuth2Response(OAuth2Response::kOkUnexpectedFormat);
OnGetTokenFailure(
GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE));
}
return;
}

// Request failed
std::string oauth2_error;
ParseGetAccessTokenFailureResponse(response_str, &oauth2_error);
OAuth2Response response = OAuth2ResponseErrorToOAuth2Response(oauth2_error);
RecordOAuth2Response(response);

switch (response_code) {
case net::HTTP_OK:
NOTREACHED();
break;
case net::HTTP_PROXY_AUTHENTICATION_REQUIRED:
NOTREACHED() << "HTTP 407 should be treated as a network error.";
Expand All @@ -210,20 +226,8 @@ void OAuth2AccessTokenFetcherImpl::EndGetAccessToken(
case net::HTTP_BAD_REQUEST: {
// HTTP_BAD_REQUEST (400) usually contains error as per
// http://tools.ietf.org/html/rfc6749#section-5.2.
std::string oauth2_error;
if (!ParseGetAccessTokenFailureResponse(std::move(response_body),
&oauth2_error)) {
OnGetTokenFailure(
GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_ERROR));
return;
}

OAuth2ErrorCodesForHistogram access_error(
OAuth2ErrorToHistogramValue(oauth2_error));
RecordBadRequestTypeUma(access_error);

OnGetTokenFailure(
access_error == OAUTH2_ACCESS_ERROR_INVALID_GRANT
response == kInvalidGrant
? GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
GoogleServiceAuthError::InvalidGaiaCredentialsReason::
CREDENTIALS_REJECTED_BY_SERVER)
Expand All @@ -247,20 +251,6 @@ void OAuth2AccessTokenFetcherImpl::EndGetAccessToken(
return;
}
}

// The request was successfully fetched and it returned OK.
// Parse out the access token and the expiration time.
OAuth2AccessTokenConsumer::TokenResponse token_response;
if (!ParseGetAccessTokenSuccessResponse(std::move(response_body),
&token_response)) {
DLOG(WARNING) << "Response doesn't match expected format";
OnGetTokenFailure(
GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE));
return;
}
// The token will expire in |expires_in| seconds. Take a 10% error margin to
// prevent reusing a token too close to its expiration date.
OnGetTokenSuccess(token_response);
}

void OAuth2AccessTokenFetcherImpl::OnGetTokenSuccess(
Expand Down Expand Up @@ -323,34 +313,52 @@ std::string OAuth2AccessTokenFetcherImpl::MakeGetAccessTokenBody(

// static
bool OAuth2AccessTokenFetcherImpl::ParseGetAccessTokenSuccessResponse(
std::unique_ptr<std::string> response_body,
const std::string& response_body,
OAuth2AccessTokenConsumer::TokenResponse* token_response) {
CHECK(token_response);
std::unique_ptr<base::DictionaryValue> value =
ParseGetAccessTokenResponse(std::move(response_body));
if (!value)
auto value = base::JSONReader::Read(response_body);
if (!value.has_value() || !value->is_dict())
return false;

const base::Value::Dict* dict = value->GetIfDict();
// Refresh and id token are optional and don't cause an error if missing.
value->GetString(krefreshTokenKey, &token_response->refresh_token);
value->GetString(kIdTokenKey, &token_response->id_token);
const std::string* refresh_token = dict->FindString(krefreshTokenKey);
if (refresh_token)
token_response->refresh_token = *refresh_token;

const std::string* id_token = dict->FindString(kIdTokenKey);
if (id_token)
token_response->id_token = *id_token;

const std::string* access_token = dict->FindString(kAccessTokenKey);
if (access_token)
token_response->access_token = *access_token;

int expires_in;
bool ok = value->GetString(kAccessTokenKey, &token_response->access_token) &&
value->GetInteger(kExpiresInKey, &expires_in);
absl::optional<int> expires_in = dict->FindInt(kExpiresInKey);
bool ok = access_token && expires_in.has_value();
if (ok) {
// The token will expire in |expires_in| seconds. Take a 10% error margin to
// prevent reusing a token too close to its expiration date.
token_response->expiration_time =
base::Time::Now() + base::Seconds(9 * expires_in / 10);
base::Time::Now() + base::Seconds(9 * expires_in.value() / 10);
}
return ok;
}

// static
bool OAuth2AccessTokenFetcherImpl::ParseGetAccessTokenFailureResponse(
std::unique_ptr<std::string> response_body,
const std::string& response_body,
std::string* error) {
CHECK(error);
std::unique_ptr<base::DictionaryValue> value =
ParseGetAccessTokenResponse(std::move(response_body));
return value ? value->GetString(kErrorKey, error) : false;
auto value = base::JSONReader::Read(response_body);
if (!value.has_value() || !value->is_dict())
return false;

const base::Value::Dict* dict = value->GetIfDict();
const std::string* error_value = dict->FindString(kErrorKey);
if (!error_value)
return false;

*error = *error_value;
return true;
}
36 changes: 21 additions & 15 deletions google_apis/gaia/oauth2_access_token_fetcher_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,24 @@ class SharedURLLoaderFactory;
// while talking to Google's OAuth servers.
class OAuth2AccessTokenFetcherImpl : public OAuth2AccessTokenFetcher {
public:
// Enumerated constants for logging server responses on 400 errors, matching
// RFC 6749.
enum OAuth2ErrorCodesForHistogram {
OAUTH2_ACCESS_ERROR_INVALID_REQUEST = 0,
OAUTH2_ACCESS_ERROR_INVALID_CLIENT,
OAUTH2_ACCESS_ERROR_INVALID_GRANT,
OAUTH2_ACCESS_ERROR_UNAUTHORIZED_CLIENT,
OAUTH2_ACCESS_ERROR_UNSUPPORTED_GRANT_TYPE,
OAUTH2_ACCESS_ERROR_INVALID_SCOPE,
OAUTH2_ACCESS_ERROR_UNKNOWN,
OAUTH2_ACCESS_ERROR_COUNT,
// Enumerated constants of server responses, matching RFC 6749.
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
enum OAuth2Response {
kUnknownError = 0,
kOk = 1,
kOkUnexpectedFormat = 2,
kErrorUnexpectedFormat = 3,
kInvalidRequest = 4,
kInvalidClient = 5,
kInvalidGrant = 6,
kUnauthorizedClient = 7,
kUnsuportedGrantType = 8,
kInvalidScope = 9,
kRestrictedClient = 10,
kRateLimitExceeded = 11,
kInternalFailure = 12,
kMaxValue = kInternalFailure,
};

OAuth2AccessTokenFetcherImpl(
Expand Down Expand Up @@ -84,8 +91,7 @@ class OAuth2AccessTokenFetcherImpl : public OAuth2AccessTokenFetcher {

// Derived classes override these methods to record UMA histograms if needed.
virtual void RecordResponseCodeUma(int error_value) const {}
virtual void RecordBadRequestTypeUma(
OAuth2ErrorCodesForHistogram access_error) const {}
virtual void RecordOAuth2Response(OAuth2Response response) const {}

// Derived class must specify the GetAccessToken base URL to use.
virtual GURL GetAccessTokenURL() const = 0;
Expand All @@ -112,11 +118,11 @@ class OAuth2AccessTokenFetcherImpl : public OAuth2AccessTokenFetcher {
const std::vector<std::string>& scopes);

static bool ParseGetAccessTokenSuccessResponse(
std::unique_ptr<std::string> response_body,
const std::string& response_body,
OAuth2AccessTokenConsumer::TokenResponse* token_response);

static bool ParseGetAccessTokenFailureResponse(
std::unique_ptr<std::string> response_body,
const std::string& response_body,
std::string* error);

// State that is set during construction.
Expand Down
Loading

0 comments on commit 900a36d

Please sign in to comment.