Skip to content

[FEATURE REQ] ProtectKeysWithAzureKeyVault is missing overload accepting Uri, Func<IServiceProvider, TokenCredential> #50062

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

Jump to bottom
Closed
abatishchev opened this issue May 14, 2025 · 5 comments · Fixed by #50103
Closed

[FEATURE REQ] ProtectKeysWithAzureKeyVault is missing overload accepting Uri, Func<IServiceProvider, TokenCredential> #50062

abatishchev opened this issue May 14, 2025 · 5 comments · Fixed by #50103
Assignees
@KrzysztofCwalina
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Extensions ASP.NET Core extensions feature-request This issue requires a new behavior in the product in order be resolved. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team

Comments

@abatishchev
Copy link
Contributor

Library name

Azure.Extensions.AspNetCore.DataProtection.Keys

Please describe the feature.

Currently, there are the following overloads (among a few other)

public static IDataProtectionBuilder ProtectKeysWithAzureKeyVault(
  this IDataProtectionBuilder builder,
  Uri keyIdentifier,
  TokenCredential tokenCredential);

and

public static IDataProtectionBuilder ProtectKeysWithAzureKeyVault(
  this IDataProtectionBuilder builder,
  string keyIdentifier,
  Func<IServiceProvider, TokenCredential> tokenCredentialFactory);

But there is none which would accept:

  • Uri instead of string
  • Func<IServiceProvider, TokenCredential> instead of TokenCredential

Per CA2234, Uri is preferred over string.
@abatishchev
Copy link
Contributor Author

I can submit a PR if it would be accepted.

@github-actions github-actions bot added Client This issue points to a problem in the data-plane of the library. Extensions ASP.NET Core extensions needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels May 14, 2025
@github-actions GitHub Actions
Copy link

Hi @abatishchev, I'm an AI Support assistant here to help with your issue. While the Azure SDK team reviews your request, I wanted to provide some possible tips and documentation that might help you in the meantime.

  • The existing overloads are:
  • A community contribution in version 1.2.0 added the overload using a factory method to create token credentials on-demand. That overload, however, accepts a string for the key identifier.
  • Your request for an overload accepting a Uri alongside a token credential factory has been noted.
  • For more details and context, see the Azure.Extensions.AspNetCore.DataProtection.Keys changelog:
    CHANGELOG.md
  • The team is evaluating feature suggestions in order to align with CA2234 recommendations and overall consistency in the SDK.

The Azure SDK team will respond to your issue shortly. I hope these suggestions are helpful in the meantime. If this comment helped you, please give it a 👍. If the suggestion was not helpful or incorrect, please give it a 👎. Your feedback helps us improve!

@jsquire jsquire assigned KrzysztofCwalina and unassigned jsquire May 14, 2025
@jsquire
Copy link
Member

jsquire commented May 14, 2025

I have no idea why string was used here, and don't believe there's enough value to just add new overloads - but there very well may be value in adding Uri based overloads and hiding the existing string based ones. This falls into architectural governance, so @KrzysztofCwalina and @christothes would be the authoritative voice here.

@jsquire jsquire added customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. labels May 14, 2025
@KrzysztofCwalina
Copy link
Member

We should add the Uri overload. The reason is that if somebody has a Uri, they will have to convert to string and there are many ways to do such conversion, yet we want that to be consistent. I think the reason the existing overload uses string is that we ultimately need a string (in the guts of this library), not a Uri. I also don't think we should hide the existing overload. We don't want users to have to unnecessarily convert to Uri when we immediately convert it back to string

@jsquire
Copy link
Member

jsquire commented May 15, 2025

Thanks, @KrzysztofCwalina. Sounds like a clear path forward for just adding.
@abatishchev: If your offer for a PR stands, we'd appreciate your contribution.

@abatishchev abatishchev mentioned this issue May 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@KrzysztofCwalina KrzysztofCwalina

Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Extensions ASP.NET Core extensions feature-request This issue requires a new behavior in the product in order be resolved. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Added overloads for ProtectKeysWithAzureKeyVault () accepting Uri abatishchev/azure-sdk-for-net
3 participants
@abatishchev @jsquire @KrzysztofCwalina