feat: support file storage for provisioned tokens in OSes != darwin #202
Conversation
You forgot the suggestion 😛 |
src/utils/token_storage/fs.ts
Outdated
| try { | ||
| const info = await Deno.lstat(credentialsPath); | ||
| if (!info.isFile || (info.mode !== null && (info.mode & 0o777) !== 0o600)) { | ||
| throw new Error("The credentials file have have been tampered with."); |
There was a problem hiding this comment.
If the user ends up in this situation (hopefully never), it is already a messy situation and the error message is very cryptic. It would be useful to split the sanity-checks and report actionable error details for each separately, only mentioning the tampering bit on the side (if at all).
There was a problem hiding this comment.
My rationale is that users are not expected to touch this file at all, and I want the error message to convey this. I don't want users to try and fix the issue, but rather to remove whatever is in there and let deployctl do its thing. Maybe I should extend the error message with this explicit action call?
Also keep in mind that upon error, the program provisions a new token and stores it in memory for the duration of the execution, thus the UX does not get blocked by a possible error anyway.
There was a problem hiding this comment.
(I don't want to stall the PR on this, so feel free to bypass this discussion if you prefer)
Maybe I should extend the error message with this explicit action call?
If that's the expected action, then maybe yes (or fixing the permissions directly?).
The closest think I can think of is the SSH client sanity-checking private-key permissions, in which case it just says the following (but keeps going):
Permissions for '<key-file>' are too open.
It is required that your private key files are NOT accessible by others.
This private key will be ignored.
I guess you can model the error-message here after that, replacing the last line with the manual action you'd expect from the user.
There was a problem hiding this comment.
(I don't want to stall the PR on this, so feel free to bypass this discussion if you prefer)
This is UX, I think it's important to discuss it. And you make very good points.
Here's an updated version:
Thoughts?
There was a problem hiding this comment.
Ack. Did you avoid mentioning the filepath for some reason on purpose? Or why not saying the credentials file at <filepath>...? Other than that, let's land this.
There was a problem hiding this comment.
On second thought, I'd like to keep the message clean. This is an edge case, I'd prefer to be reactive about it.
|
@arnauorriols it got somehow lost in a black-hole while submitting the review, apologies. |
resolves #182