WIP - Implement clone method rather than relying on Serialization #1469
Conversation
|
The PR looks ok, but I feel that the entire logic of cloning that object is not needed. I've managed to find the original commit that introduced this - AzureAD/microsoft-authentication-library-for-dotnet@95e796b (in fact you can see that they also used a member-by-member clone originally). @MarkZuber - the Clone() is only used in the TokenCache. I only see a string verification and a bunch of unrelated checks. I'm thinking we can remove this Clone logic completely ? Otherwise, Matthew's fix is good. CCing @jennyf19 as she approved the original PR 2 years ago, so she should remember all the details 😼 |
|
@bgavrilMS -- Looking at this. we're this code is returning the AdalResult directly out of the cache, and that object is mutable. So if it's not cloned, and we hand it out to a caller, they could externally modify the cache since it's the same reference. Also, it's modified directly after the clone to clear things out if the token is expired and such. I'm still wondering about the root cause here around how the strings/values in this object matter when all that's being sent into SqlConnection is the AccessToken string, which compares equally. I feel like we're missing something else that's going on. |
|
@MatthewSteeples - is this still actual ? We are releasing a new version of ADAL soon and were wondering whether to include this tactical fix |
|
@bgavrilMS The code as part of this pull request should still be usable, as it doesn't change any behaviour of the library (just changes the method of cloning) so it should be safe to include I do agree with @MarkZuber though, that we're definitely fixing a symptom rather than the underlying cause. I've tried looking at the source code of the framework and stepping through it manually but it all seems to be based on hashcodes (which should be identical for strings with equal contents, even if the strings are referentially different) so I can't determine what is causing this behaviour In summary, I'm happy for (and would appreciate) this change being included |
See #1462 for details