Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
tylerccarson
left a comment
tylerccarson
left a comment
There was a problem hiding this comment.
Overall, adding a more user-friendly way to using the keeperapi package is a great idea. Going forward, we should treat everything in @keeper-security/keeperapi as the core library (which is primarily used and maintained by the browser extensions team), and add a new npm package (@keeper-security/keeper-sdk-javascript) intended for the public audience that's a convenient wrapper of the core library.
I've added some code-level feedback to the proposed additions here, please take a look at those.
Other than that:
- we use
main, notrelease(not sure who pushedrelease?). PR should be re-targeted - not sure if you want to do this yet, but ultimately we'll need to setup an npm workflow that publishes just @keeper-security/keeper-sdk-javascript. I can work on creating the package on the npm side when the time comes.
- the additions are Node-centric, but I have a feeling there will be users who want to use this package in the browser as well. So 1) does this work in the browser and 2) if so, there should be examples that demonstrate a little bit
| @@ -0,0 +1,22 @@ | |||
| { | |||
| "name": "keeper-sdk", | |||
There was a problem hiding this comment.
We're going to publish as a separate package called @keeper-security/keeper-sdk-javascript-- name here should be updated accordingly
| "types:ci": "tsc" | ||
| }, | ||
| "dependencies": { | ||
| "@keeper-security/keeperapi": "17.1.0", |
There was a problem hiding this comment.
no change needed now-- but I will probably explore more formally re-organizing the project into npm workspaces after this. Then the packages will be implicitly linked, and share node_modules
| @@ -0,0 +1,36 @@ | |||
| export const SdkDefaults = { | |||
| CLIENT_VERSION: 'c17.0.0', | |||
There was a problem hiding this comment.
Not sure about whether we want to identify ourselves as Commander here with the c client version prefix
I've spoken with the backend team in the past already about adding a dedicated client version prefix for the js-sdk (i.e. js17.0.0), but not sure if any progress has been made.
@saldoukhov is that something you could help with? Or maybe @jgrein-keeper?
| return err.message || err.result_code || err.error || 'Unknown Keeper error' | ||
| } | ||
| if (err instanceof Error) return err.message | ||
| if (typeof err === 'string') return err |
There was a problem hiding this comment.
Could be a JSON string-- make want to attempt parsing as such first
| } catch {} | ||
| } | ||
| } | ||
| if (typeof err === 'string') return err |
| } | ||
|
|
||
| private static base64urlDecode(str: string): Buffer { | ||
| return Buffer.from(str.replace(/-/g, '+').replace(/_/g, '/'), 'base64') |
There was a problem hiding this comment.
use normal64Bytes from core lib
|
|
||
| enum ResultCode { | ||
| Success = 'success', | ||
| OK = 'OK', |
There was a problem hiding this comment.
What is the difference between Success and OK?
| history: HistoryEntry[] | ||
| } | ||
|
|
||
| type RecordHistoryRequest = { |
There was a problem hiding this comment.
This and associated types should be moved to the code lib (src/commands.ts).
| return { recordUid, history } | ||
| } | ||
|
|
||
| function normalizeBase64(source: string): string { |
There was a problem hiding this comment.
use core lib function
|
|
||
| ## Prerequisites | ||
|
|
||
| - Node.js 16+ |
There was a problem hiding this comment.
Node 16 is pretty outdated, have you run these against it to be sure? We may want a specify a more recent minimum node version and not recommend something that probably has unpatched security vulnerabilties
2 participants
Login & Records Implementation
Summary
Adds KeeperSdk wrapper and interactive CLI examples for authentication and vault record operations.
Authentication
~/.keeper/config.json.Records
Sharing