-
Notifications
You must be signed in to change notification settings - Fork 0
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
SSH requesting access message #137
SSH requesting access message #137
Conversation
src/commands/shared/request.ts
Outdated
message?: "all" | "approval-required" | "none"; | ||
} | ||
): Promise<RequestResponse<T> | undefined> => { | ||
const resolvedAuthn = authn ?? (await authenticate()); | ||
const { userCredential } = resolvedAuthn; | ||
const data = await spinUntil( | ||
"Requesting access", | ||
options?.accessMessage ? options.accessMessage : "Requesting access", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be dependent on the type union? "all" | "approval-required" | "none"
So if we see: "approval-required" we can still say: "requesting access" 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since "approval-required"
is hardcoded for the ssh session request, I don't think we can use it to change the message in this case. I think the request would still have approval-required
even if the user already had been approved in the past?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I see 🤔 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make it dependent like this:
const accessMessage = (message?: string) => {
switch (message) {
case "approval-required":
return "Requesting access";
default:
return "Requesting access";
}
};
then here
options?.accessMessage ? options.accessMessage : "Requesting access", | |
accessMessage(options?.message),, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change, but the "approval-required"
message type will return an accessMessage of Checking for access in P0
.
This makes this change apply to p0 kubeconfig
as well which I think is desirable since it's similar flow to starting an ssh session.
const accessMessage = (message?: string) => {
switch (message) {
case "approval-required":
return "Checking for access in P0";;
default:
return "Requesting access";
}
};
src/commands/shared/request.ts
Outdated
message?: "all" | "approval-required" | "none"; | ||
} | ||
): Promise<RequestResponse<T> | undefined> => { | ||
const resolvedAuthn = authn ?? (await authenticate()); | ||
const { userCredential } = resolvedAuthn; | ||
const data = await spinUntil( | ||
"Requesting access", | ||
options?.accessMessage ? options.accessMessage : "Requesting access", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make it dependent like this:
const accessMessage = (message?: string) => {
switch (message) {
case "approval-required":
return "Requesting access";
default:
return "Requesting access";
}
};
then here
options?.accessMessage ? options.accessMessage : "Requesting access", | |
accessMessage(options?.message),, |
Co-authored-by: Gergely Dányi <gergas3@gmail.com>
…isplayed-when-it-shouldnt' of github.com:p0-security/p0cli into michaeldimitras/eng-2827-requesting-access-spinner-is-displayed-when-it-shouldnt
This PR makes two changes to the status messages we use for SSH requests in the CLI.
This only affects
p0 ssh <destination>
andp0 scp <destination>
. It does not affect anyp0 request
flows.Before:
4333.mov
After:
11111.mov
Addresses: https://linear.app/p0-security/issue/ENG-2827/requesting-access-spinner-is-displayed-when-it-shouldnt