CSHARP-4610: OIDC: Automatic token acquisition for GCP Identity Provider #1315
Conversation
sanych-sun
requested review from
BorisDog and
JamesKovacs
and removed request for
a team
| @@ -980,6 +980,12 @@ string ProtectConnectionString(string connectionString) | |||
|
|
|||
| private void ParseOption(string name, string value) | |||
| { | |||
| // Should not decode authmechanismproperties before splitting it by separators (, and :). | |||
| if (!string.Equals(name, "authmechanismproperties", StringComparison.OrdinalIgnoreCase)) | |||
There was a problem hiding this comment.
This change was introduced because of spec change that is not merged yet. The value of TOKEN_RESOURCE for GCP could contain special symbols, so we have to split the properties by separators before decoding the string.
| @@ -1208,12 +1214,16 @@ private void ParseOption(string name, string value) | |||
| { | |||
| foreach (var property in value.Split(',')) | |||
| { | |||
| var parts = property.Split(':'); | |||
| if (parts.Length != 2) | |||
| var separatorPosition = property.IndexOf(':'); | |||
There was a problem hiding this comment.
Minor optimization: we know for sure it should be no more then 2 parts, so can avoid array allocation.
sanych-sun
changed the title
|
@sanych-sun you have a merge conflict. 😄 |
| { | ||
| var metadataRequestInfo = GetMetadataRequestInfo(parameters); | ||
| var request = WebRequest.CreateHttp(metadataRequestInfo.Uri); | ||
| request.Accept = "application/json"; |
There was a problem hiding this comment.
Note: the GCP response is not json, it is the token contents.
There was a problem hiding this comment.
Yep. There is a code to read the entire response body as access_token. I've refactored the code to remove this unnecessary setting of Accept header for GCP provider.
| @@ -980,6 +980,12 @@ string ProtectConnectionString(string connectionString) | |||
|
|
|||
| private void ParseOption(string name, string value) | |||
| { | |||
| // Should not decode authmechanismproperties before splitting it by separators (, and :). | |||
| if (!string.Equals(name, "authmechanismproperties", StringComparison.OrdinalIgnoreCase)) | |||
src/MongoDB.Driver.Core/Core/Authentication/Oidc/HttpRequestOidcCallback.cs
Show resolved
Hide resolved
| @@ -33,50 +31,20 @@ public AzureOidcCallback(string tokenResource) | |||
| _tokenResource = tokenResource; | |||
| } | |||
|
|
|||
| public OidcAccessToken GetOidcAccessToken(OidcCallbackParameters parameters, CancellationToken cancellationToken) | |||
| protected override (Uri Uri, IReadOnlyDictionary<string, string> headers) GetMetadataRequestInfo(OidcCallbackParameters parameters) | |||
There was a problem hiding this comment.
KeyValuePair[]or just (string, string)[] is probably sufficient.
| } | ||
|
|
||
| protected abstract (Uri Uri, IReadOnlyDictionary<string, string> headers) GetMetadataRequestInfo(OidcCallbackParameters parameters); | ||
| protected abstract OidcAccessToken ParseMetadataResponseContent(Stream responseStream); |
There was a problem hiding this comment.
minor: we are requesting the token itself, not only metadata.
Following the naming of HttpRequestOidcCallback, maybe
GetHttpRequestParams and
ProcessHttpResponse
?
| var request = CreateMetadataRequest(parameters); | ||
| using (cancellationToken.Register(() => request.Abort(), useSynchronizationContext: false)) | ||
| { | ||
| var response = request.GetResponse(); |
There was a problem hiding this comment.
We need to dispose of this response object.
There was a problem hiding this comment.
Good catch! Thank you!
No description provided.