Skip to content

Commit

Permalink
Do not replace AccessDenied error messages with generic "access den…
Browse files Browse the repository at this point in the history
…ied" (#39558)

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

* Replace `error.code` checks with `isTshdRpcError(error, code)`

* Add `ABORTED` and `UNAUTHENTICATED` codes

* Improve `isTshdRpcError` documentation

* Do not catch `retryWithRelogin` errors in `Setup.tsx`

(cherry picked from commit 48c05b6)
  • Loading branch information
gzdunek committed Mar 22, 2024
1 parent ab12a29 commit 7d26e66
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 45 deletions.
16 changes: 0 additions & 16 deletions lib/teleterm/apiserver/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,10 @@ package apiserver

import (
"context"
"errors"

"github.com/gravitational/trace"
"github.com/gravitational/trace/trail"
"github.com/sirupsen/logrus"
"google.golang.org/grpc"

"github.com/gravitational/teleport/api/client"
)

// withErrorHandling is gRPC middleware that maps internal errors to proper gRPC error codes
Expand All @@ -41,18 +37,6 @@ func withErrorHandling(log logrus.FieldLogger) grpc.UnaryServerInterceptor {
resp, err := handler(ctx, req)
if err != nil {
log.WithError(err).Error("Request failed.")
// A stop gap solution that allows us to show a relogin modal when we
// receive an error from the server saying that the cert is expired.
// Read more: https://github.com/gravitational/teleport/pull/38202#discussion_r1497181659
// TODO(gzdunek): fix when addressing https://github.com/gravitational/teleport/issues/32550
if errors.Is(err, client.ErrClientCredentialsHaveExpired) {
return resp, trail.ToGRPC(err)
}

// do not return a full error stack on access denied errors
if trace.IsAccessDenied(err) {
return resp, trail.ToGRPC(trace.AccessDenied("access denied"))
}
return resp, trail.ToGRPC(err)
}

Expand Down
32 changes: 31 additions & 1 deletion web/packages/teleterm/src/services/tshd/cloneableClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ import {
MethodInfo,
} from '@protobuf-ts/runtime-rpc';

import { cloneAbortSignal, TshdRpcError, cloneClient } from './cloneableClient';
import {
cloneAbortSignal,
TshdRpcError,
cloneClient,
isTshdRpcError,
} from './cloneableClient';

function getRpcError() {
return new RpcError('You do not have permission.', 'ACCESS_DENIED');
Expand Down Expand Up @@ -251,3 +256,28 @@ test('response error is cloned as an object in a duplex call', async () => {
expect(Object.getPrototypeOf(error).constructor).toEqual(Object);
expect(error).toMatchObject(tshdRpcErrorObjectMatcher);
});

test.each([
{
name: 'is not a tshd error',
errorToCheck: { name: 'Error' },
statusCodeToCheck: undefined,
expectTshdRpcError: false,
},
{
name: 'is a tshd error',
errorToCheck: { name: 'TshdRpcError' },
statusCodeToCheck: undefined,
expectTshdRpcError: true,
},
{
name: 'is a tshd error with a status code',
errorToCheck: { name: 'TshdRpcError', code: 'PERMISSION_DENIED' },
statusCodeToCheck: 'PERMISSION_DENIED',
expectTshdRpcError: true,
},
])('$name', testCase => {
expect(
isTshdRpcError(testCase.errorToCheck, testCase.statusCodeToCheck)
).toBe(testCase.expectTshdRpcError);
});
44 changes: 41 additions & 3 deletions web/packages/teleterm/src/services/tshd/cloneableClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,9 +254,47 @@ export type TshdRpcError = Pick<
isResolvableWithRelogin: boolean;
};

/** Checks if the given value is a `TshdRpcError`. */
export function isTshdRpcError(error: unknown): error is TshdRpcError {
return error['name'] === 'TshdRpcError';
/**
* gRPC status codes.
* @see https://grpc.io/docs/guides/status-codes
*/
type RpcStatusCode =
| 'CANCELLED'
| 'UNKNOWN'
| 'INVALID_ARGUMENT'
| 'DEADLINE_EXCEEDED'
| 'NOT_FOUND'
| 'ALREADY_EXISTS'
| 'PERMISSION_DENIED'
| 'RESOURCE_EXHAUSTED'
| 'FAILED_PRECONDITION'
| 'ABORTED'
| 'OUT_OF_RANGE'
| 'UNIMPLEMENTED'
| 'INTERNAL'
| 'UNAVAILABLE'
| 'DATA_LOSS'
| 'UNAUTHENTICATED';

/**
* Checks if the given value is a `TshdRpcError`.
* It's meant to be used to check errors received from a gRPC client
* that was produced with `cloneClient()`.
* @param error - Error to check.
* @param statusCode - Optionally, a gRPC status code to compare.
*/
export function isTshdRpcError(
error: unknown,
statusCode?: RpcStatusCode
): error is TshdRpcError {
const isTshdRpcError = error['name'] === 'TshdRpcError';
if (!isTshdRpcError) {
return false;
}
if (statusCode) {
return (error as TshdRpcError).code === statusCode;
}
return true;
}

function cloneError(error: unknown): TshdRpcError | Error | unknown {
Expand Down
21 changes: 2 additions & 19 deletions web/packages/teleterm/src/services/tshd/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,7 @@

import { isTshdRpcError } from './cloneableClient';

export function isAccessDeniedError(error: unknown): boolean {
// TODO(gzdunek): Replace it with check on the code field.
if (isTshdRpcError(error)) {
return error.message.includes('access denied');
}
return false;
}

export function isNotFoundError(error: unknown): boolean {
if (isTshdRpcError(error)) {
return error.code === 'NOT_FOUND';
}
return false;
}

/** @deprecated Use `isTshdRpcError(error, 'UNIMPLEMENTED')`. */
export function isUnimplementedError(error: unknown): boolean {
if (isTshdRpcError(error)) {
return error.code === 'UNIMPLEMENTED';
}
return false;
return isTshdRpcError(error, 'UNIMPLEMENTED');
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
useConnectMyComputerContext,
} from 'teleterm/ui/ConnectMyComputer';
import { codeOrSignal } from 'teleterm/ui/utils/process';
import { isAccessDeniedError } from 'teleterm/services/tshd/errors';
import { isTshdRpcError } from 'teleterm/services/tshd/cloneableClient';
import { useResourcesContext } from 'teleterm/ui/DocumentCluster/resourcesContext';
import { DocumentConnectMyComputer } from 'teleterm/ui/services/workspacesService';

Expand Down Expand Up @@ -238,9 +238,12 @@ function AgentSetup() {
await ctx.connectMyComputerService.createRole(rootClusterUri);
certsReloaded = response.certsReloaded;
} catch (error) {
if (isAccessDeniedError(error)) {
if (
isTshdRpcError(error, 'PERMISSION_DENIED') &&
!error.isResolvableWithRelogin
) {
throw new Error(
'Access denied. Contact your administrator for permissions to manage users and roles.'
`Cannot set up the role: ${error.message}. Contact your administrator for permissions to manage users and roles.`
);
}
throw error;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ import { wait } from 'shared/utils/wait';
import { RootClusterUri, routing } from 'teleterm/ui/uri';
import { useAppContext } from 'teleterm/ui/appContextProvider';
import { Server } from 'teleterm/services/tshd/types';
import { cloneAbortSignal } from 'teleterm/services/tshd/cloneableClient';
import { isNotFoundError } from 'teleterm/services/tshd/errors';
import {
cloneAbortSignal,
isTshdRpcError,
} from 'teleterm/services/tshd/cloneableClient';
import { useResourcesContext } from 'teleterm/ui/DocumentCluster/resourcesContext';
import { useLogger } from 'teleterm/ui/hooks/useLogger';

Expand Down Expand Up @@ -281,7 +283,7 @@ export const ConnectMyComputerContextProvider: FC<
rootClusterUri
);
} catch (error) {
if (isNotFoundError(error)) {
if (isTshdRpcError(error, 'NOT_FOUND')) {
return;
}
throw error;
Expand Down

0 comments on commit 7d26e66

Please sign in to comment.