Skip to content
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

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

mjcheetham
Copy link
Collaborator

@mjcheetham mjcheetham commented Jul 11, 2023

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!).

Fixes #1325


Bug repro instructions:

  1. Open Visual Studio
  2. File > Account Settings
  3. Add a GitHub account
  4. Open a terminal (inside or outside of VS) and attempt to clone/fetch/push to or from a private GitHub repository.

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.

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!).
Copy link
Collaborator

@dscho dscho left a 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?

@mjcheetham
Copy link
Collaborator Author

mjcheetham commented Jul 12, 2023

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?

The method is not used for testing, but the internal visibility modifier is because this method is called from unit tests.

Speaking of testing, is there a way to ensure that this does the right thing by adding a test?

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 https://example.com and git-test:https://example.com.

@mjcheetham
Copy link
Collaborator Author

@dscho I have now added extra tests around this area.

Copy link
Contributor

@ldennington ldennington left a 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.

@mjcheetham mjcheetham merged commit 0d70623 into git-ecosystem:main Jul 12, 2023
@mjcheetham mjcheetham deleted the wincred-account-fix branch July 12, 2023 16:32
@mjcheetham mjcheetham mentioned this pull request Jul 12, 2023
mjcheetham added a commit that referenced this pull request Jul 12, 2023
**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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants