Do not replace AccessDenied error messages with generic "access denied"

[gzdunek]

Closes #32550

After merging #39229 we no longer compare the error message on the Electron side to check if the error is retryable. Because of that, we don't need to convert all access denied errors to generic "access denied".
I went through all the remote calls in tshd, I didn't see any place without AddMetadataToRetryableError, so showing the error modal should work as before.

I also removed isAccessDeniedError that was doing the check on error message (I cleaned up the other functions as well).

Changelog: Teleport Connect now shows specific error messages instead of generic "access denied"

[ravicious]

Fantastic!

[ravicious]

I appreciate the thought, but I'm not totally sold on hiding ABORTED. AFAIK, CANCELLED is typically used for cancellation by the client.

ABORTED is meant to be used on the server side:

Service implementors can use the following guidelines to decide between FAILED_PRECONDITION, ABORTED, and UNAVAILABLE: (a) Use UNAVAILABLE if the client can retry just the failing call. (b) Use ABORTED if the client should retry at a higher level (e.g., when a client-specified test-and-set fails, indicating the client should restart a read-modify-write sequence). (c) Use FAILED_PRECONDITION if the client should not retry until the system state has been explicitly fixed.

We actually do use ABORTED and it's when tshd sends PromptMFA to the Electron app. If the user closes the modal, the Electron app responds with ABORTED (see below).

Overall I feel like we should keep ABORTED to avoid the inverse – encourage people to use CANCELLED where ABORTED would be better suited.

In the context of a client ABORTED seems important. A client will have little need to check for CANCELLED since cancellation will typically be done on client side, by e.g. aborting a signal. On the other hand, there might be scenarios where a client needs to know if the server ABORTED a call.

teleport/web/packages/teleterm/src/ui/tshdEvents.ts

Lines 76 to 85 in ad01546

	if (hasCanceledModal) {
	// Throwing an object here instead of an error to future-proof this code in case we need to
	// return more than just the error name and message.
	// Refer to processEvent in the preload part of tshd events service for more details.
	throw {
	isCrossContextError: true,
	name: 'AbortError',
	message: 'MFA modal closed by user',
	};
	}

teleport/web/packages/teleterm/src/services/tshdEvents/index.ts

Lines 202 to 211 in ad01546

	// TODO(ravicious): This is just an example of how cross-context errors can be signalled.
	// A more elaborate implementation should use a TypeScript assertion function
	// (isCrossContextError) and them somehow automatically build common gRPC status errors.
	// https://github.com/gravitational/teleport.e/issues/853
	// https://github.com/gravitational/teleport/issues/30753
	if (error['isCrossContextError'] && error['name'] === 'AbortError') {
	responseErr = new grpc.StatusBuilder()
	.withCode(grpc.status.ABORTED)
	.withDetails(error['message']);
	}

teleport/lib/teleterm/daemon/mfaprompt.go

Lines 93 to 97 in ad01546

	// If the user closes the modal in the Electron app, we need to be able to cancel the other
	// goroutine as well so that we stop waiting for the hardware key tap.
	if err != nil && status.Code(err) == codes.Aborted {
	cancel(err)
	}

[gzdunek]

That makes sense. I will also add UNAUTHENTICATED, I think there is no justification for omitting a single value.

[ravicious]

I'd add a line explaining that it's meant to be used to check errors received from a gRPC client that crosses context bridge.

If someone looks at this function and they're not aware of how gRPC clients and context bridge interact together, it might be tempting to use this function for other purposes, e.g. somewhere within tshd events backend implementation. But that would just lead someone to a dead end.

[gzdunek]

Right. I rephrased the messaged a bit:

It's meant to be used to check errors received from a gRPC client that was produced with cloneClient().

It's because we clone the gRPC client in the main process too (so we can check for isResolvableWithRelogin if we need to).

[ravicious]

Thanks to your work on gRPC clients and errors we can fix a bug here.

If we just catch all PERMISSION_DENIED errors, we're going to catch retryable PERMISSION_DENIED errors as well and retryWithRelogin won't be triggered.

              if (
                isTshdRpcError(error, 'PERMISSION_DENIED') &&
                !error.isResolvableWithRelogin
              ) {
                throw new Error(
                  `Cannot set up the role: ${error.message}. Contact your administrator for permissions to manage users and roles.`
                );
              }

[gzdunek]

Thanks, fixed!

[public-teleport-github-review-bot]

@gzdunek See the table below for backport results.

Branch	Result
branch/v15	Failed