Add ADFS support for ClientSecretCredential #1947

antkmsft · 2021-03-19T14:19:00Z

Closes #345.

RickWinter

This needs test cases added.

sdk/identity/azure-identity/src/client_secret_credential.cpp

sdk/core/azure-core/CMakeLists.txt

sdk/identity/azure-identity/test/ut/client_secret_credential.cpp

sdk/core/azure-core/inc/azure/core/internal/system_clock.hpp

ghost · 2021-03-26T22:32:44Z

Hello @antkmsft!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

@RickWinter

I implemented all of the @RickWinter's requests, and I haven't heard from him since then. If there's anything more, let me know.

sdk/core/azure-core/src/datetime.cpp

sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp

ahsonkhan · 2021-03-30T00:26:56Z

sdk/core/azure-core/inc/azure/core/credentials/token_credential_options.hpp

+  /**
+   * @brief Defines options for #Azure::Core::Credentials::TokenCredential.
+   */
+  struct TokenCredentialOptions : public Azure::Core::_internal::ClientOptions


Needs CL entry.

It is not a breaking change, and has the default value. All the code you could compile before can still compile now.

I understand. It's a new type/new feature (which are also added to CL) :)

ahsonkhan · 2021-03-30T00:27:35Z

sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp

        : m_tenantId(std::move(tenantId)), m_clientId(std::move(clientId)),
          m_clientSecret(std::move(clientSecret)), m_options(std::move(options))
    {
    }

+    /**
+     * @brief Construct a Client Secret credential.


This is an odd casing choice. Was it intentional?

ahsonkhan · 2021-03-30T00:28:47Z

sdk/identity/azure-identity/inc/azure/identity/client_secret_credential.hpp

+        : m_tenantId(std::move(tenantId)), m_clientId(std::move(clientId)),
+          m_clientSecret(std::move(clientSecret))
+    {
+      static_cast<Azure::Core::Credentials::TokenCredentialOptions&>(m_options) = options;


nit: Add a comment explaining why this cast is OK.

ahsonkhan · 2021-03-30T00:29:25Z

sdk/identity/azure-identity/src/client_secret_credential.cpp

@@ -9,6 +9,40 @@
 #include <chrono>
 #include <sstream>

+namespace {
+// Assumes !scopes.empty()
+std::string FormatScopes(std::vector<std::string> const& scopes, bool asResource)


What does asResource mean?

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.

ahsonkhan · 2021-03-30T00:29:57Z

sdk/identity/azure-identity/src/client_secret_credential.cpp

+  {
+    auto resource = scopes[0];
+    constexpr char suffix[] = "/.default";
+    constexpr int suffixLen = sizeof(suffix) - 1;


Why are we doing -1. Add a comment.

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.

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.

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.

ahsonkhan · 2021-03-30T00:31:41Z

sdk/identity/azure-identity/src/client_secret_credential.cpp

+  }
+
+  auto scopesIter = scopes.begin();
+  auto scopesStr = Azure::Core::Url::Encode(*scopesIter);


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

ahsonkhan · 2021-03-30T00:33:34Z

sdk/identity/azure-identity/src/environment_credential.cpp

-        ClientSecretCredentialOptions options;
-        options.AuthorityHost = authority;
+        ClientSecretCredentialOptions clientSecretCredentialOptions;
+        static_cast<Core::_internal::ClientOptions&>(clientSecretCredentialOptions) = options;


Shouldn't this be TokenCredentialOptions?

Suggested change

static_cast<Core::_internal::ClientOptions&>(clientSecretCredentialOptions) = options;

static_cast<Core::Credentials::TokenCredentialOptions&>(clientSecretCredentialOptions) = options;

What's this line doing? Is it only copying over the fields into the local clientSecretCredentialOptions that come from the base type options? Does it leave the other fields alone?

Also, add comment why static cast is OK here.

@antkmsft does this need to be fixed for correctness?

For now we are fine, but yes, it needs fixing - I'll grab this plus a few other comments and create a follow-up PR.

ahsonkhan · 2021-03-30T00:35:05Z

sdk/identity/azure-identity/src/environment_credential.cpp

      }
      else
      {
-        m_credentialImpl.reset(new ClientSecretCredential(tenantId, clientId, clientSecret));
+        m_credentialImpl.reset(
+            new ClientSecretCredential(tenantId, clientId, clientSecret, options));
      }
    }
    // TODO: These credential types are not implemented. Uncomment when implemented.


Since you are hereand modifying this code already, remove this TODO comment from source and file an issue.

ahsonkhan · 2021-03-30T00:38:14Z

sdk/identity/azure-identity/test/ut/client_secret_credential.cpp

+}
+} // namespace
+
+TEST(ClientSecretCredential, Regular)


How can I see the test coverage for this change and azure-identity overall?

It's not visible here: https://dev.azure.com/azure-sdk/public/_build/results?buildId=808213&view=results

I don't know how the code coverage integration works, but if you don't see it, it most likely was not enable for identity. We need a work item to enable it.

sdk/identity/azure-identity/test/ut/client_secret_credential.cpp

sdk/identity/azure-identity/test/ut/environment_credential.cpp

ahsonkhan · 2021-04-07T21:04:41Z

sdk/core/azure-core/CMakeLists.txt

@@ -46,6 +46,7 @@ set(
    ${CURL_TRANSPORT_ADAPTER_INC}
    ${WIN_TRANSPORT_ADAPTER_INC}
    inc/azure/core/credentials/credentials.hpp
+    inc/azure/core/credentials/token_credential_options.hpp


We need to include these headers to core.hpp and add tests to the simplified header.
Fixed in #2070

Add Azure Stack auth support for ClientSecretCredential

424a033

antkmsft added the Azure.Identity label Mar 19, 2021

antkmsft added this to the [2021] April milestone Mar 19, 2021

antkmsft self-assigned this Mar 19, 2021

antkmsft requested review from ahsonkhan, RickWinter, schaabs and vhvb1989 as code owners March 19, 2021 14:19

RickWinter previously requested changes Mar 19, 2021

View reviewed changes

sdk/identity/azure-identity/src/client_secret_credential.cpp Show resolved Hide resolved

sdk/identity/azure-identity/src/client_secret_credential.cpp Show resolved Hide resolved

schaabs approved these changes Mar 19, 2021

View reviewed changes

bhardwajs reviewed Mar 20, 2021

View reviewed changes

sdk/identity/azure-identity/src/client_secret_credential.cpp Show resolved Hide resolved

antkmsft and others added 3 commits March 23, 2021 17:02

Add unit tests

b80639d

Merge branch 'master' into adfs

a5b6bf7

Mac/Linux compilation fix

daa8456

antkmsft requested a review from RickWinter March 24, 2021 00:18

antkmsft and others added 4 commits March 23, 2021 17:25

Formatting

f252c90

Merge branch 'master' into adfs

5bf72b6

Fixes

42bfcd7

Merge branch 'master' into adfs

fc473a8

RickWinter reviewed Mar 24, 2021

View reviewed changes

sdk/core/azure-core/CMakeLists.txt Outdated Show resolved Hide resolved

sdk/identity/azure-identity/test/ut/client_secret_credential.cpp Outdated Show resolved Hide resolved

sdk/core/azure-core/inc/azure/core/internal/system_clock.hpp Outdated Show resolved Hide resolved

antkmsft and others added 9 commits March 24, 2021 21:53

Merge branch 'master' into adfs

ebc9b4b

More tests

33f6028

Clang format

e003344

Merge remote-tracking branch 'upstream/master' into adfs

b9e6682

Remove test hooks

6405c09

Remove unneccessary changes

303770b

Merge branch 'master' into adfs

0724222

Cleanup

0f2a875

Mac and Linux fixes

f3476ea

antkmsft requested a review from RickWinter March 26, 2021 20:41

antkmsft added 6 commits March 26, 2021 13:56

pointer handling

18fbf0b

try putenv on mac/linux

a29e1c3

unsetenv()

cf281a3

_putenv()

537549d

#pragme

d194d8f

)

fb7a27a

antkmsft added the auto-merge label Mar 26, 2021

Merge branch 'master' into adfs

9bf4acd

antkmsft merged commit b606ff6 into Azure:master Mar 30, 2021

ahsonkhan reviewed Mar 30, 2021

View reviewed changes

ahsonkhan mentioned this pull request Apr 7, 2021

Update the Azure.Core changelog to be more consise and complete for the beta.8 #2069

Merged

ahsonkhan reviewed Apr 7, 2021

View reviewed changes

antkmsft deleted the adfs branch June 9, 2021 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ADFS support for ClientSecretCredential #1947

Add ADFS support for ClientSecretCredential #1947

antkmsft commented Mar 19, 2021 •

edited

Loading

RickWinter left a comment

ghost commented Mar 26, 2021

ahsonkhan Mar 30, 2021

antkmsft Mar 30, 2021

ahsonkhan Mar 30, 2021 •

edited

Loading

ahsonkhan Mar 30, 2021

ahsonkhan Mar 30, 2021

ahsonkhan Mar 30, 2021

antkmsft Mar 30, 2021

ahsonkhan Mar 30, 2021

antkmsft Mar 30, 2021

ahsonkhan Mar 30, 2021

antkmsft Mar 31, 2021

ahsonkhan Mar 30, 2021

ahsonkhan Mar 30, 2021

ahsonkhan Mar 30, 2021 •

edited

Loading

antkmsft Mar 31, 2021

ahsonkhan Mar 30, 2021 •

edited

Loading

ahsonkhan Mar 30, 2021

antkmsft Mar 31, 2021

ahsonkhan Apr 7, 2021

	static_cast<Core::_internal::ClientOptions&>(clientSecretCredentialOptions) = options;
	static_cast<Core::Credentials::TokenCredentialOptions&>(clientSecretCredentialOptions) = options;

Add ADFS support for ClientSecretCredential #1947

Add ADFS support for ClientSecretCredential #1947

Conversation

antkmsft commented Mar 19, 2021 • edited Loading

RickWinter left a comment

Choose a reason for hiding this comment

ghost commented Mar 26, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antkmsft commented Mar 19, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ahsonkhan Mar 30, 2021 •

edited

Loading

ahsonkhan Mar 30, 2021 •

edited

Loading

ahsonkhan Mar 30, 2021 •

edited

Loading