Anubhav Jindal has posted comments on this change. ( http://gerrit.cloudera.org:8080/24448 )
Change subject: IMPALA-14799: Add oauth_servers support and tests ...................................................................... Patch Set 18: (7 comments) Done http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/jwt-util.cc@1025 PS17, Line 1025: if (!decoded_token->decoded_jwt_.has_key_id()) { > Please add a comment here explaining that if the decoded_jwt_ does not have Done http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/jwt-util.cc@1030 PS17, Line 1030: } > Please add a comment here explaining that the decoded_jwt_ contains a speci Done http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.h@38 PS17, Line 38: const std::string& token, std::string* u > The matched_server_idx_out parameter is only used in test code. While test Done. I removed the matched_server_idx_out parameter from both AuthenticateBearerToken() and Verify() to keep test-only behavior out of production APIs http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc@30 PS17, Line 30: string OAuthServersManager::BearerAuthFailureHeader(const Status& status) { : const string error_message = : status.GetDetail().empty() ? status.msg().msg() : status.GetDetail(); : return Substitute("WWW-Authenticate: Bearer error=\"invalid_token\"," : "error_description=\"$0 \"", error_message); : } > Please add a test to oauth-servers-manager-test.cc for this function. Done. I added OAuthServersManagerTest.BearerAuthFailureHeaderIncludesStatusMessage http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc@81 PS17, Line 81: server_idx).jwt_helper.Verify(decoded_token); : > Since line 84 handles the case where `matched_server_idx == size()`, I thin Done. I updated the loop condition in Verify() to use next_idx < jwt_helpers_->size() http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc@108 PS17, Line 108: > Should be i < jwt_helpers_->size() Done http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc@125 PS17, Line 125: : : > I don't think this condition will ever be true? If it can be true, would a Done. This is an internal-only path, so I replaced the runtime condition with DCHECK_LT(server_idx, jwt_helpers_->size()) and removed the branch. -- To view, visit http://gerrit.cloudera.org:8080/24448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib29ff36600406ba59c10f29d79cc632020f4a3f7 Gerrit-Change-Number: 24448 Gerrit-PatchSet: 18 Gerrit-Owner: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Mon, 10 Aug 2026 22:57:03 +0000 Gerrit-HasComments: Yes
