Allow TokenCredentials to be passed in Config #103
Conversation
| @@ -28,6 +28,12 @@ protected virtual void Initialize(IdentityConfiguration config) | |||
| _config = config; | |||
| _client = new TableServiceClient(_config.StorageConnectionString); | |||
|
|
|||
| if (config.TokenCredential != null) | |||
There was a problem hiding this comment.
Probably need to check for the _config.TokenCredential first since the line 29 will likely throw an exception in the _config.StorageConnectionString isNullOrEmpty. Maybe even throw an argument exception if both _config.TokenCredential and _config.StorageConnectionString are both null.
There was a problem hiding this comment.
You still need to provide some form of connection string (but the Access Key is ignored) so I've left that bit as is.
The alternative is that instead of a connection string you would need to collect the URL of the endpoint
Happy to put in an ArgumentException for StorageConnectionString, but as there wasn't one already I left it as is
Ignore me, helps if I read my own code before posting! I'll make some changes
There was a problem hiding this comment.
Btw - thanks for the feature. This will be a nice option to be able to support managed identities.
There was a problem hiding this comment.
Sorry for the delay in coming back to this. The way I'd written it (and this may not make much sense, but it's the way that we've been using these objects) is a bit unexpected:
When you're creating a TableServiceClient with a TokenCredential, you need the full URL of the table service that you're connecting to. Instead of creating a new variable for that, I was re-using the value from within the connection string. This means that you would need to pass both a StorageConnectionString and a TokenCredential in to use managed identities, and the Key part of the connection string would be ignore.
That's why I create one instance of the client first (because there's no other way to parse the connection string) and then obtain the URL from that.
MatthewSteeples
force-pushed
the
master
branch
from
7c4cd4b
to
9d98fcd
Compare
|
Pushed a change today for this that updates the version of Azure Data Tables in use. This is so that the |
|
OK, better late than never, I think this is ready to go now |
|
We've also tested cherry-picking this branch on to 6.2 (as that's the version that we're on) if you wouldn't mind backporting it. Happy to open up a separate pull request for that |
MatthewSteeples
force-pushed
the
master
branch
from
aa5c696
to
c81af12
Compare
… instead of Shared Access Keys)
MatthewSteeples
force-pushed
the
master
branch
from
c81af12
to
f640788
Compare
This change will allow a TokenCredential to be passed in to the configuration of the table client. This will allow Managed Identities to be used instead of Shared Access Keys.
Please note that currently this change will only work on "public" Azure Clouds (where the subdomain of the table service is
table.core.windows.net)Happy to make changes to this as required