Skip to content
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

Improve the API #4

Open
teythoon opened this issue Apr 13, 2020 · 1 comment
Open

Improve the API #4

teythoon opened this issue Apr 13, 2020 · 1 comment
Labels
enhancement New feature or request

Comments

@teythoon
Copy link
Contributor

I see the following problems with the API:

  • It is a bit of a leaky abstraction. Note, for example, how we couldn't change the crypto library without changing the API. Similarly, since almost all functions are public, it is impossible to change the structure of the implementation without breaking the API.
  • The API is needlessly complex. It exposes intermediate steps of the algorithm (i.e. compute the entropy first, then compute the password from it). The cli make use of this, so I think it is for the benefit of the cli.
  • However, this also complicates the cli. And while it may be neat for playing with the implementation, I don't see a use case for exposing the two-step computation in the cli. On the contrary, experience has shown that complex user interfaces have a cost, and for security tools they may adversely affect the usability and with it increase the chance of making mistakes. In fact, I think the cli should simply be a drop-in replacement for the other LessPass implementations.
  • Finally, the API lacks a simple way of computing the password. I imagine sth along the lines of
CharacterSet::default().generate("password", "site", 1)
@71 71 added the enhancement New feature or request label Apr 14, 2020
@71
Copy link
Owner

71 commented Apr 14, 2020

  • Thanks to your PR, I don't think it's a problem anymore. The API no longer exposes any external type.
  • I personally think that it's better to expose those methods, just in case (for instance, I had to fork an entire library and replace a true by false a few days ago just because a single function was not exposed and I therefore couldn't tweak the behavior like I wanted). We could however put those functions in another module like internal that we expose while explicitly stating that we make no guarantees on the compatibility of this API going forward.
  • I agree as well, but my point above still stands. In my case, I needed the fingerprint from which I derived colors, similarly to what the LessPass app does. Knowing the first few characters of the fingerprint is a good way to ensure you did not make a typo in the master password without displaying it on the screen.
    • Despite this, you make a good point. More features may make it harder to use. I'd suggest making the API closer to the other implementations (or equivalent), but with "advanced" and explicit flags like --generate-fingerprint.
  • I agree, a more general function that does everything at once should be provided. I however don't like the idea of making it a method of CharacterSet since the focus should be on generating a password rather than on creating and consuming a CharacterSet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants