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

[Identity] Explicitly implement TokenCredential protocol #25788

Closed
wants to merge 5 commits into from

Conversation

mccoyp
Copy link
Member

@mccoyp mccoyp commented Aug 20, 2022

Description

Resolves #25175. Per PEP 544, explicitly declaring protocol implementations is discouraged -- as it shouldn't be necessary -- but allowed. Our credentials are structural subtypes of the TokenCredential protocol (as shown by successful isinstance checks enabled by #25187), but PyCharm's third-party type checker still warns that credentials such as DefaultAzureCredential aren't TokenCredentials. Explicitly subtyping the protocol fixes this.

Technically, PyCharm raises these warnings because its type checker is more aggressive than isinstance; to completely implement the TokenCredential protocol's get_token spec, all credential get_token methods need to include claims and tenant_id in their signatures. This probably isn't the solution we want, as we don't actually support these keyword arguments in all credentials due to environment constraints (for example, we can't specify a tenant_id in managed identity authentication).

To choose which credentials to update, I provided each credential type in azure-identity to a Key Vault client and updated those that raised the warning described in #25175.

This also bumps the minimum version of azure-core to 1.23.1, which was the first version of the package to expose the TokenCredential type outside of TYPE_CHECKING.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@mccoyp mccoyp added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Aug 20, 2022
@mccoyp mccoyp requested a review from lmazuel August 20, 2022 01:12
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-identity

@mccoyp mccoyp force-pushed the id-tokencred-impl branch from f0cc19a to f7e196a Compare August 30, 2022 23:29
@lmazuel
Copy link
Member

lmazuel commented Sep 2, 2022

Should not be needed anymore now that runtimeChecklable has been puclished

@lmazuel lmazuel closed this Sep 2, 2022
@mccoyp mccoyp reopened this Sep 16, 2022
@mccoyp
Copy link
Member Author

mccoyp commented Sep 16, 2022

Reopening -- runtime_checkable allowed us to perform isinstance checks with the protocol, but it didn't solve our PyCharm typing issue. The description for this PR has more details.

@mccoyp
Copy link
Member Author

mccoyp commented Nov 21, 2022

@xiangyan99 what do you think of this approach? Adding claims and tenant_id kwargs to the signatures of all get_token methods doesn't make sense to me, since we don't actually honor those arguments in some credentials. So I think explicitly declaring the implementation is our only real option for resolving PyCharm type warnings

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Jan 27, 2023
@ghost
Copy link

ghost commented Jan 27, 2023

Hi @mccoyp. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@@ -72,7 +72,7 @@
),
python_requires=">=3.7",
install_requires=[
"azure-core<2.0.0,>=1.11.0",
"azure-core<2.0.0,>=1.23.1",
Copy link
Member

Choose a reason for hiding this comment

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

Let's not force users to upgrade azure-core. We can use try import catch instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was motivated by the TokenCredential import, but that may not actually be necessary. I talked this over with Laurent, and he feels that the best solution would be to actually match the method signatures of the protocol; i.e. document kwargs like claims even if they wouldn't have an effect for a particular credential. I think this does make sense, since it makes the capabilities and limitations of each credential more explicit and more correctly implements the protocol.

I want to first check what pyright thinks of this PR's approach. If explicit subtyping without full implementation raises errors with pyright anyway, then this approach won't be viable regardless and would make the choice easier to make (unless you agree that we should go for the full implementation route regardless).

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Feb 1, 2023
@mccoyp
Copy link
Member Author

mccoyp commented Mar 4, 2023

Closing in favor of a new PR that will adjust Identity credential type signatures to match the protocol definition.

@mccoyp mccoyp closed this Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client creation with DefaultAzureCredential raises PyCharm typing warning
4 participants