search

okta / okta-oidc-js

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

`authState.isAuthenticated` is not updated properly when either `idToken` or `accessToken` is expired #721

Open Valdermeyder opened 4 years ago

Valdermeyder commented 4 years ago

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

I'm submitting a:

Current behavior

When the user doesn't have idToken (it became expired) the authState.isAuthenticated property still has the value true The issue looks like come from this code which contains code which doesn't align with the documentation "By default, authService will consider a user authenticated if both getIdToken() and getAccessToken() return a value"

Expected behavior

When the user doesn't have either idToken or accessToken the authState.isAuthenticated property should have value false

Minimal reproduction of the problem with instructions

  1. Login
  2. Open DevTools
  3. Remove idToken from okta-token-storage inside Application -> Local Storage (default place for tokens)
  4. Refresh the page

You should be redirected to the login page, but it doesn't happen and if you show some information using authService.getUser() method it will not be visible

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

I have created pull-request with quick fix https://github.com/okta/okta-oidc-js/pull/718 but some tests are failed and now I have doubts if this correct solution

Workaround

For versions 2.x and 3.x

const FixedSecurity = ({ children, ...restProps }) => {
  const authService = new AuthService(...restProps);
  // eslint-disable-next-line no-unused-expressions,no-underscore-dangle
  authService._config.isAuthenticated = async () => {
    try {
      const accessToken = await authService.getAccessToken();
      const idToken = await authService.getIdToken();

      return Promise.resolve(!!(accessToken && idToken));
    } catch (err) {
      return Promise.reject(err);
    }
  };

  return <Security authService={authService}>{children}</Security>;
};

Environment

swiftone commented 4 years ago

Good catch @Valdermeyder! I have to add a quick test to the PR, but we'll definitely be fixing this ASAP!

swiftone commented 4 years ago

Internal ref: OKTA-288352

swiftone commented 4 years ago

@Valdermeyder - We've reverting this change, as internal discussions reveal that this is the intended behavior.

The issue appears to be confusion as to what "isAuthenticated" means. We are reviewing our docs to make that more clear, since (as you point out) what is said is NOT specifically what is happening.

Renewal of tokens should be automatic, customers can configure them to have different lifespans for different purposes. Currently you can override isAuthenticated() as well as set handlers for onSessionExpired and control whether tokens are autoRenewed, which can lead to a bit of a mix of scenarios.

In order to make sure your use-case is covered, can you share a bit more about how you found problems with this (aside from manually deleting tokens from localStorage)?

sero323 commented 4 years ago

Well not sure about the OP’s issue but at least on my side when the accessToken is expired i don’t get a new token unless i refresh the browser on the SecureRoute. I have a useEffect listening to authState and it never gets called. And autoRenew is true on my side. Therefore when i send that accessToken requests to the api server it fails with 401. If isAuthenticated is as designed then it would be nice to have something in authState that indicates tokens are expired and triggers our useEffect so we can act on that. I’m on version 2.0.1 for okta-react. 3.0.0’s /implicit/callback route doesn’t work. I created a separate issue for that.

Valdermeyder commented 4 years ago

@Valdermeyder - We've reverting this change, as internal discussions reveal that this is the intended In order to make sure your use-case is covered, can you share a bit more about how you found problems with this (aside from manually deleting tokens from localStorage)?

@swiftone in my particular case the the accessToken has longer expiration time than idToken. The actual problem was when authService.getUser method was used. It returns undefined because it relies on idToken which was no longer available but because authState.isAuthenticated was still true, no new request for the user info was sent.

swiftone commented 4 years ago

@sero323 - I'm curious about the issues you are experiencing with autoRenew - as designed, it SHOULD autoRenew. Are you overriding the default onSessionExpired() call?

Do you have a separate issue for this autorenewal problem?

sero323 commented 4 years ago

@swiftone, no i don't have a separate issue (i can create one if you'd like). But again, i'm using the signin widget, along with okta-react 3.0, mobx, hash router. Here's my configuration...

const CustomLoginRouter = () => {
    // console.log('CustomLoginRouter');

    const history = useHistory();

    const customAuthHandler = () => {
        console.log('customAuthHandler');
        history.push('/login');
    };

    return (
        <Security
            clientId="******************"
            issuer="https://************/oauth2/default"
            onAuthRequired={customAuthHandler}
            pkce
            redirectUri={`${window.location.origin}/implicit/callback`}
        >
            <Route
                component={Home}
                exact
                path="/"
            />
            <Route
                component={LoginCallback}
                path="/implicit/callback"
            />
            <Route
                component={Login}
                path="/login"
            />
            <SecureRoute
                component={Protected}
                path="/Protected"
            />
        </Security>
    );
};

const App = () => (
    <Router>
        <CustomLoginRouter />
    </Router>
);

export default App;

and also, here's how i have "Protected" component setup...

//export default (props) => {  //   <------------------- I've tried this as well
export default withOktaAuth((props) => {
    const { authState, authService } = useOktaAuth();
    ....
    useEffect(() => { //        <-------------------  never called after first render
        if (authState.isAuthenticated) {
            console.log('User is Authenticated!', authState);
        } else {
            console.log('User is NOT authenticated.', authState);
            logout();
        }
    }, [authState, authService]);
    ....
});
swiftone commented 4 years ago

@sero323 - I've moved your autoRenew issue out to https://github.com/okta/okta-oidc-js/issues/744 so it doesn't get lost.

swiftone commented 4 years ago

@Valdermeyder - We're working to make sure we have a consistent and understandable definition of isAuthenticated. As a workaround in the meantime, if you specifically need to call getUser() I suggest checking authState.idToken.

Valdermeyder commented 4 years ago

@swiftone thanks for the help. However, the proposed workaround will not fully work in my case. I need the user information to be shown on the UI. If I use your suggestion the end result will be the same I will see nothing. Probably I can call authService.login manually in the case when idToken is missed.

sarahdayan commented 4 years ago

Hey @swiftone, I'm curious about the definition of isAuthenticated if it doesn't mean that the user is properly authenticated, as you brought up in https://github.com/okta/okta-oidc-js/issues/721#issuecomment-611212502 and https://github.com/okta/okta-oidc-js/issues/721#issuecomment-615406979.

I followed this tutorial and it ecommends using authState.isAuthenticated to determine whether the user is authenticated (e.g., to display the login form or to redirect to the app's entry point). I rely on the method for this, and to determine whether I can safely call authService.getUser() and expect a truthy value, as recommended in this guide. In my case as well, after a certain period of time, authService.getUser() returns undefined, but authState.isAuthenticated still returns true and onAuthRequired doesn't seem to be called.

I would be down to use authState.idToken everywhere instead of authState.isAuthenticated if that was a safer indicator, but it doesn't seem to be a good replacement. The user is still logged in, they still have access to secured route, they can't go back to the login form and they're not being automatically redirected there. So whether authState.idToken returns true or false, the user remains logged in, without being able to access user info.

Am I missing something? Thanks in advance for taking the time to explain 🙂

swiftone commented 4 years ago

@sarahdayan - Happy to answer what I can:

the definition of isAuthenticated if it doesn't mean that the user is properly authenticated

The problem here is "what does it mean to be 'authenticated'?". The code currently says something like: "If you have either a valid access token or a valid id token, that means you successfully proved your authentication and don't yet need to do it again".

But often those two tokens have different expiration dates. And if (for example), your idToken expired but your accessToken hasn't, does that mean you are or are not "authenticated"? Today, the code says you are, and (as you discover), that stinks if you then rely on this to mean the idToken remains valid. (Of course, if you aren't relying on that, using a "requires both tokens" definition means users will be reauthenticated more frequently)

One solution is to use the override for isAuthenticated() and have it require both tokens, though currently this requires that you provide your own AuthService instance to <Security> as shown in the README.

Another solution is to explicitly check what you are relying on, such as confirming that authState.idToken is present before relying upon it.

In upcoming major versions we hope to clarify (or remove) the confusion in the labels, but for now, the above options are the ones that you have.

Does that address your question?

sarahdayan commented 4 years ago

Hi @swiftone, thanks a bunch for the detailed answer. This is crystal clear.

After implementing the suggestion solution, I'm hitting a (non-blocking) error. I opened a case here to avoid derailing the thread.