Skip to content
This repository has been archived by the owner on Oct 24, 2024. It is now read-only.

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

Open
2 of 9 tasks
Valdermeyder opened this issue Mar 27, 2020 · 13 comments

Comments

@Valdermeyder
Copy link
Contributor

Valdermeyder commented Mar 27, 2020

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

  • jwt-verifier
  • okta-angular
  • oidc-middleware
  • okta-react
  • okta-react-native
  • okta-vue

I'm submitting a:

  • Bug report
  • Feature request
  • Other (Describe below)

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 #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

  • Package Version: 3.0.0, 2.x, 1.x
  • Browser: Chrome
  • OS: MacOS
  • Node version (node -v): v13.10.1
  • Other:
@swiftone
Copy link
Contributor

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

@swiftone
Copy link
Contributor

swiftone commented Apr 3, 2020

Internal ref: OKTA-288352

@swiftone
Copy link
Contributor

swiftone commented Apr 8, 2020

@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
Copy link

sero323 commented Apr 9, 2020

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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link

sero323 commented Apr 10, 2020

@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
Copy link
Contributor

@sero323 - I've moved your autoRenew issue out to #744 so it doesn't get lost.

@swiftone
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link

sarahdayan commented Aug 14, 2020

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 #721 (comment) and #721 (comment).

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
Copy link
Contributor

@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
Copy link

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants