aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.44k stars 2.13k forks source link

AuthError is an unexpected type calling getCurrentUser() with no authenticated user. #13400

Closed jmarshall9120 closed 5 months ago

jmarshall9120 commented 6 months ago

Before opening, please confirm:

JavaScript Framework

Not applicable

Amplify APIs

Authentication

Amplify Version

v6

Amplify Categories

auth

Backend

Amplify Gen 2 (Preview)

Environment information

``` # Put output below this line System: OS: Windows 11 10.0.22631 CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz Memory: 15.05 GB / 31.74 GB Binaries: Node: 20.12.2 - C:\Program Files\nodejs\node.EXE Yarn: 1.22.22 - ~\AppData\Roaming\npm\yarn.CMD npm: 10.5.0 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: 124.0.6367.208 Edge: Chromium (123.0.2420.97) Internet Explorer: 11.0.22621.3527 npmPackages: %name%: 0.1.0 @aws-amplify/backend: ^1.0.2 => 1.0.2 @aws-amplify/backend-cli: ^1.0.3 => 1.0.3 @aws-sdk/client-cognito-identity-provider: ^3.577.0 => 3.577.0 @aws-sdk/client-sso-oidc: 3.575.0 => 3.575.0 (3.576.0, 3.577.0, 3.338.0) @aws-sdk/types: 3.575.0 => 3.575.0 (3.387.0, 3.398.0, 3.433.0, 3.338.0, 3.577.0) @mdi/font: ^7.4.47 => 7.4.47 @pinia/nuxt: ^0.5.1 => 0.5.1 @types/aws-lambda: ^8.10.138 => 8.10.138 (8.10.137) @unocss/reset: 0.60.2 => 0.60.2 aws-amplify: ^6.3.1 => 6.3.1 aws-amplify/adapter-core: undefined () aws-amplify/analytics: undefined () aws-amplify/analytics/kinesis: undefined () aws-amplify/analytics/kinesis-firehose: undefined () aws-amplify/analytics/personalize: undefined () aws-amplify/analytics/pinpoint: undefined () aws-amplify/api: undefined () aws-amplify/api/server: undefined () aws-amplify/auth: undefined () aws-amplify/auth/cognito: undefined () aws-amplify/auth/cognito/server: undefined () aws-amplify/auth/enable-oauth-listener: undefined () aws-amplify/auth/server: undefined () aws-amplify/data: undefined () aws-amplify/data/server: undefined () aws-amplify/datastore: undefined () aws-amplify/in-app-messaging: undefined () aws-amplify/in-app-messaging/pinpoint: undefined () aws-amplify/push-notifications: undefined () aws-amplify/push-notifications/pinpoint: undefined () aws-amplify/storage: undefined () aws-amplify/storage/s3: undefined () aws-amplify/storage/s3/server: undefined () aws-amplify/storage/server: undefined () aws-amplify/utils: undefined () aws-cdk: ^2.141.0 => 2.141.0 aws-cdk-lib: ^2.141.0 => 2.141.0 bin: 1.0.0 constructs: ^10.3.0 => 10.3.0 dist: 1.0.0 esbuild: ^0.21.2 => 0.21.2 (0.20.2) floating-vue: 5.2.2 => 5.2.2 nuxt: ^3.11.2 => 3.11.2 pinia: ^2.1.7 => 2.1.7 react: 18.3.1 => 18.3.1 react-dom: 18.3.1 => 18.3.1 terser: 5.31.0 => 5.31.0 tsx: ^4.10.2 => 4.10.2 typescript: ^5.4.5 => 5.4.5 (4.4.4, 4.9.5) unocss: 0.60.2 => 0.60.2 vite-plugin-vuetify: ^2.0.3 => 2.0.3 vue: ^3.4.27 => 3.4.27 vue-router: ^4.3.2 => 4.3.2 vuetify: ^3.6.5 => 3.6.5 npmGlobalPackages: yarn: 1.22.22 ```

Describe the bug

Using Gen2 Amplify if you call amplify.Auth.getCurrentUser() with no user set it returns this error:

'UserUnAuthenticatedException'

Despite Gen2 being built for typescript the error is actually an undocumented output of type AuthError. It is returned from here: https://github.com/aws-amplify/amplify-js/blob/d7d33da192ff610c01fcdd417460ddd79d3dcb8d/packages/auth/src/providers/cognito/apis/internal/getCurrentUser.ts#L22

And defined here: https://github.com/aws-amplify/amplify-js/blob/d7d33da192ff610c01fcdd417460ddd79d3dcb8d/packages/auth/src/providers/cognito/utils/types.ts#L33

Documentation does not list this as a possible type of error to be returned from this call: https://aws-amplify.github.io/amplify-js/api/functions/aws_amplify.auth.getCurrentUser.html

This seems to be one of those bug, meets design philosophy issues. JS errors can be very confusing, as giving an error a 'name' instead of a unique type can make them hard to trap. There is very little control over how some string writer will output your error, etc. It is my opinion that if we're going to be using typescript, errors should use unique types, or at least not have different names for a single type. I find this approach to be more consistent with strongly typed languages.

Also - the use case here should be pretty common. As far as I can see there is no way to do a login without first calling getCurrentUser() to test if a login is needed. If this is not an intended use case, and there is another way to establish a users auth status when loading the site please advise. Otherwise, I can't see pretty much every user of amplify tripping across this error.

Expected behavior

This code should work:

try {
    const authUser = await getCurrentUser()
} catch (e) {
    switch(true) {
        catch (e instanceof UserUnAuthenticatedException):
            break;
        default:
            throw e
    }
}

Right now it's necessary to catch it by string compare. Which is more JS than TS.

try {
    const authUser = await getCurrentUser()
} catch (e) {
    switch(true) {
        catch (e instanceof AuthError and e.name==='UserUnAuthenticatedException'):
            break;
        default:
            throw e
    }
}

Reproduction steps

// without  user logged in
import getCurrentUser from "aws-amplify/auth";
await getCurrentUser()

Code Snippet

// Put your code below this line.

Log output

``` // Put your logs below this line ```

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

No response

cwomack commented 6 months ago

Hey, @jmarshall9120 👋 and thank you for opening this issue. I'll mark this as a feature request for now because we are intentionally throwing the UserUnAuthenticatedException error in v6 for the getCurrentUser() API when there is no authenticated user. This was done to optimize bundle size because the solution to cast and throw a specific instance of this error (similar to how the SDK does it) creates more code.

I'll be sure to review this with the team internally and follow up with any additional questions we have, but feel free to add any additional context or questions! Thanks.

jmarshall9120 commented 6 months ago

Thanks @cwomack. I have no idea how tight your bundling requirements/goals are, but maybe you could at least update the documentation I referenced to show this error as a potential result getCurrentUser().

cwomack commented 5 months ago

@jmarshall9120, we're already tracking improvements to the error types that are given from the Cognito Service Team in issue #9104 and will consolidate this issue into that one. Feel free to add any additional context, comments, +1/thumbs up on that issue to help it gain traction... but we've already noted this issues comments/details as part of the acceptance criteria for that issue's resolution.

In the meantime, we also created a docs issue to track improvements to the API documentation as you suggested.