Hook up webflow auth during credential checks #94

shana · 2018-07-13T20:17:50Z

This is a Proof of Concept of implementing #93 with the webflow + localhost approach. I didn't want to rewrite the existing Configuration type (and I initially did this as a separate extension), so this has a separate implementation of the configuration class that keeps the token data - which is of course not really a good way to do it, but for this prototype it's probably good enough.

I've also added validating tokens for their required scopes, and triggering a "permissions upgrade" webflow if they don't. That also happens for any tokens that are read from the git credential helper.

This is how it works:

Some thoughts:

When and how should the authentication trigger? Right now it triggers automatically when the PRManager gets initialized, which means it will just happen as soon as VSCode opens and popup on a user's face before they have a chance to think.
- Should we show a UI before we start the authentication? A dialog box of some sort perhaps?
- If we show a UI to start the auth process in the editor, then we can skip the webpage confirmation page that currently shows up before the GitHub permissions page (see gif above), since the user is already aware that they're going through an authentication process. Should we do that?
I'm really not partial to storing tokens in the settings, I'd rather store them in the system credential manager. I assume there's some handy node library to do that for us?

I'm really new to both typescript and node applications in general so any feedback on the code as well as the overall design of the process is very much appreciated!

Tasks

Implement authentication for the server that the remote is pointing to
Validate existing credentials, reauthenticate if they're expired/invalid
Detect whether the server is a github instance, skip if it isn't
Show "You need to sign in to [server]" UI in VSCode
Show "You are signed in to [server]" at the end of the login flow or warning/errors in case something failed
Trigger authentication when expanding a node in the PR view
- Right now it's triggering at extension load time
Save credentials in system credential manager instead of the user settings file

msftclas · 2018-07-13T20:18:01Z

All CLA requirements met.

RMacfarlane

Nice! This is great.

Should we show a UI before we start the authentication?

I think this would be a better experience, we could show a notification telling the user they need to authenticate, with a button to do so. Skipping to the GitHub permissions page would make sense then 👍

There's an example of using the notification UI here: https://github.com/Microsoft/vscode-pull-request-github/blob/master/src/github/pullRequestManager.ts#L290

I'm also wondering if we should show another notification in VSCode after authentication. The redirect page already says the status, but it might be nice to have this in the VSCode UI as well?

I'm really not partial to storing tokens in the settings, I'd rather store them in the system credential manager. I assume there's some handy node library to do that for us?

yeah storing them in settings is quite bad. I believe https://www.npmjs.com/package/keytar could be used for this

RMacfarlane · 2018-07-16T17:48:18Z

src/authentication/configuration.ts

+        this.onDidChange = this.emitter.event;
+    }
+
+    update(username: string | undefined, token: string | undefined) {


This is looking good! I don't think we have a formalized style guide on TS that we use, but I'll share my opinions anyway 😄

Nice to have a return type on methods, even if it's void

I think marking public member variables explicitly as public is easier to read

I also prefer using const for variable declarations except when the variable will be reassigned to, in which case I use let. We're not very consistent about this as a team, this is just personal preference - I think this makes it easier to read since you (and the compiler) then have expectations about how variables should be used

Avoid any if possible/not extremely laborious

Thanks for this! I agree, explicit visibility and constness markers make things much easier to navigate. I'm using tslint and tslint-config-prettier locally to guide me through the language and nag me about the things I should/should not be doing, so I can configure it to follow these rules. Is tslint something you use as a team?

Yup, we use tslint. In VSCode we have a pretty small set of style rules and a larger number of rules dictating what can be imported where: https://github.com/Microsoft/vscode/blob/master/tslint.json

Adding a tslint.json to this project sounds like a good idea to me

I can add a basic tslint config file first.

RMacfarlane · 2018-07-16T18:15:15Z

src/authentication/vsConfiguration.ts

+}
+
+export class VSCodeConfiguration extends Configuration {
+	private hosts: { [key: string]: any };


We have a Map type that I think could be used here, perhaps private hosts: Map<string, IHostConfiguration>? Internally it looks very similar to the type wrote here. The syntax looks like

const map = new Map<T, T>();
map.set(someKey, someValue)
map.get(someKey)

(Reset would just be creating a new map, I think)

The current data structure is a bit confusing to me, since it seems to map the host to a number, and then the number to the whole host configuration object. If two different maps are needed, I think it would be clearer to have separate variables for them, but in this case I think it can be simplified to one map of host string to host configuration object

Does Map easily convert to an array?

I was struggling to find a type that could give me key access but easily turn into an array for saving into the configuration file (as that takes a simple []).

What I did here is a bit of a hack - it seems the typescript { [key: string]: any } syntax basically just adds an indexer to the underlying object is, so it's basically an array with an extra indexer (so I just store the index of the object to look it up in the array).

I first implemented a Dictionary type, and then discovered this key access syntax and kept two fields, one with the array of all the hosts and another with the keys and indexes to the array, but then discovered I could do everything in one object, which is what is there now. I agree it's really hard to read, I'll see if Map does the trick.

Ah, I see! I don't think there's any method that directly gives an array primitive from the map, but there are a couple of ways to iterate through it that could be used to create an array:

const flattenedMap = []; map.forEach(value => flattenedMap.push(value));

or

Array.from(map.values());

RMacfarlane · 2018-07-16T18:23:09Z

src/authentication/vsConfiguration.ts

+		super.update(config.username, config.token);
+	}
+
+	listenForVSCodeChanges() {


should this be called in the constructor?

Yeah, probably. Not sure where my head was on that one 🤔

RMacfarlane · 2018-07-16T18:30:19Z

src/authentication/webflow.ts

+				}
+				let expected = SCOPES.split(' ');
+				let serverScopes = new Set(scopes.split(', '));
+				var ret = expected.filter(x => {


another way to write this is

expected.every(x => serverScopes.has(x))

RMacfarlane · 2018-07-16T20:38:09Z

package-lock.json

@@ -50,18 +50,71 @@
      "integrity": "sha512-ONhaKPIufzzrlNbqtWFFd+jlnemX6lJAgq9ZeiZtS7I1PIf/la7CW4m83rTXRnVnsMbW2k56pGYu7AUFJD9Pow==",
      "dev": true
    },
+    "@types/body-parser": {


could you also stage the changes to package.json?

rebornix · 2018-07-17T02:47:53Z

Rachel already left good suggestions/comments, here are my 2 cents.

When and how should the authentication trigger
Should we show a UI before we start the authentication?

Having some UI before the authentication is a good idea and we can use a specific tree node in Pull Request tree view, or a statusbar item and together with a registered command to trigger the auth workflow. For example, Live Share uses a status bar item to indicate the auth status

The Azure Account extension provides a command

If we use a status bar item, then it can show authenticated user info once the auth part is done. If we show a specific login tree node in Pull Request Tree View then we don't need to anything specific, the login node will be removed and replaced with categories when the user logs in.

If we show a UI to start the auth process in the editor, then we can skip the webpage confirmation page that currently shows up before the GitHub permissions page

If I remember correctly the oauth/permission page needs a redirect page and it's static (ignore this if I'm wrong), what if the port we use is occupied?

rebornix · 2018-07-17T05:52:34Z

src/extension.ts

 	repository.onDidRunGitStatus(async e => {
 		if (repositoryInitialized) {
 			return;
 		}

+		let remotes = repository.remotes.filter(remote => remote.host);
+		let remote = remotes.find(remote => remote.remoteName === 'origin');


We may want to avoid relying on origin or probably making this a configuration (for example, github.defaultRemoteName). While working on VSCode code base, I use upstream for Microsoft/vscode and rebornix for my own fork, so it will fail if we check origin.

It's a corner case I believe but all our code logic right now doesn't rely on origin.

Yes, I agree, hardcoding origin is bad. Your logic right now relies on looking for a remote that is pointing to github.com; this logic finds either one that's called origin or the first remote if origin doesn't exist.

This means it will likely find rebornix for you before it finds upstream, if you cloned your fork first and then added the upstream remote. It's highly dependent on the clone/remote order. Definitely not ideal :P

On GHfVS, I think we look for remotes and then check whether the repository they point to is a fork, in which case we look up the fork parent and use that by default for PRs. I think we also let you switch between PR sources for a fork/upstream combo (i.e., you can see the PRs from upstream or you can see PRs from a fork, for long-lived project forks that have their own community)

The reason I don't want to scan for github.com is because github enterprise instances will work just as well with these features, and we can't really rely on looking at the name of the server. I probably need to add a check for a specific header that dotcom or enterprise always return, to make sure the server is usable (besides the whole "which remote do I use" thing)

A configuration option is probably good enough to start with to avoid these issues.

rebornix · 2018-07-17T05:59:13Z

src/authentication/configuration.ts

+    username: string | undefined;
+    token: string | undefined;
+    onDidChange: vscode.Event<IConfiguration>;
+    private emitter: vscode.EventEmitter<IConfiguration>;


We prefer to have private members to start with _.

rebornix · 2018-07-17T06:05:45Z

src/github/credentials.ts

-			});
-			return this._octokits[remote.url];
+		const webflow = new WebFlow(new VSCodeAppConfiguration(), remote.host);
+		let creds = this._configuration as IHostConfiguration;


It seems we only need IHostConfiguration in CredentialStore, we can unrestrict the type of this._configuration to be IHostConfiguration.

Oh, yeah, good catch, this was me carefully walking around your existing Configuration implementation by avoiding changing signatures. All the configuration stuff needs cleaning up :P

rebornix · 2018-07-17T06:22:36Z

src/authentication/configuration.ts

+import * as vscode from 'vscode';
+
+export interface IAppConfiguration {
+    clientId: string | undefined;


Not a big deal but our existing code is using tabs ;)

Darnit, I noticed that and configured prettier to use tabs but looks like spaces still snuck in. Fixicating...

shana · 2018-07-17T14:55:14Z

If we show a UI to start the auth process in the editor, then we can skip the webpage confirmation page that currently shows up before the GitHub permissions page

If I remember correctly the oauth/permission page needs a redirect page and it's static (ignore this if I'm wrong), what if the port we use is occupied?

Yeah, that's a problem I go over in #93. I think the ideal way is for the login to be handled by a webapp on a separate server instead of localhost, bypassing the need for local ports entirely - the local app talks to the server via a websocket, and the server and github talk to each other. That also mitigates the problem of distributing client secrets (as the server is the one keeping them)

shana · 2018-07-17T15:16:42Z

Should we show a UI before we start the authentication?

I think this would be a better experience, we could show a notification telling the user they need to authenticate, with a button to do so. Skipping to the GitHub permissions page would make sense then 👍

There's an example of using the notification UI here: https://github.com/Microsoft/vscode-pull-request-github/blob/master/src/github/pullRequestManager.ts#L290

Oh awesome, thanks, I'll take a look!

I'm also wondering if we should show another notification in VSCode after authentication. The redirect page already says the status, but it might be nice to have this in the VSCode UI as well?

Yeah, I was thinking how to redirect the user "eye" back to the editor. A notification would probably do it 👍

shana · 2018-07-31T17:23:42Z

I noticed that there's yarn support (and now I know what all the *.json files mean, yay for knowledge), so I've rebased all the code on top of master and redid the package changes for npm and for yarn. This version is using an external webapp to authenticate, so it doesn't need half the dependencies the previous version used (much simpler code, too!). I've address the style feedback, so the next step is to add the ui notification, make sure the remote selection code isn't regressing behaviour, and then make the response page pretty.

…ports it Make sure we only try to sign in once to a given server.

…e, etc)

Also show a message when the user is signed in successfully.

shana · 2018-08-03T14:44:32Z

Almost all the functionality is in (:tada:), I've added a list of tasks done and not done on the issue body, which I'm tackling next. I don't think I forgot anything, if I did let me know!

Right now this is using the websocket approach, but I think that we can actually use a protocol handler with the same model and skip the web socket entirely, if it turns out that web sockets cause problems. Web sockets basically go through the same pipe as https requests, so I feel that if a web socket doesn't work, https wouldn't either and you'd be kinda stuck either way, so not sure I want to change that right now.

rebornix · 2018-08-06T21:55:02Z

src/github/credentials.ts

+			// see if the system keychain has something we can use
+			const data = await fill(host);
+			if (data) {
+				const login = await server.validate(data.username, data.password);


I'm using personal access token for https remotes so this line is being executed as I have username/token saved in git credentials. However this line never returns. Once I commented out this part and it fell back to the oauth login, everything works as expected.

Interesting, I wonder what's going on. It might be throwing somewhere and getting swallowed. I'll try to repro locally.

I think I fixed this, there were some very silly bugs in that code.

Delay the credential check and doing sign in until the first interaction of the user with something that requires API access, so basically it will only kick in when the user expands one of the PR lists.

A token with a "user" scope will work just as well as one with "user:email".

shana · 2018-08-07T18:32:22Z

I think this is done! Here's how the flow looks now:

The webbrowser UI is being worked on so it'll look better soon 😄

The credentials are still being stored in the settings file, but I think I'd rather do the work of moving them to the system credential manager on a separate PR, as there might be some other complications with that and I don't want to delay this more. I'd also like to add a command to trigger the login, probably something that can be done separately.

I'm also unsure whether the sign in notification will work or if it's too subtle. It will pop up for anything that needs to call octokit (because getting an octokit instance triggers the whole credential loading thing). UX is hard!

RMacfarlane · 2018-08-08T23:42:24Z

package.json

+          "default": [],
+          "description": "Host tokens",
+          "items": {
+            "type": "object"


I think we can also add a "properties" property within items, which helps intellisense in the settings editor

something like

"properties": { "host": { "type": "string", "description": "The host name to access GitHub." }, "username": { "type": "string", "description": "The host name to access GitHub." }, "token": { "type": "string", "description": "GitHub access token." } }

Oh cool, didn't know that, thanks!

RMacfarlane · 2018-08-08T23:47:05Z

src/authentication/configuration.ts

+export const HostHelper = class {
+	public static getApiHost(host: IHostConfiguration | vscode.Uri): vscode.Uri {
+		const hostUri: vscode.Uri = host instanceof vscode.Uri ? host : vscode.Uri.parse(host.host);
+		if (hostUri.authority === 'github.com') {


is it possible for there to be a case mismatch?

hmmmmm, good question. possibly, yes, I can test that quick

RMacfarlane · 2018-08-09T00:54:53Z

src/github/pullRequestManager.ts

@@ -14,7 +14,8 @@ import { IPullRequestManager, IPullRequestModel, IPullRequestsPagingOptions, PRT
 import { PullRequestGitHelper } from "./pullRequestGitHelper";
 import { PullRequestModel } from "./pullRequestModel";
 import { parserCommentDiffHunk } from "../common/diffHunk";
-import { Configuration } from "../configuration";
+import { Configuration } from '../authentication/configuration';
+import { GitHubManager } from '../authentication/githubserver';


../authentication/githubServer

ooof, good catch, that would fail on linux :/

RMacfarlane

This looks great! I had some small comments but I think this looks good to merge

shana · 2018-08-09T14:59:14Z

I went through and normalized all the hostnames to lowercase, there are other places where there could be case mismatches. Tested that out with another repo I have with a bunch of different remotes (to which I added a few more), and it matches things correctly for all of them.

another-upstream        https://github.com/octokit/octokit.net.git (fetch)
another-upstream        https://github.com/octokit/octokit.net.git (push)
editor-tools    https://github.com/editor-tools/octokit.net (fetch)
editor-tools    https://github.com/editor-tools/octokit.net (push)
origin  git@github.com:github-for-unity/octokit.net (fetch)
origin  git@github.com:github-for-unity/octokit.net (push)
origin-http     https://github.com/github-For-Unity/octokit.net.git (fetch)
origin-http     https://github.com/github-For-Unity/octokit.net.git (push)
shana   https://github.com/shana/octokit.net (fetch)
shana   https://github.com/shana/octokit.net (push)
upstream        https://github.com/Octokit/Octokit.net (fetch)
upstream        https://github.com/Octokit/Octokit.net (push)

rebornix

LGTM.

RMacfarlane reviewed Jul 16, 2018

View reviewed changes

rebornix reviewed Jul 17, 2018

View reviewed changes

Andreia Gaita added 6 commits July 31, 2018 14:15

Hook up webflow auth during credential checks

6108777

Update yarn dependencies

db06fca

Update package-lock.json

6590351

Add missing hosts property

5a1a708

Add ws

33d5ff5

Implementing oauth webflow via external webapp

d605f65

shana force-pushed the shana/login branch from 621b589 to d605f65 Compare July 31, 2018 17:17

Andreia Gaita added 2 commits August 1, 2018 15:43

Add UI for initiating sign in. Try anonymous access if the server sup…

04219f0

…ports it Make sure we only try to sign in once to a given server.

Add support for any github host (hosted business instances, enterpris…

3c4c30d

…e, etc)

shana changed the title ~~[do not merge] Hook up webflow auth during credential checks~~ [wip] Hook up webflow auth during credential checks Aug 1, 2018

Andreia Gaita added 3 commits August 2, 2018 16:16

Check if the server is a github instance when selecting remotes to use

5b198d5

Also show a message when the user is signed in successfully.

Oh yeah, we don't want to save all the other fields...

3856af5

Tweak the authentication user messages

7c64a68

rebornix mentioned this pull request Aug 6, 2018

Repo is null error when the remote url has username in it #97

Closed

rebornix reviewed Aug 6, 2018

View reviewed changes

shana and others added 4 commits August 7, 2018 11:50

Merge master into shana/login

87e21ae

Sync yarn.lock and package-lock.json

f6f7799

Delay checking credentials and displaying signin

1b51607

Delay the credential check and doing sign in until the first interaction of the user with something that requires API access, so basically it will only kick in when the user expands one of the PR lists.

Improve scope validation by checking supersets as well

ea59abb

A token with a "user" scope will work just as well as one with "user:email".

shana changed the title ~~[wip] Hook up webflow auth during credential checks~~ Hook up webflow auth during credential checks Aug 7, 2018

This was referenced Aug 8, 2018

No comments in the editor when visiting file changes from commits #141

Closed

Commits file change tree node misaligned after being refreshed #142

Closed

RMacfarlane reviewed Aug 8, 2018

View reviewed changes

RMacfarlane reviewed Aug 9, 2018

View reviewed changes

RMacfarlane approved these changes Aug 9, 2018

View reviewed changes

Andreia Gaita added 4 commits August 9, 2018 15:51

Merge master into shana/login

7f1873f

Add nice descriptions of the settings

4e49ad6

Fix import casing

cb7a3d2

Make sure we always look at uris in a case-insensitive way

d62e444

rebornix approved these changes Aug 9, 2018

View reviewed changes

rebornix merged commit 6e31f82 into master Aug 9, 2018

rebornix mentioned this pull request Aug 14, 2018

Save oauth tokens to system keychain #190

Closed

rebornix deleted the shana/login branch November 20, 2018 18:29

alexr00 restored the shana/login branch December 18, 2023 16:26

alexr00 deleted the shana/login branch December 18, 2023 16:27

Hook up webflow auth during credential checks #94

Hook up webflow auth during credential checks #94

Conversation

shana commented Jul 13, 2018 • edited Loading

Tasks

msftclas commented Jul 13, 2018 • edited Loading

RMacfarlane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebornix commented Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shana commented Jul 17, 2018

shana commented Jul 17, 2018

shana commented Jul 31, 2018

shana commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shana commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RMacfarlane left a comment

Choose a reason for hiding this comment

shana commented Aug 9, 2018

rebornix left a comment

Choose a reason for hiding this comment

shana commented Jul 13, 2018 •

edited

Loading

msftclas commented Jul 13, 2018 •

edited

Loading

rebornix commented Jul 17, 2018 •

edited

Loading