search

okta / okta-oidc-js

okta-oidc-js
https://github.com/okta/okta-oidc-js
Other
395 stars 232 forks source link

When autoRenew prop is set to false, it is still set as true. #596

Open khanguslee opened 4 years ago

khanguslee commented 4 years ago

I'm submitting this issue for the package(s):

I'm submitting a:

Current behavior

Whilst trying to figure out how to set autoRenew to false for the Security component, I was wondering why my tokens were still getting automatically removed. After finding out that @okta/okta-react actually uses version 2.5.0 of @okta/okta-auth-js, I was met with this bug.

If you set the prop autoRenew={false}, the tokens will still get renewed. https://github.com/okta/okta-oidc-js/blob/2c41c3f1eefb7bd19bb08adb77e448d989789f18/packages/configuration-validation/src/lib.js#L50 The above line will equal to undefined since false || undefined is undefined. However, if the user sets both autoRenew={false} and auto_renew={false} then, the resultant value will be false. If the user ONLY sets auto_renew={false} then undefined || false is false.

The obvious solution is to remove auto_renew as it is not camelcase like the other props. This was also discussed here: https://github.com/okta/okta-oidc-js/pull/529

Expected behavior

When I only set autoRenew={false} I expect this to have the same behaviour as if I had only set auto_renew={false}.

Minimal reproduction of the problem with instructions

Extra information about the use case/user story you are trying to implement

Any Security component with autoRenew={false} prop set.

     <Security
        issuer={getIssuer()}
        client_id={getAppId()}
        redirect_uri={getRedirectUri()}
        autoRenew={false}
      >
...
</Security>

Environment

Version 1.2.0 of @okta/okta-react library. Version 2.5.0 of @okta/okta-auth-js Version 0.3.0 of @okta/configuration-validation

khanguslee commented 4 years ago

A solution to this would be:

const autoRenew = !!config.autoRenew || !!config.auto_renew;

Happy to create a PR for this?

khanguslee commented 4 years ago

I've implemented a fix on a fork on master branch. https://github.com/khanguslee/okta-oidc-js However, I get 6 failed tests. Am I misinterpreting what should be passed into config.autoRenew?

From https://github.com/okta/okta-auth-js#the-tokenmanager it appears autoRenew is meant to be a boolean?

swiftone commented 4 years ago

Internal ref: 260077

khanguslee commented 4 years ago

@swiftone is this currently getting looked at internally? As I see it currently, we either:

swiftone commented 4 years ago

@khanguslee - It's on our list, but in the absence of a lot of reports it falls behind other issues that are granted higher priority. From the look of it, your fix makes sense (we only keep the old syntax around to support backward compatibility).

Thanks for the detailed report - I know it can be frustrating to see a simple fix get delayed (believe me, I and every other developer knows that pain) but your efforts aren't being ignored.

khanguslee commented 4 years ago

@swiftone I deeply understand since there is a simple workaround for this!

Happy to create the fix and amend the tests and pin it to this issue so when the time comes, it'll be easier for you!

dgangwar commented 4 years ago

@khanguslee Is there any update on this issue?

khanguslee commented 4 years ago

@khanguslee Is there any update on this issue?

Not that I know of, but if you are having this issue, the workaround is to only use the auto_renew prop!

swiftone commented 4 years ago

I have poked The Powers That Be on this one.

khanguslee commented 4 years ago

@swiftone Appreciate it! 👏