Thanks Julien for quick implementation. Minor comments.

This looks good overall! I have a number of comments, but they're all fairly minor things, mostly about using C++ style.

Please let me know if you have any questions. Thanks!

Reviewed 10 of 14 files at r1, 1 of 1 files at r3, 2 of 4 files at r4, 1 of 1 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @jboeuf and @jiangtaoli2016)

src/core/lib/security/credentials/credentials.h, line 77 at r6 (raw file):

  "client_id=%s&client_secret=%s&refresh_token=%s&grant_type=refresh_token"

#define GRPC_STS_POST_MINIMAL_BODY_FORMAT_STRING                               \

This seems specific to the STS creds implementation. It might be a good idea to move it to oauth2_credentials.h.

(I realize that there are other macros here that are specific to just one type of credentials. I'd like to see them all moved to their respective implementations at some point. But for now, let's at least stop making the problem worse.)

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 24 at r6 (raw file):

#include <grpc/support/port_platform.h>

#include "grpc/grpc_security.h"

This include should use angle-brackets.

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 153 at r6 (raw file):

    grpc_millis* token_lifetime);

grpc_uri* grpc_validate_sts_credentials_options(

It looks like this is only for internal use, so let's use C++ style (i.e., call it grpc_core::ValidateStsCredentialsOptions()).

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 153 at r6 (raw file):

    grpc_millis* token_lifetime);

grpc_uri* grpc_validate_sts_credentials_options(

Please document this function. It's not obvious what the return value will be.

Also, it looks like this is exposed only for testing purposes, so please document that fact.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 572 at r1 (raw file):

Instead of using goto and logging errors, please consider an alternative pattern here:

• Have the function return a grpc_error* in case of failure, which the caller can log as appropriate.
• Construct an InlinedVector<> of grpc_error* objects, where we add an error to the vector for each problem we detect.
• At the end of the function, if the vector is non-empty, use GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING() to construct a "wrapper" error that refers to the errors in the vector. This is the error that the function will return.

This approach has the added advantage that it allows the caller to see all of the errors at once, rather than having to fix one at a time and iterate.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 21 at r6 (raw file):

#include <grpc/support/port_platform.h>

#include "grpc/grpc_security.h"

These 3 includes should use angle braces instead of quotes.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 25 at r6 (raw file):

#include "grpc/slice.h"
#include "src/core/lib/iomgr/error.h"
#include "src/core/lib/security/credentials/oauth2/oauth2_credentials.h"

This should be the first include after port_platform.h.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 252 at r6 (raw file):

      new_error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
          "Error occurred when fetching oauth2 token.", &error, 1);
      GRPC_ERROR_UNREF(error);

This looks wrong to me. If this function is taking ownership of a ref to error, then it needs to unref unconditionally before returning, not just in this branch. And if it's not taking a ref to error, then it doesn't need to unref at all (which indeed the existing code is not doing).

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 489 at r6 (raw file):

//

namespace {

I suggest putting this anonymous namespace inside of the grpc_core namespace, and using C++ style for all code within it (e.g., MaybeAddToBody() instead of maybe_add_to_body(), StsTokenFetcherCredentials instead of grpc_sts_token_fetcher_credentials, etc).

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 527 at r6 (raw file):

  ~grpc_sts_token_fetcher_credentials() override { grpc_uri_destroy(sts_url_); }

 protected:

Is this expected to have subclasses? If not, why protected instead of private?

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 540 at r6 (raw file):

      return;
    }
    grpc_http_header header = {(char*)"Content-Type",

Please use C++-style const_cast<> for these.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 558 at r6 (raw file):

    grpc_httpcli_post(http_context, pollent, resource_quota, &request, body,
                      body_length, deadline,
                      GRPC_CLOSURE_CREATE(response_cb, metadata_req,

This does a memory allocation that can be avoided. Instead, please declare a grpc_closure data member and use GRPC_CLOSURE_INIT() to initialize it in the ctor, and then use the already-initialized closure here.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 576 at r6 (raw file):

  grpc_core::UniquePtr<char> actor_token_type_;

  grpc_error* fill_body(char** body, size_t* body_length) {

Methods should be listed before data members, as per the style guide.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 605 at r6 (raw file):

    }

  end:

Can we avoid the use of goto here? For example, could we move this code above into a separate helper method that gets called from here, and then change all of the goto end statements to just return from the helper method? We could even make the helper method a lambda.

src/core/lib/security/util/json_util.cc, line 30 at r6 (raw file):

const char* grpc_json_get_string_property(const grpc_json* json,
                                          const char* prop_name,
                                          bool suppress_log) {

It seems kind of messy to have this do the logging in the first place. Instead, how about adding a grpc_error** parameter, which the function can set if it encounters an error? Then the caller can decide whether or not to log it.

test/core/security/credentials_test.cc, line 21 at r6 (raw file):

#include <grpc/support/port_platform.h>

#include "grpc/grpc_security.h"

This include should use angle braces. It should also be moved down next to the other grpc includes (line 28).

Thanks for the thorough review. Just one question on the closure as a member variable inline. I think that it would be better suited for a subsequent PR.

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @jiangtaoli2016 and @markdroth)

src/core/lib/security/credentials/credentials.h, line 77 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 24 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 153 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 153 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 521 at r1 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 538 at r1 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 572 at r1 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 21 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 25 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 252 at r6 (raw file):

This function has the ownership of the error according to the ownership rules described in https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 489 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 527 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 540 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 558 at r6 (raw file):

Not sure it can be done since metadata_req and reponse_cb are not known when this object is constructed. I may be missing something though.
In any case, maybe we can add this optimization on a later PR since all the subclasses of grpc_oauth2_token_fetcher_credentials use the same pattern and I believe that it would be better to have them all changing at the same time if applicable.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 576 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 605 at r6 (raw file):

Good idea. Done.

src/core/lib/security/util/json_util.cc, line 30 at r6 (raw file):

Done.

test/core/security/credentials_test.cc, line 21 at r6 (raw file):

Done.

Just a few minor issues remaining. Please let me know if you have any questions.

Reviewed 7 of 7 files at r7.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @jboeuf and @jiangtaoli2016)

src/core/lib/security/credentials/credentials.h, line 77 at r6 (raw file):

Looks like you copied this into the other file but didn't remove it from here.

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 153 at r6 (raw file):

Even though this is exposed for testing only, I'd still like to see a comment explaining the purpose of the function and its inputs and outputs.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 25 at r6 (raw file):

The include order still looks wrong here. We should be following the order dictated by the style guide, with the exception that port_platform.h should always come first:

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 540 at r6 (raw file):

Looks like this was done above, but not here.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 558 at r6 (raw file):

Good point that we can't initialize the closure earlier. However, I think we can still make it a data member of the class and avoid the allocation, because there will only be a single fetch in flight at any given time.

I'm fine with adding TODOs for the existing code, but I'd prefer to see new code written the right way, so that we're not just adding more technical debt. This also minimizes the chance of propagating the problem further through future cut-and-paste additions.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 519 at r7 (raw file):

}

class grpc_sts_token_fetcher_credentials

This can be called StsTokenFetcherCredentials.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 638 at r7 (raw file):

  };
  *sts_url_out = nullptr;
  grpc_core::InlinedVector<grpc_error*, 3> error_list;

grpc_core:: prefix not needed, since we're already in that namespace.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 639 at r7 (raw file):

  *sts_url_out = nullptr;
  grpc_core::InlinedVector<grpc_error*, 3> error_list;
  grpc_core::UniquePtr<grpc_uri, GrpcUriDeleter> sts_url(

Same here.

test/core/security/credentials_test.cc, line 21 at r6 (raw file):

The header order is still incorrect here.

Thanks and sorry for the sloppiness. I believe that all comments are addressed now. Also, I was wrong on the error ownership issue. I fixed that as well.

Reviewable status: 9 of 13 files reviewed, 11 unresolved discussions (waiting on @jiangtaoli2016 and @markdroth)

src/core/lib/security/credentials/credentials.h, line 77 at r6 (raw file):

Ooops. Sorry about that. Forgot to save the file. Good catch.

src/core/lib/security/credentials/oauth2/oauth2_credentials.h, line 153 at r6 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 25 at r6 (raw file):

Sorry, I'll double-check all these next time. Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 540 at r6 (raw file):

ah. Wrong one sorry. I changed them all while I was at it.
Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 558 at r6 (raw file):

That's true. I checked that the assumption is true and that in the case where the same creds is shared between multiple calls, we don't request another token if a request is already in flight.
I changed all the GRPC_CLOSURE_CREATE while I was at it.
Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 519 at r7 (raw file):

Done. I also changed the helper functions using the C++ style. However, note that this creates some inconsistency with the rest of the code (other subclasses) and the fetch_oauth2 method which is overridden from the base class.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 638 at r7 (raw file):

Done.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 639 at r7 (raw file):

Done.

test/core/security/credentials_test.cc, line 21 at r6 (raw file):

Sorry. Done.

Looks great!

Reviewed 5 of 5 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jiangtaoli2016)

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 519 at r7 (raw file):

Yeah, understood.

@soheilhy, when you converted this code to C++ in #17291, you promised to update the names in subsequent PR. Do you still plan to do that?

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jiangtaoli2016)

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc, line 519 at r7 (raw file):

Yes, I still have plan to do this. Sorry for the delay.