Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Refactor Logins, Part Uno #845
Conversation
Injecting it into RepositoryHost, but not using it yet.
Also updated serilog to correctly handle `Received()` on async methods.
Rather than having login logic in `RepositoryHost`.
Delegate loading/saving of connections to an `IConnectionCache` interface so that loading/saving of connections can be tested separately from actual `ConnectionManager` behavior.
This comment has been minimized.
This comment has been minimized.
shana
commented on src/GitHub.Exports/Models/ConnectionDetails.cs in 164e780
Feb 3, 2017
|
This will call |
This comment has been minimized.
This comment has been minimized.
|
If you override the However, this code already does that checking in the other public override bool Equals(object obj)
{
return Equals(obj as ConnectionDetails);
}There's no need to duplicate all that |
This comment has been minimized.
This comment has been minimized.
|
Heh, so this is what I get for using Resharper to generate the equality code automatically. First and last time! Thanks guys. |
This comment has been minimized.
This comment has been minimized.
|
I spent two hours on this code last week because I suspected infinite recursion somewhere in here and still couldn't see it, nice catch! We already have code that does this in a safe way in our various models that implement IEquatable, btw, for sanity checking. |
This comment has been minimized.
This comment has been minimized.
|
To be clear, this recursion is totally on me! The code I pasted above is how we implement equals everywhere else, but it relies on != and == being implemented differently, the way we do it in other places. |
And add some doc comments.
Make it return a `TwoFactorChallengeResult` as that's what we're interested in, rather than returning a `RecoveryOptionResult` and passing the actual result back in the `UserError` that was passed into it.
Don't stop listening for changes to `UserError` when control is unloaded - this meant that if a page with a `UserError` is removed and then shown again, the `UserErrorMessages` control stopped working.
And fixed unused fields that it detected.
Rather than using reflection in `GitHub.Api/ApiClientConfiguration` to get hold of the client ID/secret from `ApiClient`, inject them directly into `ApiClientConfiguration` and retreive them from there in `ApiClient.
Conflicts: src/GitHub.App/ViewModels/TwoFactorDialogViewModel.cs src/GitHub.Exports/Settings/Guids.cs src/GitHub.VisualStudio/UI/Views/Controls/LoginControl.xaml.cs
This reverts commit f684f6f.
Make sure the observables are signalled on the UI thread.
| namespace GitHub.Api | ||
| { | ||
| public partial class ApiClient : IApiClient | ||
| static partial class ApiClientConfiguration |
jcansdale
Apr 28, 2017
•
Collaborator
It would be nice if rather than getting the ClientId / ClientSecret from source, it got it from an environment variable (GITHUB_CLIENT_ID or something). That way the developer wouldn't have to dirty their repo whenever they wanted to try something out (a hassle when changing branches). It could complain with a visible/helpful error when these variables aren't set.
It would be nice if rather than getting the ClientId / ClientSecret from source, it got it from an environment variable (GITHUB_CLIENT_ID or something). That way the developer wouldn't have to dirty their repo whenever they wanted to try something out (a hassle when changing branches). It could complain with a visible/helpful error when these variables aren't set.
grokys
May 3, 2017
Author
Contributor
Yeah, I was wondering about that as well. We can think about that later I'd say.
Yeah, I was wondering about that as well. We can think about that later I'd say.
shana
May 3, 2017
Collaborator
These IDs need to be in the code, users can't be configuring this data manually on their systems in order to get the extension working. We can gitignore the file that keeps these variables in the public build so people can configure and build with them without having the repo dirtied or accidentally pushed.
These IDs need to be in the code, users can't be configuring this data manually on their systems in order to get the extension working. We can gitignore the file that keeps these variables in the public build so people can configure and build with them without having the repo dirtied or accidentally pushed.
jcansdale
May 3, 2017
Collaborator
Sounds like a plan!
@grokys can we .gitignore the local ApiClientConfiguration_User.cs file?
Sounds like a plan!
@grokys can we .gitignore the local ApiClientConfiguration_User.cs file?
shana
May 3, 2017
Collaborator
We might need to tweak the csproj and the default codepath a bit so that if it doesn't find the file, it can still build with fake data (right now the file always has to be there). Probably also better to move it to the common folder, since it's "common" configuration data and that's an easier location to point to than all the way inside GitHub.Api, and maybe rename it to something more obvious. The steps would probably be something like:
- Set default
ClientId and ClientSecret values in ApiClientConfiguration.cs if they're null after Configure()
- Delete the public
ApiClientConfiguration_User.cs file
- Add a
src/common/ApplicationKeys.cs file to .gitignore
- Include
src/common/ApplicationKeys.cs in the csproj if it exists and it's not an internal build
- Add a little script that asks the user for a client id and client secret and generates the code with them.
- Update the docs with these instructions (and also the template if people want to do it manually)
We can do this on a separate PR, probably 😄
We might need to tweak the csproj and the default codepath a bit so that if it doesn't find the file, it can still build with fake data (right now the file always has to be there). Probably also better to move it to the common folder, since it's "common" configuration data and that's an easier location to point to than all the way inside GitHub.Api, and maybe rename it to something more obvious. The steps would probably be something like:
- Set default
ClientIdandClientSecretvalues inApiClientConfiguration.csif they're null afterConfigure() - Delete the public
ApiClientConfiguration_User.csfile - Add a
src/common/ApplicationKeys.csfile to .gitignore - Include
src/common/ApplicationKeys.csin the csproj if it exists and it's not aninternalbuild - Add a little script that asks the user for a client id and client secret and generates the code with them.
- Update the docs with these instructions (and also the template if people want to do it manually)
We can do this on a separate PR, probably
jcansdale
May 3, 2017
Collaborator
A separate PR that fixes a few things for public builds would certainly be good. We could sort out the public / open source signing issue at the same time. Recent versions need to be signed and this isn't happening at the moment. They're all related issues with quite similar fixes.
A separate PR that fixes a few things for public builds would certainly be good. We could sort out the public / open source signing issue at the same time. Recent versions need to be signed and this isn't happening at the moment. They're all related issues with quite similar fixes.
| @@ -130,6 +132,7 @@ protected override Task InitializeAsync(CancellationToken cancellationToken, IPr | ||
| { | ||
| AddService(typeof(IGitHubServiceProvider), CreateService, true); | ||
| AddService(typeof(IUsageTracker), CreateService, true); | ||
| AddService(typeof(ILoginManager), CreateService, true); |
jcansdale
Apr 28, 2017
Collaborator
Could this be exposed as MEF export rather than a Visual Studio service? If we're ever going to add support for Blend (which also hosts TeamExplorer), we'll need to avoid using custom services.
Could this be exposed as MEF export rather than a Visual Studio service? If we're ever going to add support for Blend (which also hosts TeamExplorer), we'll need to avoid using custom services.
grokys
May 3, 2017
Author
Contributor
Actually, I think it could. I think it was originally a service as it had an async initialization, but that's not there any more.
Actually, I think it could. I think it was originally a service as it had an async initialization, but that's not there any more.
shana
May 3, 2017
Collaborator
The services that need to be initialized here are things that we need to use at VS startup. MEF is too slow at loading its catalog and causes a performance hit that we can't afford at this stage. The login manager is not one of these services, so it should definitely be an export.
The services that need to be initialized here are things that we need to use at VS startup. MEF is too slow at loading its catalog and causes a performance hit that we can't afford at this stage. The login manager is not one of these services, so it should definitely be an export.
grokys
May 16, 2017
Author
Contributor
Replying to first message.
Replying to first message.
|
I've prodded and poked these changes and they look good to me! |
| @@ -14,7 +14,7 @@ | ||
| <NuGetPackageImportStamp> | ||
| </NuGetPackageImportStamp> | ||
| <TargetFrameworkProfile /> | ||
| <BuildType Condition="Exists('..\..\script\src\ApiClientConfiguration.cs')">Internal</BuildType> | ||
| <BuildType Condition="Exists('..\..\script\src\ApiClientConfiguration_User.cs')">Internal</BuildType> |
jcansdale
Apr 28, 2017
Collaborator
Just a observation having seen all the .csproj tweaks. It would be nice to factor out these common signing related properties into a separate .props file. I'll do this in a separate PR along with the public key stuff.
Just a observation having seen all the .csproj tweaks. It would be nice to factor out these common signing related properties into a separate .props file. I'll do this in a separate PR along with the public key stuff.
grokys
May 3, 2017
Author
Contributor
👍
|
FYI, @StanleyGoldman and I went to town on how git manages credentials yesterday and found a few interesting things, including how to easily break our credential and login code in GHfVS by logging in with GCM, wincred, Unity and command line git. I suspect some of the issues we are seeing are coming from other providers doing things and we not handling that very well (especially the weird I'm |
Conflicts: src/GitHub.App/Models/RepositoryHost.cs
This is the first step of #809. I wanted to stop here and submit a PR even if everything isn't finished as there's enough to review already. If need be I can probably split this into separate "refactor" and "bug fix" PRs but I wanted to a) actually get some bug fixes in with the refactoring, and b) test that the refactoring actually helps solve real problems.
Refactored
ApiClientConfigurationclass in GitHub.ApiGitHub.Api.LoginManagerservice which contains the actual login code (moved out ofRepositoryHostwhich is a model and shouldn't contain such code)LoginCache(nowWindowsLoginCache)ConnectionManagerand intoIConnectionCache(JsonConnectionCache)SimpleApiClientFactory.CreateandApiClientFactory.Createasync (this will pave the way for first checking if there is a valid connection)UserError.ThrowonLoginTabViewModel- instead just expose theUserErrorin a property - this removes the need for the changing tabs hack, simplifies things, and more importantly avoids a race condition when moving back to the login view with an error.TwoFactorDialogViewModel.Show: 7843278Fixes