-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Only match credential entries with correct namespace in the Windows Credential Manager #1328
Only match credential entries with correct namespace in the Windows Credential Manager #1328
Conversation
GCM by default creates entries in the Windows Credential Manager on Windows, and prefixes the 'target name' of the entry with "git:". This 'namespace' prefix is configurable, but is not often changed in practice outside of tests. Visual Studio, when adding GitHub accounts (either natively or by the older GitHub extension for VS), it creates three credential entries: 1. GitHub for Visual Studio - https://github.com 2. git:https://github.com 3. https://github.com Entry 1 is used by VS for it's own purposes. Entry 2 is created for the benefit for GCM, so that we are 'primed'. It is unknown what entry 3 is for at this time. There is an error in our existing logic for enumerating credentials that is also matching entry 3 as well as the expected entry 2. Modify and fix the matching logic to ensure that the namespace prefix matches, rather than just stripping it and matching (even if it doesn't exist!).
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 de7b93b#diff-c4c58b08f112c67b1d49742bdc348cbfc3dd410fb2e46ecfee795175d3cd5e4bL268, the modified method is marked with the comment /* for testing */
, which looks a bit funny to me: is this comment obsolete? Or is this method really used only for testing?
Speaking of testing, is there a way to ensure that this does the right thing by adding a test?
The method is not used for testing, but the
Although we have tests for all sorts of matching, we never tested the matching of no-namespace credential entries.. I can add a test for |
249112e
to
88e1297
Compare
@dscho I have now added extra tests around this area. |
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.
New tests are great, and I got the fix working on my machine.
**Changes since 2.2.1:** - Fix an issue where duplicate "Personal Access Token" GitHub account options are shown when Visual Studio has a GitHub account signed-in (#1325 #1328) - Fix an issue with Azure DevOps Server (TFS) and Windows Integrated Authentication (#1331 #1332) - Fix an issue with OAuth redirects GitHub Enterprise Server (#1329 #1330) - Correctly handle non-ASCII username/passwords with the WPF UI helpers (#1287 #1326)
GCM by default creates entries in the Windows Credential Manager on Windows, and prefixes the 'target name' of the entry with "git:". This 'namespace' prefix is configurable, but is not often changed in practice outside of tests.
Visual Studio, when adding GitHub accounts (either natively or by the older GitHub extension for VS), it creates three credential entries:
Entry 1 is used by VS for it's own purposes. Entry 2 is created for the benefit for GCM, so that we are 'primed'. It is unknown what entry 3 is for at this time.
There is an error in our existing logic for enumerating credentials that is also matching entry 3 as well as the expected entry 2.
Modify and fix the matching logic to ensure that the namespace prefix matches, rather than just stripping it and matching (even if it doesn't exist!).
Fixes #1325
Bug repro instructions:
At this point a window should appear asking you to select between two "Personal Access Token" accounts.
After installing the bits from this PR build (artifacts > win-x86), attempting step 4 should no longer result in a prompt to select between two "Personal Access Token" accounts.