Added ClientAssertionCredential to enable applications to authenticate with custom client assertions.#5789
Conversation
…ate with custom client assertions.
…o ClientAssertionCredential
ahsonkhan
requested review from
a team,
LarryOsterman,
RickWinter,
antkmsft,
gearama and
schaabs
as code owners
ahsonkhan
requested review from
KarishmaGhiya,
billwert,
chlowell,
maorleger,
pvaneck and
xiangyan99
|
|
||
| ClientAssertionCredential::ClientAssertionCredential( | ||
| std::string tenantId, | ||
| std::string clientId, |
There was a problem hiding this comment.
This one can be const ref, and it will not be a breaking change to change it back to by-value, if at some point we will be copying and storing one.
| std::string clientId, | |
| const std::string& clientId, |
There was a problem hiding this comment.
I think it's better not to design (or change) public surface area based on the implementation detail which could change.
I like the pattern for the ctor signatures being consistent with other credential types that we have.
I am not sure I see customer value by having some string params as const& and others as by-value, especially if we change those whenever the underlying implementation changes. What does the end user get out of that decision?
It's simpler to keep all params like these as std::string across all the creds.
| ClientAssertionCredential::ClientAssertionCredential( | ||
| std::string tenantId, | ||
| std::string clientId, | ||
| std::function<std::string(Context const&)> const& assertionCallback, |
There was a problem hiding this comment.
This one is more efficient to take by-value and move:
| std::function<std::string(Context const&)> const& assertionCallback, | |
| std::function<std::string(Context const&)> assertionCallback, |
(part 1/4)
There was a problem hiding this comment.
Can you share more details on this. When is this more efficient vs take by-reference and why does std::function behave better as a move? Also, are there any consequences of doing an std::move (in general)? Why don't we do that everywhere?
|
|
||
| IdentityLog::Write( | ||
| IdentityLog::Level::Warning, | ||
| AuthUnavailable + "See earlier " + GetCredentialName() + " log messages for details."); |
There was a problem hiding this comment.
"See earlier ClientAssertionCredential log messages for details." - that's where that failure reason grouping and using GetCredentialName() at the start of the message will become handy, together with https://aka.ms/azsdk/cpp/identity/troubleshooting link that we include in some of our exceptions, and I think we should include it in the throw exception below too, using the similar patterns to existing credentials, they do this too.
There was a problem hiding this comment.
I don't understand what you mean here. The pattern I used here is consistent with the existing credentials (environment, workload, etc.).
Look at this, as an example. This is almost identical to what we are doing here:
azure-sdk-for-cpp/sdk/identity/azure-identity/src/environment_credential.cpp
Lines 150 to 157 in fb6c039
| { | ||
| IdentityLog::Write( | ||
| IdentityLog::Level::Warning, | ||
| "The assertionCallback must be a valid function that returns assertions for " |
There was a problem hiding this comment.
I think we can be less general, i.e. not "valid function that returns", but "not be null".
There was a problem hiding this comment.
I am not sure I see the value in being that specific. An "empty/no target" std::function would still fail. The invalid case isn't only if null is passed.
antkmsft
left a comment
antkmsft
left a comment
There was a problem hiding this comment.
Sample is missing in sdk/identity/azure-identity/samples. BTW, sample for AzurePipelinesCredential is also missing there.
|
Issue for samples: #5798 Samples will get added later, separately, after identity sample building is re-enabled. They were disabled here: #5754 (comment) |
|
Merging this to unblock the follow-up work I have on top that's required to fix #4905 |
3 participants
Part of #4905