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.43k stars 2.12k forks source link

allow refreshSession to set deviceKey, be written to cookies #11651

Open AllanJard opened 1 year ago

AllanJard commented 1 year ago

Before opening, please confirm:

JavaScript Framework

Not applicable

Amplify APIs

Authentication

Amplify Categories

auth

Environment information

``` # Put output below this line System: OS: Linux 6.2 openSUSE Tumbleweed 20230408 CPU: (16) x64 AMD Ryzen 9 5900HX with Radeon Graphics Memory: 10.58 GB / 30.76 GB Container: Yes Shell: 5.2.15 - /bin/bash Binaries: Node: 16.14.2 - ~/.nvm/versions/node/v16.14.2/bin/node Yarn: 1.22.19 - ~/.nvm/versions/node/v16.14.2/bin/yarn npm: 8.5.0 - ~/.nvm/versions/node/v16.14.2/bin/npm Browsers: Chromium: 112.0.5615.49 npmGlobalPackages: corepack: 0.10.0 generator-code: 1.7.6 npm: 8.5.0 yarn: 1.22.19 yo: 4.3.1 ```

Describe the bug

I'm trying to do login and token refresh with the amazon-cognito-identity-js package. It was going well until I had to do token refresh, which I was doing with the CognitoUser.refreshSession() method. I was getting an auth token invalid, which I realised from this SO post is due to the deviceKey missing from the authentication.

Looking at the CognitoUser code I realised that it is possible to have the method send the deviceKey, but you need to have used the storage, which I don't have (running a script every 15 minutes, reading token information from a file).

As a workaround I've added a fourth parameter to refreshSession to pass in the device key, which then lets me do the refresh no problem. Another option might be to see if this.deviceKey is set, and if so then set it on the authParameters.

Expected behavior

I expected to be able to pass in the deviceKey to the refresh method, since it is required for a correct refresh of the token. Or perhaps there should be a refreshTokens method which takes the refresh token and deviceKey.

Reproduction steps

Attempt to call CognitoUser.refreshSession() with just a refresh token when device tracking is enabled. It will give an error unless you store the session.

Code Snippet

This is the hacky workaround I put into refreshSession:

        if (deviceKey) {
                authParameters.DEVICE_KEY = deviceKey;
        }

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 1 year ago

Hello @AllanJard, and thank you for opening this issue. It sounds like this issue might be solved by potentially turning off device tracking from within the Cognito console for your User Pool. You can find this under Cognito Console > App's User Pool > "Sign-in Experience" tab, then scroll down to "Device Tracking" and hit edit button. There's 3 options, but changing your settings to "Don't Remember" should create the behavior you're looking for.

There's a comment from another issue related to this here for more context and experiences from others that described something similar. Let me know if this helps!

AllanJard commented 1 year ago

Hi @cwomack - many thanks for your reply!

Unfortunately, disabling device tracking is not an option in my use case. It is a third party service which provides authentication via AWS, necessitating a workaround at the moment, since the API doesn't quite support this (yet? ;-)).

cwomack commented 1 year ago

@AllanJard, understandable that disabling device tracking is off the table. After reviewing this with the term internally to see if we can recommend a workaround, I'm not sure there's a clean cut way to do it.

If you're not using localStorage (where the deviceKey is written to) and with the deviceKey not being written to cookies... I believe you'd need to fork the amazon-cognito-identity-js package repo and implement a custom/viable workaround.

Since there's no "out of the box" way for the deviceKey to be written to cookies, I'll convert this issue to a feature request and update the title to reflect the limitation.

AllanJard commented 1 year ago

I'm not using http at all at this point actually. Its a script bring run by a cron job, with the device key and refresh token being stored in a file. That's why I needed the fourth parameter to be able to pass the device key for the refreshSession() function.

Agreed that this is a feature request. I'd like it to be that refreshSession() have the ability to set the device key without need for the built in storage options.

cwomack commented 1 year ago

Updated the title to better reflect that and appreciate you submitting this @AllanJard. If you're able to come up with a viable workaround or forked implementation that allows refreshSession() to set the deviceKey, feel free to comment here and share the details!

We'll update this issue if there's any updates or progress on this from our side regarding the feature request.

wduminy commented 1 year ago

I am trying to implement MFA with remembered devices, but I am using sessionStorage for my Cognito session.

Maybe it makes sense to always store the device key on local storage.

Or must I assume session storage is not supported when I want to remember devices for MFA?

So for me providing the device key to getSession() would be handy.

AllanJard commented 1 year ago

I think having a way to send the device key to refreshSession() will be useful - that way it is storage type independent. I'll send a PR for that.

If you are using the built in session support I think you can just set the .deviceKey property directly. I tried that but there is a little logic in the refreshSession() method that doesn't use it (it checked the session is used).

wduminy commented 1 year ago

@AllanJard I agree. I like the idea of having some storage type independent solution. It will solve my problem too.

AllanJard commented 1 year ago

I've committed the change but haven't sent a pull request yet, as I've not been able to build the package.

The readme doesn't include build instructions, and if I npm install then I get a error about needing to install legacy peer dependencies. Do that, and then npm run build and I get this error during the build:

> amazon-cognito-identity-js@6.3.1 build:umd
> webpack

node:internal/modules/cjs/loader:936
  throw err;
  ^

Error: Cannot find module 'compression-webpack-plugin'

@cwomack - is this something you can help with? What is the correct build process for the amazon-congnito-identity package?

wduminy commented 1 year ago

@AllanJard Please consider also adding an optional deviceKey argument to getSession(callback, options = {}) This is the call I am using. It calls refreshSession when needed.

wduminy commented 1 year ago

@AllanJard

The readme doesn't include build instructions, and if I npm install then I get an error

I have not tried building myself. Have you tried npm ci instead of npm install?

AllanJard commented 1 year ago

npm ci results in the same error unfortunately.

Regarding getSession() - sure. I've added it to the options for that method it makes sense to have it there and optional (a second commit made the options optional in TypeScript).

It will be up to the Amazon team if they want to pull this in. I hope so as I think it is useful, but I can't build it yet, so I still haven't sent a PR.