Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ADFS support for ClientSecretCredential #1947

Merged
merged 24 commits into from
Mar 30, 2021
Merged

Add ADFS support for ClientSecretCredential #1947

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
424a033
Add Azure Stack auth support for ClientSecretCredential
antkmsft Mar 19, 2021
b80639d
Add unit tests
antkmsft Mar 24, 2021
a5b6bf7
Merge branch 'master' into adfs
antkmsft Mar 24, 2021
daa8456
Mac/Linux compilation fix
antkmsft Mar 24, 2021
f252c90
Formatting
antkmsft Mar 24, 2021
5bf72b6
Merge branch 'master' into adfs
antkmsft Mar 24, 2021
42bfcd7
Fixes
antkmsft Mar 24, 2021
fc473a8
Merge branch 'master' into adfs
antkmsft Mar 24, 2021
ebc9b4b
Merge branch 'master' into adfs
antkmsft Mar 24, 2021
33f6028
More tests
antkmsft Mar 25, 2021
e003344
Clang format
antkmsft Mar 25, 2021
b9e6682
Merge remote-tracking branch 'upstream/master' into adfs
antkmsft Mar 26, 2021
6405c09
Remove test hooks
antkmsft Mar 26, 2021
303770b
Remove unneccessary changes
antkmsft Mar 26, 2021
0724222
Merge branch 'master' into adfs
antkmsft Mar 26, 2021
0f2a875
Cleanup
antkmsft Mar 26, 2021
f3476ea
Mac and Linux fixes
antkmsft Mar 26, 2021
18fbf0b
pointer handling
antkmsft Mar 26, 2021
a29e1c3
try putenv on mac/linux
antkmsft Mar 26, 2021
cf281a3
unsetenv()
antkmsft Mar 26, 2021
537549d
_putenv()
antkmsft Mar 26, 2021
d194d8f
#pragme
antkmsft Mar 26, 2021
fb7a27a
)
antkmsft Mar 26, 2021
9bf4acd
Merge branch 'master' into adfs
antkmsft Mar 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 45 additions & 9 deletions sdk/identity/azure-identity/src/client_secret_credential.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,40 @@
#include <chrono>
#include <sstream>

namespace {
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
// Assumes !scopes.empty()
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
std::string FormatScopes(std::vector<std::string> const& scopes, bool asResource)
Copy link
Member

Choose a reason for hiding this comment

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

What does asResource mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is internal function. You can only have single scope as resource, and when you want to build resource, you don't add scope suffix.

{
if (asResource && scopes.size() == 1)
{
auto resource = scopes[0];
constexpr char suffix[] = "/.default";
constexpr int suffixLen = sizeof(suffix) - 1;
Copy link
Member

Choose a reason for hiding this comment

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

Why are we doing -1. Add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this necessarily requires a comment, this is the part of the language - when an array is initialized from a literal, \0 is added to the array - https://en.cppreference.com/w/c/language/array_initialization.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this necessarily requires a comment

Source comments are rarely necessary. It's just meant for helpful hints for future maintainability when code changes, moves around. What happens if I change line 19 tomorrow to std::string, now line 20 is broken, if I don't update -1, correct?

It's up to your discretion, as the PR author, what's clear/obvious by just reading the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add it, but if we change it to std::string, suffixLen should be removed entirely, it is no longer necessary. So, the only context in which it makes sense, is when suffix is a C-string, so it is 0-terminated, so that "- 1" is needed.

But even without the "-1", sizeof(std::string()) is broken. It will be the size of the std::string class, not the length of the string. length() should be used.

auto const resourceLen = resource.length();

// If scopes[0] ends with '/.default', remove it.
if (resourceLen >= suffixLen
&& resource.find(suffix, resourceLen - suffixLen) != std::string::npos)
{
resource = resource.substr(0, resourceLen - suffixLen);
}

return Azure::Core::Url::Encode(resource);
}

auto scopesIter = scopes.begin();
auto scopesStr = Azure::Core::Url::Encode(*scopesIter);
Copy link
Member

Choose a reason for hiding this comment

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

nit: re-add the comment here, that we assume scopes.size() > 0


auto const scopesEnd = scopes.end();
for (++scopesIter; scopesIter != scopesEnd; ++scopesIter)
{
scopesStr += std::string(" ") + Azure::Core::Url::Encode(*scopesIter);
}

return scopesStr;
}
} // namespace

using namespace Azure::Identity;

std::string const Azure::Identity::_detail::g_aadGlobalAuthority
Expand All @@ -27,24 +61,21 @@ Azure::Core::Credentials::AccessToken ClientSecretCredential::GetToken(
static std::string const errorMsgPrefix("ClientSecretCredential::GetToken: ");
try
{
auto const isAdfs = m_tenantId == "adfs";

Url url(m_options.AuthorityHost);
url.AppendPath(m_tenantId);
url.AppendPath("oauth2/v2.0/token");
url.AppendPath(isAdfs ? "oauth2/token" : "oauth2/v2.0/token");

std::ostringstream body;
body << "grant_type=client_credentials&client_id=" << Url::Encode(m_clientId)
<< "&client_secret=" << Url::Encode(m_clientSecret);

auto const& scopes = tokenRequestContext.Scopes;
if (!scopes.empty())
{
auto scopesIter = scopes.begin();
body << "&scope=" << Url::Encode(*scopesIter);

auto const scopesEnd = scopes.end();
for (++scopesIter; scopesIter != scopesEnd; ++scopesIter)
auto const& scopes = tokenRequestContext.Scopes;
antkmsft marked this conversation as resolved.
Show resolved Hide resolved
if (!scopes.empty())
{
body << " " << *scopesIter;
body << "&scope=" << FormatScopes(scopes, isAdfs);
}
}

Expand All @@ -58,6 +89,11 @@ Azure::Core::Credentials::AccessToken ClientSecretCredential::GetToken(
request.SetHeader("Content-Type", "application/x-www-form-urlencoded");
request.SetHeader("Content-Length", std::to_string(bodyString.size()));

if (isAdfs)
{
request.SetHeader("Host", url.GetHost());
}

HttpPipeline httpPipeline(m_options, "Identity-client-secret-credential", "", {}, {});

std::shared_ptr<RawResponse> response = httpPipeline.Send(request, context);
Expand Down