Automatic session refresh and expiry information & Sessions Domain Refactoring According to Review

[DrFacepalm]

(I created this issue using the 'reference in new issue' feature)

Currently being handled in: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/202

Expiry Date / Time

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.

Automatic session refresh

The session should refresh every time a command is done.

(The below is from a previous issue)

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:

• Lock the file
• If yes, proceed to write new session
• If you couldn't get the lock, assume that another PK client process is refreshing it, and don't bother doing i

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?

Originally posted by @CMCDragonkai in #204 (comment)

[CMCDragonkai]

Try to structure it with the new issue templates.

[CMCDragonkai]

See if some of this might be done in Nodes CLI MR @joshuakarp

[CMCDragonkai]

Before working on this, please address these review points.

Some review notes for https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/189:

1. This MR brought in the prompts package
2. The tests/utils.test.ts has some unnecessary imports, should be fixed up later in a lintfix.
3. The tests/sessions/SessionManager.ts should be described with SessionManager.
4. It should be src/sessions not src/session. You are using sessions in other places to describe this domain. This should be standardised.
5. The bit size for session manager should be 4096 not 4069. The 4096 is the correct size.
6. The 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.
7. Was the removal of the credentials property for openTestClientClient in tests/client/utils.ts necessary? How does this interact with the call credentials?
8. Do not use sigchain claim type in Session.ts the tests is currently using them. This is currently affecting all tests involving sessions and also the source code.
9. The 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.
10. I don't it's necessary to have the aliases for lock, lockall, and unlock commands, we can reduce the complexity.
11. We should abstract the call credential into our credential-specific type into src/client/types.ts instead of just using: let callCredentials: Partial<grpc.CallOptions>;.
12. All the 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.
13. The test groups and test descriptions for 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".
14. What is the function generateUserToken for in src/utils.ts?
15. Are there tests for testing the checking of 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.
16. When constructing the session, we can either receive the token from the grpc client service during 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.
17. The src/session/errors.ts errors can be more abstractly named. No need to mention JWT. That's an implementation detail.
18. Try to ensure there's only one right way, one right place to do anything that constructing call credentials.. etc.
19. The Client.proto should have a naming standardisation process applied. New issue for this.
20. I feel like the password checking functions in 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.
21. Utility functions should not be injected with the domain class. Utility functions should be pure and easy to use. So utils.verifyToken call should be inverted.
22. We should preserve the naming of the actions across CLI commands and GRPC service commands as much as possible. We have 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.
23. The lifetime of the Session should be tied to the creation of the actual session file... but that's point 16 anyway.

[CMCDragonkai]

Also added @tegefaulkes on this issue as these changes some of them can be placed directly into client-refactoring, and some can go into the nodes CLI.

[CMCDragonkai]

On 16. the Session is only intended to represent the lifetime of a session on the client side. Therefore its async start stop should also deal with the construction of the file on disk, as well as using the lock file mechanism to prevent clobbering.

[DrFacepalm]

Another point to address:

There still exists several instances of @/ aliases in src.

• Replace them with relative referencing.

[CMCDragonkai]

I thought these were addressed ages ago lol.

…

On 27 July 2021 11:47:09 am AEST, DrFacepalm ***@***.***> wrote: Another point to address: There still exists several instances of `@/` aliases in `src`. * [ ] Replace them with relative referencing. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #211 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[DrFacepalm]

I thought these were addressed ages ago lol.
…
On 27 July 2021 11:47:09 am AEST, DrFacepalm @.***> wrote: Another point to address: There still exists several instances of @/ aliases in src. * [ ] Replace them with relative referencing. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #211 (comment)
-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

Apparently some new ones popped up

[DrFacepalm]

More things:

• We need to lock the token file. If we find that the thing is already locked, we just ignore cause some new thing is already refreshing it.

[DrFacepalm]

Things left for this issue:

• Standardization of bin tests
• Make Session.ts start method actually perform a read from file on start, if a token is not provided
• Create a lock for the token file that works
• "We should preserve the naming of the actions across CLI commands and GRPC service commands as much as possible. We have 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."
• Update GRPC calls to match Standardize the naming of GRPC Functions/Commands and Message Types #218 (comment)

[DrFacepalm]

There is currently an issue with this test:

[CMCDragonkai]

Has this been updated with the fact that Keys now uses DB?

…

On 8/2/21 4:47 PM, DrFacepalm wrote: There is currently an issue with this test: image <https://user-images.githubusercontent.com/31267481/127816128-f5093c0e-3cda-44cc-943c-f15dbbb9d5e7.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#211 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHNBYWXICHY5YLYU5ELT2Y5R5ANCNFSM5AIJYN4A>.

[DrFacepalm]

I believe so. All the other keys tests works

[joshuakarp]

I made a comment on Slack re this a week or so ago, when gestalt-discovery was merged in:

Re the bin/keys test failing, it was passing before I rebased on client refactoring (there was 7 commits I brought in from client-refactoring after I rebased) - so it's been caused by one of these changes https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/compare/b924e0de6cd149e00b3ba7bdf32408e1441072d9...be20d9d78320cad6a4c6f7853927769908c516b6

https://matrixai.slack.com/archives/CEAUUV5QX/p1626841840454900

[CMCDragonkai]

@tegefaulkes you should take over this issue and consider the timing issues for all domains as the final polish.

[CMCDragonkai]

Remaining unticked testing issues here go to #178.

My post-merge review copied here:

• Standardize the naming for all bin command files: Standardize the naming of GRPC Functions/Commands and Message Types #218 (comment)
• CLI GRPC Client should be using direct TLS to the GRPC Client Service: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/207/diffs#note_651473935
• Wondering of src/client/ should be structured similarly to src/bin/ they both have a set of "procedures" relating to the domains.
• Check the robustness of the usage of call credentials and SessionCredentials: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/207#note_652476363 and https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/207#note_652440417
• Might as well address this at the same time: Wrapper methods in GRPCClientClient to get parameter types of the wrapped GRPC call #200

[CMCDragonkai]

@DrFacepalm commentary on?:

• When constructing the session, we can either receive the token from the grpc client service during 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.
• We should preserve the naming of the actions across CLI commands and GRPC service commands as much as possible. We have 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.

[CMCDragonkai]

MR 202 merged.

[DrFacepalm]

Usually we would program the read from file to the async start method of Session.

We DO read the token during the async start.

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.

We DO allow the token to be passed during start, and if it is not passed, it attempts to read it.

But the client service has sessionRequestJWT and sessionChangeKey. You can do something like sessionUnlock, sessionLock and sessionLockAll. The JWT is an implementation detail.

I believe I've removed all the references to JWT in the function names..