Skip to content

feat: implemented MakeUserAccountCredentials#16090

Open
sachinpro wants to merge 1 commit intogoogleapis:mainfrom
sachinpro:feat-make-user-account-credentials
Open

feat: implemented MakeUserAccountCredentials#16090
sachinpro wants to merge 1 commit intogoogleapis:mainfrom
sachinpro:feat-make-user-account-credentials

Conversation

@sachinpro
Copy link
Copy Markdown
Contributor

@sachinpro sachinpro commented May 6, 2026

The C++ SDK function MakeServiceAccountCredentials throws Invalid ServiceAccountCredentials, the client_email field is missing on data loaded from memory for a json with authorized_user.

The PR implements MakeUserAccountCredentials, a dedicated option for authorized_user credentials.

@sachinpro sachinpro requested a review from a team as a code owner May 6, 2026 14:06
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for user account credentials by adding the MakeUserAccountCredentials factory function and the AuthorizedUserConfig class. The changes include updates to the credential visitor pattern, implementation of REST transport support, and explicit handling for gRPC transport which currently returns an unimplemented error. Feedback was provided regarding a potential issue in unified_rest_credentials.cc where the unspecified order of evaluation for function arguments could result in using a moved-from client_factory_ object.

Comment on lines +165 to +168
result = Decorate(
std::make_shared<oauth2_internal::AuthorizedUserCredentials>(
*info, cfg.options(), client_factory_),
std::move(client_factory_), cfg.options());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The order of evaluation for function arguments is unspecified in C++. In this statement, std::move(client_factory_) might be evaluated before client_factory_ is passed to std::make_shared, which would result in an empty or invalid factory being used to create the credentials. It is safer to create the credentials object in a separate statement to ensure the factory is copied before it is moved into the Decorate function.

      auto creds = std::make_shared<oauth2_internal::AuthorizedUserCredentials>(
          *info, cfg.options(), client_factory_);
      result = Decorate(std::move(creds), std::move(client_factory_),
                        cfg.options());

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.

@sachinpro sachinpro force-pushed the feat-make-user-account-credentials branch from 3e42a37 to 798a51b Compare May 6, 2026 14:12
@sachinpro sachinpro force-pushed the feat-make-user-account-credentials branch from 798a51b to 6f551a7 Compare May 6, 2026 14:20
@scotthart
Copy link
Copy Markdown
Member

/gcbrun

@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 97.36842% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.70%. Comparing base (3852865) to head (6f551a7).

Files with missing lines Patch % Lines
google/cloud/internal/unified_rest_credentials.cc 80.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #16090   +/-   ##
=======================================
  Coverage   92.70%   92.70%           
=======================================
  Files        2353     2353           
  Lines      218352   218428   +76     
=======================================
+ Hits       202425   202497   +72     
- Misses      15927    15931    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants