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.

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

Continue consolidating the auth methods for the hub device #2854

Merged
merged 24 commits into from Oct 20, 2022

Conversation

brycewang-microsoft
Copy link
Contributor

@brycewang-microsoft brycewang-microsoft commented Oct 12, 2022

What's addressed in this PR:

  • consolidate "SasTokenBuilder.cs" and "SharedAccessSignatureBuilder.cs" as they are basically the same thing
  • consolidate "ClientAuthenticationWithTokenRefresh.cs" and "AuthenticationWithTokenRefresh.cs" as both are base classes derived by "ClientAuthenticationWithSakRefresh.cs" and "EdgeModuleAuthenticationWithHsm.cs"
  • rename "ClientAuthenticationWithToken.cs" to "ClientAuthenticationWithSharedAccessSignature.cs"
  • consolidate ClientAuthenticationWithConnectionString.cs, ClientAuthenticationWithRegistrySymmetricKey.cs and ClientAuthenticationWithSharedAccessPolicy.cs into ClientAuthenticationWithSakRefresh.cs
  • update tests

After these changes, the auth methods on the hub device side will be like this.

@brycewang-microsoft
Copy link
Contributor Author

I'd like to hold this for now until we have the v2 sync meeting to decide if anything else we want to consolidate further for the auth methods in the hub device.

Copy link
Member

@abhipsaMisra abhipsaMisra left a comment

Choose a reason for hiding this comment

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

I like the changes that you have here. We'll still want to discuss if we need both ClientAuthenticationWithRegistrySymmetricKey and ClientAuthneticationWithSakRefresh.

@brycewang-microsoft brycewang-microsoft marked this pull request as draft October 18, 2022 01:06
@brycewang-microsoft brycewang-microsoft marked this pull request as ready for review October 19, 2022 00:56
Copy link
Contributor

@drwill-ms drwill-ms left a comment

Choose a reason for hiding this comment

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

Needs a migration guide update

Copy link
Member

@abhipsaMisra abhipsaMisra left a comment

Choose a reason for hiding this comment

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

A couple of small comments

@brycewang-microsoft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@brycewang-microsoft brycewang-microsoft merged commit 27efde5 into previews/v2 Oct 20, 2022
@brycewang-microsoft brycewang-microsoft deleted the brycewang/auth-consolidate branch October 20, 2022 21:43
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.

None yet

3 participants