Add recipe for auth-source-keytar

[jcs090218]

Brief summary of what the package does

Emacs-Lisp interface for node-keytar

Direct link to the package repository

• https://github.com/emacs-grammarly/keytar
• https://github.com/emacs-grammarly/auth-source-keytar

Your association with the package

The maintainer.

Relevant communications with the upstream package maintainer

None needed.

Checklist

Please confirm with x:

• The package is released under a GPL-Compatible Free Software License.
• I've read CONTRIBUTING.org
• I've used the latest version of package-lint to check for packaging issues, and addressed its feedback
• My elisp byte-compiles cleanly
• M-x checkdoc is happy with my docstrings
• I've built and installed the package using the instructions in CONTRIBUTING.org
• I have confirmed some of these without doing them

[tarsius]

That's a very heavy dependency (for lsp-grammarly). I wouldn't install a package that forced me to install node just so secrets can be stored. I suggest you make this optional and let users use the built-in auth-source if they prefer that.

[jcs090218]

• lsp-grammarly uses node to install the language server so there is no way to avoid using node
• keytar is lightweight, this is only use when you need premium version

I am not familiar with auth-source, does it work on Windows? By using keytar, I could sync with vscode Grammarly client (because they use keytar as well).

[tarsius]

lsp-grammarly uses node to install the language server so there is no way to avoid using node

Alright, then node of course isn't an unnecessary dependency and it is okay to depend on it for other things as well. That being said...

keytar is lightweight, this is only use when you need premium version

So many (most?) users won't even need it. So I still think it would be a good idea to make this an optional dependency.

I am not familiar with auth-source, does it work on Windows?

Yes, though that might be more complicated than on Linux and I have no experience with it.

It supports multiple plug-able backends and keytar could become one of them. I.e. you might want to look into implementing such a backend.

[jcs090218]

So many (most?) users won't even need it. So I still think it would be a good idea to make this an optional dependency.

Yeah, you are right. Yet I don't like asking people to install another package if they want a feature working unless it is heavy dependency. And I think premium version is a major feature and should not be optional. I don't know how many users would need it or not, but since many people were asking this grammarly/#1, grammarly/#3, lsp-grammarly/#3, I think it would be great to have it.

Yes, though that might be more complicated than on Linux and I have no experience with it.

It supports multiple plug-able backends and keytar could become one of them. I.e. you might want to look into implementing such a backend.

I don't know if I want another layer to do this, but good to know. Thanks! :)

[riscy]

since many people were asking this grammarly/#1, grammarly/#3, lsp-grammarly/#3, I think it would be great to have it.

My reading of those issues is that people wanted a way to login with their own credentials -- fair. But nobody is saying "why doesn't this support logging in with keytar?" So you're requiring (paid and unpaid) users to use/install keytar whether they prefer to use another password manager or not.

In fact -- and this is back to referencing this PR rather than lsp-grammarly -- if my understanding is correct, this package is requiring users to install your fork (!) of the keytar-cli while pointing them to https://www.npmjs.com/package/keytar in your commentary. This is extremely sketchy. Your users will end up with keytar on their PATH, and will probably assume it's the official version and not a fork, and we're talking about a password manager here.

[jcs090218]

But nobody is saying "why doesn't this support logging in with keytar?" So you're requiring (paid and unpaid) users to use/install keytar whether they prefer to use another password manager or not.

My first thought is "why not?", but I guess this would not be an acceptable answer. Below are my reasons.

• lsp-grammarly ported from Grammarly (unofficial), and it uses keytar
• I am being ignorance on password manager since I don't know much about it

I don't have preference on a password manager, I am just providing a solution. If people doesn't like it, they can fork or open a PR for it.

This is extremely sketchy. Your users will end up with keytar on their PATH, and will probably assume it's the official version and not a fork, and we're talking about a password manager here.

node-keytar doesn't have an official keytar-cli, and my upstream repository isn't an official one too.

if my understanding is correct, this package is requiring users to install your fork (!) of the keytar-cli while pointing them to https://www.npmjs.com/package/keytar in your commentary.

I am pointing to that repository because it attempt to serve API from that specific npm package.

[tarsius]

This is extremely sketchy. Your users will end up with keytar on their PATH, and will probably assume it's the official version and not a fork, and we're talking about a password manager here.

node-keytar doesn't have an official keytar-cli,

emacs-grammarly/keytar-cli is a fork of mattrenaud/keytar-cli aka the upstream. If users end up using the former while expecting to be using the latter, then that is sketchy. It would probably be best if you could get your changes merged upstream before we proceed here.

and my upstream repository isn't an official one too.

What repository are you referring to?

[tarsius]

I don't have preference on a password manager, I am just providing a solution. If people doesn't like it, they can fork or open a PR for it.

I still don't think you should force this solution on users. auth-source is the default solution for this sort of thing in Emacs, so its what you should support.

[purcell]

auth-source is the default solution for this sort of thing in Emacs, so its what you should support.

Totally agree. You can then trivially provide an auth-source-keytar package (like the existing auth-source-* packages in MELPA) for users like yourself who want keytar.

[riscy]

I'd confused keytar-cli (the original) for an official package, so apologies if I seemed quick to judge. I still think forking the existing command-line app and having users access it with the same command (keytar) could lead to confusion and/or missed security patches which is why I sounded so concerned. You should definitely aim to get your necessary change merged into the upstream (and avoid saddling yourself with that maintenance burden while you're at it).

It also sounds like auth-source is the way to go here, and it seems like an auth-source-keytar package would be a strong contribution.

[jcs090218]

emacs-grammarly/keytar-cli is a fork of mattrenaud/keytar-cli aka the upstream. If users end up using the former while expecting to be using the latter, then that is sketchy. It would probably be best if you could get your changes merged upstream before we proceed here.

The upstream seems to be unmaintained? hmm...

What repository are you referring to?

https://github.com/mattrenaud/keytar-cli

otally agree. You can then trivially provide an auth-source-keytar package (like the existing auth-source-* packages in MELPA) for users like yourself who want keytar.

Okay, I can see the point making it with auth-soruce-*.

You should definitely aim to get your necessary change merged into the upstream (and avoid saddling yourself with that maintenance burden while you're at it).

I think it would be easier for me to maintain it myself since I don't think @mattrenaud is very active on GitHub and the upstream isn't official repository (basically anyone can have their own fork). In addition, I have wrapped the npm package with scope of @emacs-grammarly so it should be clear to see IMO.

• npm page, https://www.npmjs.com/package/@emacs-grammarly/keytar-cli
• Install it through @emacs-grammarly/keytar-cli should be obvious

Of course, if there is an official keytar cli repository, I would switch to that npm package instead of my own fork.

It also sounds like auth-source is the way to go here, and it seems like an auth-source-keytar package would be a strong contribution.

I can work on this. Would it still be different packages or all in one package?

• keytar (repo)
  • keytar.el
  • auth-source-keytar.el

Do I need to open another PR? Thanks!

[tarsius]

IMO neither a separate repository nor a new pull-request are needed. I would even distribute both libraries as one package (keytar) because (unlike say helm) auth-source is part of Emacs, so it doesn't bring in additional dependencies.

[tarsius]

On the other hand distributing auth-source-keytar as a separate package would serve to advertise its existence.

[jcs090218]

Okay, I have managed to have it in the current repository by having the file auth-source-keytar.el for the auth-source support. In addition, I'm don't really know much about auth-source so please give my any feedback to improve it. Thanks!

[riscy]

I think what we're aiming for is a separate package called auth-source-keytar. Although I haven't looked at them, there are a couple examples on MELPA already.

[riscy]

Sorry, I'm just reading what @tarsius wrote. Let me think some more.

[riscy]

Yeah, regarding discoverability I think it would be best to name this auth-source-keytar so that it shows up with its siblings in the package search. Then you could move all of the code that fetches your keytar-cli fork into one file (auth-source-keytar.el).

[jcs090218]

Done.

[riscy]

Thanks very much for accommodating -- ideally to align with the prefix rules you could just move all of the keytar.el code into auth-source-keytar.el. I still have misgivings about potential conflicts with the "original" keytar-cli; maybe you could rename your fork's shell command to emacs-keytar or something just to be safe? Otherwise the code is shaping up well.

[jcs090218]

ideally to align with the prefix rules you could just move all of the keytar.el code into auth-source-keytar.el.

I think it is better to keep it separately since it serves directly to node-keytar (module by module). Do you think I should split into another package for this goal? 😕

I still have misgivings about potential conflicts with the "original" keytar-cli; maybe you could rename your fork's shell command to emacs-keytar or something just to be safe? Otherwise the code is shaping up well.

I attempt to serve the official keytar so I would like to keep as is it. And if Atom's developer willing to create their own official cli then I might switch it over and delete my own fork. Furthermore, I think by having scope wrapped is enough, @emacs-grammarly.

[riscy]

I think it is better to keep it separately since it serves directly to node-keytar (module by module). Do you think I should split into another package for this goal? 😕

No, I think they belong together. I'm afraid I don't understand your rationale that it serves directly to node-keytar (couldn't the same be said about auth-source-keytar?) as a reason for not including the code under the same namespace.

I attempt to serve the official keytar so I would like to keep as is it.

After scrolling around in the code I see now that the installation step is specifying --prefix, so it won't interfere with the user's PATH like I wrote above. That's fine then.

[jcs090218]

I was thinking to keep the module separate just to keep the namespace to keytar and not auth-source-keytar, but it's fine. I have made the changes!

[riscy]

I was thinking to keep the module separate just to keep the namespace to keytar and not auth-source-keytar

On further reading it seems like there is some precedent for what you want to do here. For example, there's auth-source-pass (now included in Emacs) and the password-store package. But I do think before going the route of having two packages you should make sure the auth-source approach isn't accomplishing everything you need it to.

Your code looks great now re: the checklist, but I'm having some trouble when I test it out. For example:

(auth-source-pick-first-password :host "[email protected]")
=> "great-passw0rd"

(auth-source-keytar-enable)
(auth-source-pick-first-password :host "[email protected]")
=> Wrong type argument: (or eieio-object class), (auth-source-backend "keytar" :source "." :type 'keytar :search-function #'auth-source-keytar-search), obj

Are you able to see something similar on your end?

[riscy]

Friendly ping. :)

[jcs090218]

On further reading it seems like there is some precedent for what you want to do here. For example, there's auth-source-pass (now included in Emacs) and the password-store package. But I do think before going the route of having two packages you should make sure the auth-source approach isn't accomplishing everything you need it to.

Yes, and I agree with you. :)

Are you able to see something similar on your end?

I have now fixed this issue. Please try the latest source and check to see if the issue resolves.

You might want to use :service keyword instead of :host? Is keyword :host the standard auth-source? 😕

(auth-source-keytar-enable)
(auth-source-pick-first-password :service "hello")

[jcs090218]

Forgot to mention, I have split (revert) the package into two. I reckon keeping the namespace on it's own would be easier for me to maintain. Like the example Chris brought up, auth-source-pass and password-store.

https://github.com/emacs-grammarly/keytar
https://github.com/emacs-grammarly/auth-source-keytar

[riscy]

You might want to use :service keyword instead of :host? Is keyword :host the standard auth-source? 😕

I'm not an auth-source expert and am getting caught up as I review this PR, but if you read the help for auth-source-search it says:

Common search keys are :max, :host, :port, and :user. In
addition, :create specifies if and how tokens will be created.
Finally, :type can specify which backend types you want to check.

so probably you want to support those. I'm curious, where did you read that :service was standard?

In the meantime, one small byte-compile warning which may point to an issue:

byte-compile (using Emacs 27.1):

auth-source-keytar.el:75:1:Warning: Unused lexical argument `account'

[jcs090218]

so probably you want to support those. I'm curious, where did you read that :service was standard?

:service is from keytar, they use service, account, and password. I have applied :host keyword so it should work with auth-source's standard.

byte-compile (using Emacs 27.1):

Fixed! Thanks!

[riscy]

Hello, I had another look at the code and it looks like auth-source-keytar.el cannot be loaded (even after successful byte-compilation) because defvar auth-source-keytar-backend tries to reference the function auth-source-keytar-search before it has been defined.

[jcs090218]

Sorry for the issue here. I am also having the same issue when declaring it using defvar, now should be fixed.

I was using auth-source-pass and auth-source-xoauth2 as references but they all use keyword defvar to define their backend. After that, I have looked into auth-source-kwallet and this is the only source that works for me. What could be wrong? 😕

[riscy]

What could be wrong? 😕

I will assume this is rhetorical. :) Thanks for this -- I'll merge.