-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add VisualStudioCodeCredential #10979
Conversation
- Remove unused code from VisualStudioCodeCredential
sdk/identity/Azure.Identity/src/LinuxVisualStudioCodeAdapter.cs
Outdated
Show resolved
Hide resolved
{ | ||
internal static class StaticCachesUtilities | ||
{ | ||
private static readonly Lazy<Action> s_clearStaticMetadataProvider = new Lazy<Action>(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is compiling to expressions here worth it?
public void Dispose() => WindowsNativeMethods.CredDelete(_target, WindowsNativeMethods.CRED_TYPE.GENERIC); | ||
} | ||
|
||
private sealed class OsxRefreshTokenFixture : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
public VisualStudioCodeCredentialLiveTests(bool isAsync) : base(isAsync) | ||
{ | ||
Matcher.ExcludeHeaders.Add("Content-Length"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For curiosity sake why are we excluding content length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IdentityRecordedTestSanitizer
changes request body, so computed "Content-Length" will be incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schaabs , should we change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory it should just work, can you debug and see why that codepaths is not getting hit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The identity tests have their own sanitizer, and it should update Content-Length as well.
azure-sdk-for-net/sdk/identity/Azure.Identity/tests/IdentityRecordedTestSanitizer.cs
Lines 28 to 30 in 854baa7
entry.Request.Body = Encoding.UTF8.GetBytes("Sanitized"); | |
UpdateSanitizedContentLength(entry.Request.Headers, 0, entry.Request.Body.Length); |
If we can't figure out why we still need to omit this from the matcher let's not block merging this. Just file an issue to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error looks like this:
Header differences:
<Content-Length> values differ, request <181>, record <9>
So it is playback Content-Length
that isn't sanitized.
sdk/identity/Azure.Identity/src/WindowsVisualStudioCodeAdapter.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending comments
Great job adding test coverage to this not-so-easy-to-cover feature 👍 |
- On Linux, disable tests that require Libsecret
{ | ||
internal sealed class LinuxVisualStudioCodeAdapter : IVisualStudioCodeAdapter | ||
{ | ||
private static readonly string s_userSettingsJsonPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), "Code", "User", "settings.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem like settings on Linux is under $HOME/.config/Code/User/settings.json.
See https://vscode.readthedocs.io/en/latest/getstarted/settings/
No description provided.