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

Remember Me? #1327

Merged
merged 34 commits into from Mar 6, 2018

Conversation

Projects
None yet
3 participants
@smashwilson
Member

smashwilson commented Mar 1, 2018

Add a "remember me" checkbox to the authentication dialog that's launched by the bundled git credential helper. When checked, if authentication is successful (as detected by git invoking git credential approve), store the provided credentials in your OS keychain by way of keytar. If authentication fails, delete them.

Remaining
  • Refactor the KeytarStrategy implementations out of github-login-model so I can reuse them from the credential helper.
  • Introduce a FileStrategy that fakes keytar by reading and writing a JSON file. This is insecure as hell, so put in extra guards against it handling actual credentials.
  • Pre-transpile keytar-strategy.js and get its transpiled path so that it can be executed from the credential helper subprocess (from specs).
  • Attempt to read credentials from keytar within the credential handler before prompting from the Atom dialog. If credentials are found, short-circuit and reply.
  • Store the username as a per-host default as well if it wasn't provided in the original query.
  • Store a successfully used credential pair on git credential approve.
  • Delete an unsuccessfully used credential pair on git credential reject.
  • Re-use the token from the GitHub panel if it's been set. The interesting wrinkle here is that we don't have a username, so we'll need to pull it from the GitHub API.
  • Add the actual "remember me" text to the authentication dialog.
  • Test the pre-transpiled keytar-strategy.js in (a) dev mode and (b) when bundled within an Atom build. Find an alternative if it doesn't work.
  • Investigate and correct test failures
  • Give it a spin on Windows
  • Give it a spin on Linux

Fixes #861.

@smashwilson smashwilson added this to In Progress 💻 in Short-Term Roadmap Mar 1, 2018

@smashwilson

This comment has been minimized.

Member

smashwilson commented Mar 2, 2018

@simurai: When you get the chance, would you mind styling the "remember me" box I've added to the credentials dialog? It's not what you'd call "pretty" right now:

screen shot 2018-03-02 at 4 47 06 pm

To get it to pop up:

  1. Clone a private repository with an https remote URL.
  2. Disable any credential helpers you have installed. You can see which ones are installed by running:
    git config --get-all credential.helper
    I had to comment out osxkeychain from my system git configuration. The easiest way to do that is to run:
    EDITOR='atom --wait' git config --edit --system
  3. Open the "Keychain Access" app and delete an atom-github key if you have one.
  4. Open the repository in Atom and fetch with the branch controls.

Feel free to mess with the CSS classes, markup, verbiage, and anything - I'll get the tests again after you're done 😄

@simurai

simurai approved these changes Mar 5, 2018

Checkbox is now styled.

remember

smashwilson added some commits Feb 28, 2018

wip

simurai and others added some commits Mar 5, 2018

@smashwilson smashwilson changed the title from [wip] Remember Me? to Remember Me? Mar 5, 2018

@smashwilson smashwilson merged commit d204c64 into master Mar 6, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Short-Term Roadmap automation moved this from In Progress 💻 to Complete ✅ Mar 6, 2018

@smashwilson smashwilson deleted the aw-remember-me branch Mar 6, 2018

@jlsam

This comment has been minimized.

jlsam commented Apr 20, 2018

This is probably something very trivial, but I installed Atom only last week and I keep seeing the dialog for authentication every time I try to push something. I don't see any check box there.

I was trying to follow the instructions to make it appear, but I got stuck at 'Open the "Keychain Access" app'. I have no idea what this could be, as I wasn't able to find anything like that within Atom. Can you explain better how to get there?

Also, I added a GitHub token authorization to Atom, is this important? I imagined this token would allow for authentication without login/pass, but that is not the case. What is the point of this token then?

@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 20, 2018

@jlsam This shipped in Atom 1.26 just yesterday, so it's possible that your Atom installation just hasn't picked up the upgrade yet. If you are on Atom 1.26 and you're not seeing the checkbox I'd love to know.

I was trying to follow the instructions to make it appear, but I got stuck at 'Open the "Keychain Access" app'. I have no idea what this could be, as I wasn't able to find anything like that within Atom. Can you explain better how to get there?

Oh, those instructions were meant specifically for @simurai - he needed to be able to consistently reach the dialog to apply CSS magic. They aren't the general instructions needed to see the authorization dialog; you see it any time a git operation needs to interact with a remote and doesn't have credentials cached.

Also, I added a GitHub token authorization to Atom, is this important? I imagined this token would allow for authentication without login/pass, but that is not the case. What is the point of this token then?

Previously, we used the token to authenticate to the GitHub API to fetch things like pull request data, but not to push. This PR changed that, so once you upgrade, the same token will be used for both.

@jlsam

This comment has been minimized.

jlsam commented Apr 20, 2018

I still have version 1.25.1, and in settings the 'Automatically update' checkbox is ticked. I installed via .deb package, does the auto-update not work? I followed the instructions from this issue and the output was

$ apm outdated; apm update;
Package Updates Available (0)
└── (empty)
Package Updates Available (0)
└── (empty)
@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 20, 2018

Ah, no, Atom doesn't update itself on Linux, we prefer to use the host distribution's mechanisms if possible. Rather than installing the .deb directly, you could try installing it through aptitude to receive updates through a regular apt-get upgrade.

The apm commands update packages that you have installed; the GitHub package is bundled with Atom, so you'll only receive updates for it when Atom itself is updated.

@jlsam

This comment has been minimized.

jlsam commented Apr 20, 2018

I uninstalled the version I had, added the PPA and installed Atom using apt.
No password is even asked now, stuff just gets sent, I guess that's because of the certificate. Cool. What happens though, is Atom now says in 'About' that my system does not support automatic updates. I didn't run sudo apt-get install atom-beta, only sudo apt-get install atom. This is off-topic, perhaps I should start an issue for this?

@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 20, 2018

That just means that Atom doesn't handle automatic updates internally - now you'll get updates through apt. You should be good now!

@smashwilson smashwilson removed this from Complete ✅ in Short-Term Roadmap Apr 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment