This repository was archived by the owner on Jun 21, 2023. It is now read-only.
Master PR for refactoring connections #1277
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously credentials were read by 4 separate interfaces/classes: - `IKeychain`/`WindowsKeychain` - `ILoginCache`/`LoginCache` - `SimpleCredentialStore` - `CredentialCache` Make everything go via `IKeychain`.
Just call `ToObservable` on the task returned by `IKeychain`.
Conflicts: src/GitHub.App/Models/RepositoryHost.cs src/UnitTests/GitHub.App/Models/RepositoryHostTests.cs
Store our own "master" credential in the Windows Credential store that is unlikely to get overwritten.
Removed `IConnection.Repositories` and `IConnectionManager.RefreshRepositories` and replaced them with `LocalRepositories` which acts as a central place to find known repositories. As part of this, also added `IReadOnlyObservableCollection` and `ObservableCollectionEx` because the BCL's `ReadOnlyObservableCollection` sucks. Made `TrackingCollection` implement `IReadOnlyObservableCollection`.
Following advice by @shana.
This services the same purpose as `IConnectionRepositoryHostMap` served when `RepositoryHost` was still around: to provide access to the globally registered connection.
…local-repositories
…connection-manager Conflicts: src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs src/UnitTests/GitHub.VisualStudio/Services/ConnectionManagerTests.cs
When creating a `ModelService`, ensure that the correct `AccountCacheItem` is inserted.
Conflicts: src/GitHub.App/Caches/LoginCache.cs src/GitHub.App/Caches/SharedCache.cs src/GitHub.App/Factories/ApiClientFactory.cs src/GitHub.App/Models/RepositoryHost.cs src/GitHub.App/Services/GitHubCredentialStore.cs
…local-repositories Conflicts: src/GitHub.VisualStudio/Services/ConnectionManager.cs
…connection-manager Conflicts: src/GitHub.App/Models/RepositoryHost.cs src/GitHub.App/Models/RepositoryHosts.cs src/GitHub.App/ViewModels/PullRequestListViewModel.cs src/GitHub.VisualStudio/Services/ConnectionManager.cs
…remove-repositoryhosts Conflicts: src/GitHub.App/Models/RepositoryHost.cs src/GitHub.App/Models/RepositoryHosts.cs src/GitHub.App/Services/ModelService.cs src/GitHub.App/ViewModels/GistCreationViewModel.cs src/GitHub.App/ViewModels/LoginTabViewModel.cs src/GitHub.App/ViewModels/PullRequestCreationViewModel.cs src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs src/GitHub.App/ViewModels/PullRequestListViewModel.cs src/GitHub.App/ViewModels/RepositoryCloneViewModel.cs src/GitHub.App/ViewModels/RepositoryCreationViewModel.cs src/GitHub.App/ViewModels/RepositoryPublishViewModel.cs src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs
…sitoryhosts Remove RepositoryHost(s)
Conflicts: src/GitHub.App/ViewModels/PullRequestCreationViewModel.cs test/UnitTests/GitHub.App/Caches/CredentialCacheTests.cs test/UnitTests/GitHub.App/Models/RepositoryHostTests.cs test/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs test/UnitTests/Helpers/TestLoginCache.cs
shana
previously approved these changes
Nov 17, 2017
There was a problem hiding this comment.
This looks good and the PRs that compose it have already been separately reviewed 👍
A few things that are important to note following this:
- Visual Studio Login with Email #1297 Making sure the username is always saved in the credential manager in all the entries. Even though it's not technically needed for using the token, it's useful for knowing what user created it in order to recreate connections later if data goes out of sync without having to explicitely ping the API for it, which is especially important if the internet goes out while we're doing this (we need to know the user to query the akavache database for most cached things)
- Create connection from credential #1331 Hardening and testing the credential manager and connection data link - if we have the custom credential manager entry but one of the other entries is missing or the connection entry is missing in our json file, recreate them. This is something we're probably doing already, but we should test to make sure it's sturdy
- Testing the interaction between GitHub for Unity logout/login with GHfVS. Can GHfVS recover if GHfU (or other git clients, really) stomps all over its credentials?
- Validate scopes for login. #1332 Validating the token for permission scopes. If other git clients replace the token, they may not create one with the correct scopes, we need to handle that.
- Share login credentials between GHfVS and GHfU. #1333 Getting together with @StanleyGoldman to figure out what's a good way of sharing credential and user information across GHfVS and GHfU so that you can login on one side and use the other seamlessly
Conflicts: src/GitHub.App/Models/RepositoryHost.cs src/GitHub.App/Services/PullRequestService.cs src/GitHub.App/ViewModels/RepositoryCreationViewModel.cs src/GitHub.App/ViewModels/RepositoryPublishViewModel.cs test/UnitTests/GitHub.App/Models/RepositoryHostTests.cs
Conflicts: src/GitHub.App/ViewModels/PullRequestListViewModel.cs
This was referenced Nov 21, 2017
This was referenced Dec 11, 2017
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will serve as a master PR for connection refactor work. This is a protected branch, submit PRs to this branch to modify.
The Plan
Merge duplicate classes
There are 2 ways to do a lot of things, a situation caused by having to remove Rx from Team Explorer at the last minute. Refactor everything into a single class for each concern, using
Tasks rather than observables.Harden Credentials
Store our own "master" credential in the Windows Credential store which we can restore on startup.
Remove IRepositoryHost(s)
Merge
IConnectionManagerandIRepositoryHostsintoIConnectionManager-RepositoryHostsis old code ported from the .NET desktop app. It does pretty much the same thing asIConnectionManagerexcept it gets the list of connections from the Windows Credential Manager instead ofconnections.jsonwhich causes #1242 and #1094.Future: Provide better failure modes
Currently if VS is started up disconnected from the internet, the connection is simply removed from Team Explorer. We need to display a message with a "Retry" button or similar if a connection error happens, and similarly display a "You need to log in again" button or similar if the users credentials have changed. This will be done in a separate, future PR,
PRs to be merged in/already merged in:
IConnection.Repositories. #1279 - RemovedIConnection.Repositories