Skip to content
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

Provide dedicated error message for when a token is marked as disabled due to inactivity #2084

Merged
merged 9 commits into from
Nov 25, 2024

Conversation

andrea-sdl
Copy link
Contributor

Description

This PR adds support for the new unauthorized error for inactive tokens.

Changelog Description

Adds support for the disabled token due to inactivity error so that it shows a dedicated error message.

Pull request checklist

New release checklist

Steps to Test

  1. Check out PR.
  2. Run npm run build
  3. Run VIP_PROXY="" API_HOST=http://localhost:4000 node ./dist/bin/vip app list with a token that is disabled due to inactivity
  4. Check the error message is 'Unauthorized: Your token has been disabled due to inactivity, please logout with vip logout then try again.'
  5. Repeat the same test with a valid token and an expired token, to ensure previous behavior hasn't changed.

Copy link
Contributor

github-actions bot commented Nov 6, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

@andrea-sdl andrea-sdl changed the title WIP Provide dedicated error message for when a token is marked as disabled due to inactivity Provide dedicated error message for when a token is marked as disabled due to inactivity Nov 7, 2024
@andrea-sdl andrea-sdl requested a review from a team November 7, 2024 15:40
@andrea-sdl andrea-sdl marked this pull request as ready for review November 7, 2024 15:40
Copy link
Member

@mjangda mjangda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor wording changes, otherwise this is great.

src/lib/api.ts Outdated
);
let message =
'You are unauthorized to perform this request, please logout with `vip logout` then try again.';
if ( 'result' in networkError && networkError.result?.code === 'token-disabled-inactivity' ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the in check actually necessary since we have ??

Suggested change
if ( 'result' in networkError && networkError.result?.code === 'token-disabled-inactivity' ) {
if ( networkError.result?.code === 'token-disabled-inactivity' ) {

Copy link
Contributor Author

@andrea-sdl andrea-sdl Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjangda We need the result in to make TS aware we're using the class ServerError instead of ServerParseError from the Apollo Client.

import { ServerError } from '../utils';
import { ServerParseError } from '../http';
export interface ErrorResponse {
    graphQLErrors?: ReadonlyArray<GraphQLError>;
    networkError?: Error | ServerError | ServerParseError;
    [....]
}

If we use your code the type checker will error out with

 Property 'result' does not exist on type 'ServerParseError'.

42    if ( networkError.result?.code === 'token-disabled-inactivity' ) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thank you for explaining that.

Maybe we should make the check more obvious to indicate that?

Suggested change
if ( 'result' in networkError && networkError.result?.code === 'token-disabled-inactivity' ) {
if ( networkError instanceof ServerError && networkError.result?.code === 'token-disabled-inactivity' ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work either (I tested before landing with the result in) since we're checking against a type (ServerError) and not a class.

> tsc

src/lib/api.ts:43:33 - error TS2359: The right-hand side of an 'instanceof' expression must be either of type 'any', a class, function, or other type assignable to the 'Function' interface type, or an object type with a 'Symbol.hasInstance' method.

43    if ( networkError instanceof ServerError && networkError.result?.code === 'token-disabled-inactivity' ) {
                                   ~~~~~~~~~~~

src/lib/api.ts:43:61 - error TS2339: Property 'result' does not exist on type 'ServerError | ServerParseError'.
  Property 'result' does not exist on type 'ServerParseError'.

43    if ( networkError instanceof ServerError && networkError.result?.code === 'token-disabled-inactivity' ) {
                                                               ~~~~~~


Found 2 errors in the same file, starting at: src/lib/api.ts:43

What we can do, if we want to improve the readability, is to have a isServerError function with the same check @mjangda

something like

function isServerError(networkError){
return 'result' in networkError;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even more interesting :)

I think the readability would be nice improvement but it's very minor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 0d8eefe . For future reference here's also the docs around using in operator narrowing: https://www.typescriptlang.org/docs/handbook/2/narrowing.html#the-in-operator-narrowing

src/lib/api.ts Outdated Show resolved Hide resolved
src/lib/api.ts Outdated Show resolved Hide resolved
@andrea-sdl andrea-sdl merged commit d61e0c7 into trunk Nov 25, 2024
16 checks passed
@andrea-sdl andrea-sdl deleted the add/inactive-token-error-support branch November 25, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants