From f6b38f75a4c715598eb8a8ff28c67e70e4d7bc43 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Mon, 24 Feb 2020 09:56:52 -0800 Subject: [PATCH] Revert "feat: add x-goog-user-project header from quota_project_id field" --- .../lib/security/credentials/credentials.h | 1 - .../credentials/oauth2/oauth2_credentials.cc | 19 --------- .../credentials/oauth2/oauth2_credentials.h | 10 +---- .../security/transport/client_auth_filter.cc | 14 ++----- src/core/lib/transport/metadata_batch.cc | 13 ------ src/core/lib/transport/metadata_batch.h | 11 ----- test/core/security/credentials_test.cc | 40 +------------------ test/core/security/json_token_test.cc | 31 -------------- 8 files changed, 6 insertions(+), 133 deletions(-) diff --git a/src/core/lib/security/credentials/credentials.h b/src/core/lib/security/credentials/credentials.h index e68462d077f..65996fd09e4 100644 --- a/src/core/lib/security/credentials/credentials.h +++ b/src/core/lib/security/credentials/credentials.h @@ -56,7 +56,6 @@ typedef enum { #define GRPC_CALL_CREDENTIALS_TYPE_COMPOSITE "Composite" #define GRPC_AUTHORIZATION_METADATA_KEY "authorization" -#define GRPC_AUTH_QUOTA_PROJECT_METADATA_KEY "x-goog-user-project" #define GRPC_IAM_AUTHORIZATION_TOKEN_METADATA_KEY \ "x-goog-iam-authorization-token" #define GRPC_IAM_AUTHORITY_SELECTOR_METADATA_KEY "x-goog-iam-authority-selector" diff --git a/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc b/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc index 67b4bf57b2c..3a6d19c11ec 100644 --- a/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc +++ b/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc @@ -74,9 +74,6 @@ grpc_auth_refresh_token grpc_auth_refresh_token_create_from_json( } result.type = GRPC_AUTH_JSON_TYPE_AUTHORIZED_USER; - // quota_project_id is optional, so we don't check the result of the copy. - grpc_copy_json_string_property(json, "quota_project_id", - &result.quota_project_id); if (!grpc_copy_json_string_property(json, "client_secret", &result.client_secret) || !grpc_copy_json_string_property(json, "client_id", &result.client_id) || @@ -117,10 +114,6 @@ void grpc_auth_refresh_token_destruct(grpc_auth_refresh_token* refresh_token) { gpr_free(refresh_token->refresh_token); refresh_token->refresh_token = nullptr; } - if (refresh_token->quota_project_id != nullptr) { - gpr_free(refresh_token->quota_project_id); - refresh_token->quota_project_id = nullptr; - } } // @@ -283,7 +276,6 @@ bool grpc_oauth2_token_fetcher_credentials::get_request_metadata( grpc_polling_entity* pollent, grpc_auth_metadata_context /*context*/, grpc_credentials_mdelem_array* md_array, grpc_closure* on_request_metadata, grpc_error** /*error*/) { - maybe_add_additional_metadata(md_array); // Check if we can use the cached token. grpc_millis refresh_threshold = GRPC_SECURE_TOKEN_REFRESH_THRESHOLD_SECS * GPR_MS_PER_SEC; @@ -461,17 +453,6 @@ void grpc_google_refresh_token_credentials::fetch_oauth2( gpr_free(body); } -void grpc_google_refresh_token_credentials::maybe_add_additional_metadata( - grpc_credentials_mdelem_array* md_array) { - if (refresh_token_.quota_project_id != nullptr) { - grpc_mdelem quota_project_md = grpc_mdelem_from_slices( - grpc_core::ExternallyManagedSlice(GRPC_AUTH_QUOTA_PROJECT_METADATA_KEY), - grpc_core::ExternallyManagedSlice(refresh_token_.quota_project_id)); - grpc_credentials_mdelem_array_add(md_array, quota_project_md); - GRPC_MDELEM_UNREF(quota_project_md); - } -} - grpc_google_refresh_token_credentials::grpc_google_refresh_token_credentials( grpc_auth_refresh_token refresh_token) : refresh_token_(refresh_token) {} diff --git a/src/core/lib/security/credentials/oauth2/oauth2_credentials.h b/src/core/lib/security/credentials/oauth2/oauth2_credentials.h index eea7edadc08..97579522950 100644 --- a/src/core/lib/security/credentials/oauth2/oauth2_credentials.h +++ b/src/core/lib/security/credentials/oauth2/oauth2_credentials.h @@ -32,13 +32,12 @@ "s&subject_token_type=%s" // auth_refresh_token parsing. -struct grpc_auth_refresh_token { +typedef struct { const char* type; char* client_id; char* client_secret; char* refresh_token; - char* quota_project_id = nullptr; -}; +} grpc_auth_refresh_token; /// Returns 1 if the object is valid, 0 otherwise. int grpc_auth_refresh_token_is_valid( @@ -91,9 +90,6 @@ class grpc_oauth2_token_fetcher_credentials : public grpc_call_credentials { grpc_httpcli_context* httpcli_context, grpc_polling_entity* pollent, grpc_iomgr_cb_func cb, grpc_millis deadline) = 0; - // Sub class may override this for adding additional metadata other than - // credentials itself. - virtual void maybe_add_additional_metadata(grpc_credentials_mdelem_array*) {} private: gpr_mu mu_; @@ -121,8 +117,6 @@ class grpc_google_refresh_token_credentials final grpc_httpcli_context* httpcli_context, grpc_polling_entity* pollent, grpc_iomgr_cb_func cb, grpc_millis deadline) override; - void maybe_add_additional_metadata( - grpc_credentials_mdelem_array* md_array) override; private: grpc_auth_refresh_token refresh_token_; diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc index 6ed363ba8fe..c3658a99d2e 100644 --- a/src/core/lib/security/transport/client_auth_filter.cc +++ b/src/core/lib/security/transport/client_auth_filter.cc @@ -165,17 +165,9 @@ static void on_credentials_metadata(void* arg, grpc_error* input_error) { grpc_metadata_batch* mdb = batch->payload->send_initial_metadata.send_initial_metadata; for (size_t i = 0; i < calld->md_array.size; ++i) { - // Only add x-goog-user-project header if not present. - if (grpc_slice_str_cmp(GRPC_MDKEY(calld->md_array.md[i]), - GRPC_AUTH_QUOTA_PROJECT_METADATA_KEY) == 0) { - add_error(&error, grpc_metadata_batch_add_tail_when_key_not_exist( - mdb, &calld->md_links[i], - GRPC_MDELEM_REF(calld->md_array.md[i]))); - } else { - add_error(&error, grpc_metadata_batch_add_tail( - mdb, &calld->md_links[i], - GRPC_MDELEM_REF(calld->md_array.md[i]))); - } + add_error(&error, grpc_metadata_batch_add_tail( + mdb, &calld->md_links[i], + GRPC_MDELEM_REF(calld->md_array.md[i]))); } } if (error == GRPC_ERROR_NONE) { diff --git a/src/core/lib/transport/metadata_batch.cc b/src/core/lib/transport/metadata_batch.cc index fc7c382e916..66749b671c7 100644 --- a/src/core/lib/transport/metadata_batch.cc +++ b/src/core/lib/transport/metadata_batch.cc @@ -205,19 +205,6 @@ grpc_error* grpc_metadata_batch_add_tail(grpc_metadata_batch* batch, return grpc_metadata_batch_link_tail(batch, storage); } -grpc_error* grpc_metadata_batch_add_tail_when_key_not_exist( - grpc_metadata_batch* batch, grpc_linked_mdelem* storage, - grpc_mdelem elem_to_add) { - auto cur = batch->list.head; - while (cur != nullptr) { - if (grpc_slice_cmp(GRPC_MDKEY(cur->md), GRPC_MDKEY(elem_to_add)) == 0) { - // We already have the same key, just returning. - return GRPC_ERROR_NONE; - } - } - return grpc_metadata_batch_add_tail(batch, storage, elem_to_add); -} - static void link_tail(grpc_mdelem_list* list, grpc_linked_mdelem* storage) { assert_valid_list(list); GPR_DEBUG_ASSERT(!GRPC_MDISNULL(storage->md)); diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h index 4ef4654d141..46a437e4f1b 100644 --- a/src/core/lib/transport/metadata_batch.h +++ b/src/core/lib/transport/metadata_batch.h @@ -138,17 +138,6 @@ grpc_error* grpc_metadata_batch_add_tail( grpc_metadata_batch* batch, grpc_linked_mdelem* storage, grpc_mdelem elem_to_add) GRPC_MUST_USE_RESULT; -/** Add \a elem_to_add as the last element in \a batch, only - when the current batch doesn't have the same key in the - given element, using \a storage as backing storage for the - linked list element. \a storage is owned by the caller - and must survive for the lifetime of batch. This usually - means it should be around for the lifetime of the call. - Takes ownership of \a elem_to_add */ -grpc_error* grpc_metadata_batch_add_tail_when_key_not_exist( - grpc_metadata_batch* batch, grpc_linked_mdelem* storage, - grpc_mdelem elem_to_add) GRPC_MUST_USE_RESULT; - inline grpc_error* GRPC_MUST_USE_RESULT grpc_metadata_batch_add_tail( grpc_metadata_batch* batch, grpc_linked_mdelem* storage, grpc_metadata_batch_callouts_index idx) { diff --git a/test/core/security/credentials_test.cc b/test/core/security/credentials_test.cc index f41e818ff92..b58973d4c34 100644 --- a/test/core/security/credentials_test.cc +++ b/test/core/security/credentials_test.cc @@ -97,13 +97,6 @@ static const char test_refresh_token_str[] = " \"refresh_token\": \"1/Blahblasj424jladJDSGNf-u4Sua3HDA2ngjd42\"," " \"type\": \"authorized_user\"}"; -static const char test_refresh_token_with_quota_project_id_str[] = - "{ \"client_id\": \"32555999999.apps.googleusercontent.com\"," - " \"client_secret\": \"EmssLNjJy1332hD4KFsecret\"," - " \"refresh_token\": \"1/Blahblasj424jladJDSGNf-u4Sua3HDA2ngjd42\"," - " \"quota_project_id\": \"my-quota-project-id\"," - " \"type\": \"authorized_user\"}"; - static const char valid_oauth2_json_response[] = "{\"access_token\":\"ya29.AHES6ZRN3-HlhAPya30GnW_bHSb_\"," " \"expires_in\":3599, " @@ -720,37 +713,7 @@ static void test_refresh_token_creds_success(void) { /* Check security level. */ GPR_ASSERT(creds->min_security_level() == GRPC_PRIVACY_AND_INTEGRITY); - /* First request: http post should be called. */ - request_metadata_state* state = - make_request_metadata_state(GRPC_ERROR_NONE, emd, GPR_ARRAY_SIZE(emd)); - grpc_httpcli_set_override(httpcli_get_should_not_be_called, - refresh_token_httpcli_post_success); - run_request_metadata_test(creds, auth_md_ctx, state); - grpc_core::ExecCtx::Get()->Flush(); - - /* Second request: the cached token should be served directly. */ - state = - make_request_metadata_state(GRPC_ERROR_NONE, emd, GPR_ARRAY_SIZE(emd)); - grpc_httpcli_set_override(httpcli_get_should_not_be_called, - httpcli_post_should_not_be_called); - run_request_metadata_test(creds, auth_md_ctx, state); - grpc_core::ExecCtx::Get()->Flush(); - - creds->Unref(); - grpc_httpcli_set_override(nullptr, nullptr); -} - -static void test_refresh_token_with_quota_project_id_creds_success(void) { - grpc_core::ExecCtx exec_ctx; - expected_md emd[] = { - {"authorization", "Bearer ya29.AHES6ZRN3-HlhAPya30GnW_bHSb_"}, - {"x-goog-user-project", "my-quota-project-id"}}; - grpc_auth_metadata_context auth_md_ctx = {test_service_url, test_method, - nullptr, nullptr}; - grpc_call_credentials* creds = grpc_google_refresh_token_credentials_create( - test_refresh_token_with_quota_project_id_str, nullptr); - - /* First request: http post should be called. */ + /* First request: http put should be called. */ request_metadata_state* state = make_request_metadata_state(GRPC_ERROR_NONE, emd, GPR_ARRAY_SIZE(emd)); grpc_httpcli_set_override(httpcli_get_should_not_be_called, @@ -1752,7 +1715,6 @@ int main(int argc, char** argv) { test_compute_engine_creds_success(); test_compute_engine_creds_failure(); test_refresh_token_creds_success(); - test_refresh_token_with_quota_project_id_creds_success(); test_refresh_token_creds_failure(); test_valid_sts_creds_options(); test_invalid_sts_creds_options(); diff --git a/test/core/security/json_token_test.cc b/test/core/security/json_token_test.cc index f059a57d4ef..d93be88aba5 100644 --- a/test/core/security/json_token_test.cc +++ b/test/core/security/json_token_test.cc @@ -75,13 +75,6 @@ static const char test_refresh_token_str[] = " \"refresh_token\": \"1/Blahblasj424jladJDSGNf-u4Sua3HDA2ngjd42\"," " \"type\": \"authorized_user\"}"; -static const char test_refresh_token_with_quota_project_id_str[] = - "{ \"client_id\": \"32555999999.apps.googleusercontent.com\"," - " \"client_secret\": \"EmssLNjJy1332hD4KFsecret\"," - " \"refresh_token\": \"1/Blahblasj424jladJDSGNf-u4Sua3HDA2ngjd42\"," - " \"quota_project_id\": \"my-quota-project-id\"," - " \"type\": \"authorized_user\"}"; - static const char test_scope[] = "myperm1 myperm2"; static const char test_service_url[] = "https://foo.com/foo.v1"; @@ -396,29 +389,6 @@ static void test_parse_refresh_token_success(void) { GPR_ASSERT(refresh_token.refresh_token != nullptr && (strcmp(refresh_token.refresh_token, "1/Blahblasj424jladJDSGNf-u4Sua3HDA2ngjd42") == 0)); - GPR_ASSERT(refresh_token.quota_project_id == nullptr); - grpc_auth_refresh_token_destruct(&refresh_token); -} - -static void test_parse_refresh_token_with_quota_project_id_success(void) { - grpc_auth_refresh_token refresh_token = - grpc_auth_refresh_token_create_from_string( - test_refresh_token_with_quota_project_id_str); - GPR_ASSERT(grpc_auth_refresh_token_is_valid(&refresh_token)); - GPR_ASSERT(refresh_token.type != nullptr && - (strcmp(refresh_token.type, "authorized_user") == 0)); - GPR_ASSERT(refresh_token.client_id != nullptr && - (strcmp(refresh_token.client_id, - "32555999999.apps.googleusercontent.com") == 0)); - GPR_ASSERT( - refresh_token.client_secret != nullptr && - (strcmp(refresh_token.client_secret, "EmssLNjJy1332hD4KFsecret") == 0)); - GPR_ASSERT(refresh_token.refresh_token != nullptr && - (strcmp(refresh_token.refresh_token, - "1/Blahblasj424jladJDSGNf-u4Sua3HDA2ngjd42") == 0)); - GPR_ASSERT( - refresh_token.quota_project_id != nullptr && - (strcmp(refresh_token.quota_project_id, "my-quota-project-id") == 0)); grpc_auth_refresh_token_destruct(&refresh_token); } @@ -475,7 +445,6 @@ int main(int argc, char** argv) { test_service_account_creds_jwt_encode_and_sign(); test_jwt_creds_jwt_encode_and_sign(); test_parse_refresh_token_success(); - test_parse_refresh_token_with_quota_project_id_success(); test_parse_refresh_token_failure_no_type(); test_parse_refresh_token_failure_no_client_id(); test_parse_refresh_token_failure_no_client_secret();