Closed CMCDragonkai closed 3 years ago
# parameter style
pk agent unlock 'rootpassword'
# prompt style
pk agent unlock
> ...
> EOF
# file style
pk agent unlock -pf ./file
For all CLI commands, the exact options -pf
should always have a long form like --password-file
.
When running pk agent unlock
twice or more, it's idempotent.
pk agent unlock '...'
pk agent unlock '...'
Basically a new token is created.
When unlocking the agent and creating a session, you should show the information about the session. In particular show that the session has an expiry date.
One question do you refresh the session on each command. So if you do:
pk agent unlock '...'
pk vaults list
pk vaults list
Is each pk vault list
refreshing the session? Ideally it should be, that way the more you use the client, you don't expire it inadvertently.
However if you are refreshing the session each time, you have you write the session file onto the disk.
Note that when you do this, you may clobber the file or be writing at the same time as some other command.
pk vault list &
pk vault list &
wait
The above will run the command twice at the same time. We would want to avoid refreshing the session file and clobbering each other.
One way to resolve this is to acquire a lock on the session file. This very similar to the lockfile being used by the agent to ensure that one node state has one agent instance.
You can have a procedure like:
You will also need to resolve how to lock the file if the file doesn't exist.
Perhaps you can use the same lockfile mechanism and abstract it for this usecase?
Also @DrFacepalm please ensure you have a mutex in your SessionManager
and wrap all your public API commands with await this._transaction(...)
.
You have to beware of concurrency here, all our code is running asynchronously!
https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/189 should fix everything except for making normal commands refresh the session, and dealing with the concurrency issues that come with that.
Already merged, so this just requires review.
@tegefaulkes I think since you are going to be rebasing on the client-refactoring to make use of this for identities CLI, your comments are going to be relevant.
@joshuakarp since you're starting on the Nodes CLI. You can check basically whether everything here is addressed, and if not, the remaining issue is #211 (automatic session refresh on commands and expiry information).
If so, then this issue can be closed. Otherwise subissues should be created.
So for the bin CLI commands, they should require session authentication now.
This means for all the tests/bin
tests, they should have a fixture like beforeAll
or beforeEach
, there needs to be an a session authentication with pk agent unlock
, and then using that token for all subsequent CLI commands.
Instead of relying on a default file path for the session token, it should be able to be passed in directly via a command line parameter or an environment variable, or a path to a file containing the session information.
@joshuakarp best done while you're working on the Nodes CLI: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/198
@tegefaulkes might have to add some additional stuff to https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/201
Some review notes for https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/189:
prompts
packagetests/utils.test.ts
has some unnecessary imports, should be fixed up later in a lintfix.tests/sessions/SessionManager.ts
should be described with SessionManager
.src/sessions
not src/session
. You are using sessions
in other places to describe this domain. This should be standardised.4096
not 4069
. The 4096
is the correct size.tests/client/clientService.test.ts
constructs callCredentials
manually. This should either be a function available inside src/client/utils.ts
or if it is test-specific, it goes into tests/client/utils.ts
. This construction looks complicated and should be abstracted.credentials
property for openTestClientClient
in tests/client/utils.ts
necessary? How does this interact with the call credentials?Session.ts
the tests is currently using them. This is currently affecting all tests involving sessions and also the source code.Session
class represents a client that constructs it (when receiving the underlying token from the client service), and also the SessionManager
that constructs the object from the agent side (when the client wants to authenticate). It's just an abstraction, the tests for Session
doesn't seem to accurately represent this abstraction.lock
, lockall
, and unlock
commands, we can reduce the complexity.src/client/types.ts
instead of just using: let callCredentials: Partial<grpc.CallOptions>;
.bin
tests are going to require some form of session-management token related tests. However I don't think this is necessary. For the bin
tests I believe all session management should be done as just a fixture. It would having an n+1 problem which every command we need a session token specific tests. It should just be assumed to work. Instead you have tests/bin/agent.test.ts
for session-token specific tests, and put them all there specifying the behaviour.tests/bin/agent.test.ts
should be more descriptive and use our abstraction representations as keywords. Like "unlocking by constructing a session"
or "locking by destroying the session" or "lockall by destroying all sessions through changing the session key"
.generateUserToken
for in src/utils.ts
?stateVersion
? Right now we don't do anything different, but the stateVersion
should be checked. That is we need to check if the state version is ahead or behind the current state version that's programmed for the Polykey. So src/config.ts
means this is our current state version. There should be functions that check this against the lock file in our PK node state. Which is why Lockfile
is sort of a strange name, it is more specific to our usecase, unless we abstract them into PolykeyAgent
and PolykeyClient
(and can use it for both session tokens and polykey node state). I'd call it the StateFile
and document its generic use.unlock
, or we can read it from a file in the PK client state. Usually we would program the read from file to the async start method of Session
. But I can see that would conflict from receiving the grpc client service. So if the Session
is constructed with a passed in token, then it shouldn't have an async start or stop method at all, it's just a class wrapped POJO and should only have a constructor. In that sense it's only a slightly more advanced Opaque
type with additional methods. Alternatively when you start the Session
, if you pass in the token, then it just uses that, but if you don't it will perform the actual read itself.src/session/errors.ts
errors can be more abstractly named. No need to mention JWT. That's an implementation detail.Client.proto
should have a naming standardisation process applied. New issue for this.src/client/utils.ts
should really be in src/session/utils.ts
. But you can keep functions that transform from call credentials and back there... Furthermore the usage of throwing exceptions in the utility functions should be minimised, it deally utility functions are pure and return true/false or whatever, and the throwing of exceptions should be the domain classes.utils.verifyToken
call should be inverted.lock
, unlock
and lockall
. But the client service has sessionRequestJWT
and sessionChangeKey
. You can do something like sessionUnlock
, sessionLock
and sessionLockAll
. The JWT is an implementation detail. The usage of password is an implementation detail. Functions/commands like sessionUnlock
should just be requesting the password
as a parameter.Session
should be tied to the creation of the actual session file... but that's point 16 anyway.Changes addressing the above should go into client-refactoring
.
This will go into new issue.
Closing this, review points going to #211.
Specification
Additional context
This is all done as part of client-refactoring for now.
Tasks
pk agent unlock
- for authenticating a session for the CLIpk agent lock
- for destroying the current session for the CLI (delete client's session token)pk agent lockall
- for changing the session key and ensuring that all sessions should be made invalid