Skip to content
This repository has been archived by the owner on Jul 26, 2022. It is now read-only.

OktaAuthService.getUser() throws TypeError #192

Closed
jakehockey10 opened this issue Apr 17, 2018 · 18 comments
Closed

OktaAuthService.getUser() throws TypeError #192

jakehockey10 opened this issue Apr 17, 2018 · 18 comments

Comments

@jakehockey10
Copy link

Hello,

I'm using the latest version of the @okta/okta-angular npm package and the interface has changed quite a bit. Some things are now promises while other things are not. And I used to be able to access the TokenManager. Not sure if that is possible now.

But when I use getUser from the service, I sometimes get this error:

TypeError: Unable to get property 'sub' of undefined or null reference

I'm having a little bit of trouble with the new interface because I attempted to wrap the OktaAuthService with my own AuthenticationService so that I could cache things and have a central location for authentication related questions and activities. Now that things are promise-based (like the access token and user claims object), my app can't initialize in the same way that I have had it initialize before. Not a huge difference, but I wanted to generally asks if I'm approaching this right. Should I be trying to cache the results of getUser, isAuthenticated, getAccessToken, etc. or should I be using these methods any time a component or service needs to know the value of them? Does the @okta/okta-angular library handle excessive calls to remote authorization servers for me?

@jmelberg-okta
Copy link
Contributor

jmelberg-okta commented Apr 17, 2018

Hey @jakehockey10!

First, let me recommend what you should/shouldn't cache:

  1. getUser: Absolutely. It is very common to take the response from this method and create/query for a user. If at any time the user's claims update, you can call this method again.
  2. isAuthenticated: I'd suggest not caching this response. This provides local validation that the accessToken and idToken have not expired. If you cache this response, you may be referencing an expired user.
  3. getAccessToken: By default, an accessToken will get refreshed if it has expired. Similar to the isAuthenticated method, I'd recommend not caching this value to ensure your app is aware of the current state of the user.

Does the @okta/okta-angular library handle excessive calls to remote authorization servers for me?

It doesn't, but I like the suggestion! We have an open enhancement to add support for an HTTP interceptor, during which we'd explore adding retry logic, a backoff strategy, etc. Want to create an issue for the enhancements you'd like to see?

Regarding the error mentioned in your comment - are you waiting for waiting for the response to resolve? Here is an example of how we are waiting for getUser to finish, and populate the UI with the claims: samples-js-angular/okta-hosted-login.

Hope this helps!

@jakehockey10
Copy link
Author

@jmelberg-okta Thank you so much for the suggestions! I took your suggestions into account and things are working much better for me, thank you!

@jakehockey10
Copy link
Author

In terms of enhancements, I'll think of a better way to phrase it and try to write something up! Thanks!

@jakehockey10
Copy link
Author

As for the error, I'm still seeing it locally and in a deployed dev environment. I think it is IE 11 related though. I don't remember seeing the issue in other browsers. But yes, any time I call getUser, I'm using await.

@jakehockey10
Copy link
Author

@jmelberg-okta Some more info on the error: I am indeed using await on each usage of getUser but I found out that i was actually doing this twice when I only needed to do it once. It seems to be a timing issue on IE only, but it also only occurs on the second call the getUser. I don't know if this narrows it down at all?

@jakehockey10
Copy link
Author

jakehockey10 commented Apr 20, 2018

@jmelberg-okta Could I talk to you a little bit more about this? I'm still having issues with getUser:

TypeError: Unable to get property 'sub' of undefined or null reference

It happens way more frequently in IE11. After authenticating through the redirect flow, and as my application is bootstrapping again, the app.component.ts's ngOnInit() method is being called before the OktaAuthService's authentication state is emitted. So I wait for the authentication state is emitted before setting up the contents of my main menu, which depends on whether you are authenticated or not as well as whether or not I can get the user. Any time I need to await the getUser method, I await the isAuthenticated first, and if isAuthenticated, then I await the promise of UserClaims. But this call is what ends up throwing the error.

The error seems to be thrown after calling into okta-auth-js's getUserInfo method in the token.js file. This method is returning undefined, and hence the error. But looking at that source code seems to only return undefined if the access token isn't right or missing. But before going into okta-auth-js, it seems that the access token IS there.

Any ideas on how I can diagnose this and avoid it in the future?

For additional context: Here are the problem areas:

...
export class AppComponent implements OnIt {
...
  async ngOnInit() {
    // Checking isAuthenticated right here will return true if
    // the user logged in a while back and they are coming back
    // to the site.  It will return false when coming back from
    // the login redirect.  This method is called prior to
    // the change in authentication state is emitted.
    this.isAuthenticated = await this._auth.isAuthenticated();
    if (this.isAuthenticated) {
      this.user = await this._auth.getUser();
      if (this.user) {
        this._zendesk.identify({
          name: this.user.name,
          email: this.user.email
        });
        this._alerts.successOccurred.next(`Hello ${this.user.name}`);
      }
    }
    this._devExtremeTheme.apply(this._settings.theme);
    this.mainMenu$ = this._menu.getCurrentMenu();
  }
...
  private listenToAuthentication() {
    this._auth.$authenticationState.subscribe(
      async (isAuthenticated: boolean) => {
        this.isAuthenticated = isAuthenticated;
        if (isAuthenticated) {
          this.user = await this._auth.getUser();
          if (this.user) {
            this._zendesk.identify({
              name: this.user.name,
              email: this.user.email
            });
          }
          this._zendesk.show();
        } else {
          this.user = null;
          this._zendesk.hide();
        }
        this.mainMenu$ = this._menu.getCurrentMenu();
      }
    );
  }
...
}

@Injectable()
export class MenuService {
...
  getCurrentMenu(): Observable<MenuItem[]> {
    return defer(async () => {
      let isAuthenticated = false;
      try {
        isAuthenticated = await this._auth.isAuthenticated();
      } catch (e) {
        console.error(e);
        return this.loggedOutMenu;
      }
      if (isAuthenticated) {
        try {
          const user = await this._auth.getUser();
          return user ? this.customizedMenu(user) : this.mainMenu;
        } catch (e) {
          console.error(e);
          return this.mainMenu;
        }
      } else {
        return this.loggedOutMenu;
      }
    });
  }
...
}

At first, I didn't have all these try/catch blocks all over the place, but I couldn't tell which call to getUser was throwing the error. I've been trying to figure out the timing of everything and why it fails so much more frequently on IE11.

@jmelberg-okta
Copy link
Contributor

Hey @jakehockey10,

My guess is this could be due to a couple of issues. Since this only occurs in IE 11, can you verify that Third-party cookies are enabled? You can see how to via visit this forum post.

After a user's accessToken is expired, it'll attempt to refresh it by leveraging the Okta session. Without third-party cookies allowed, this request will fail and the accessToken will become undefiend.

The second reason could be that isAuthenticated is returning true because you have a valid idToken, not accessToken. Are your requesting both? For example, your configuration should look similar to:

const oktaConfig = {
  issuer: 'https://{yourOktaDomain}.com/oauth2/default',
  clientId: '{clientId}',
  redirectUri: 'http://localhost:{port}/implicit/callback',
  responseType: 'id_token token'
}

Let me know if this solves the issue!

@jakehockey10
Copy link
Author

Here is my IE 11 settings. I had to add my issuer to the sites collection of trusted sites as well. The only thing I hadn't done was specify the responseType in my oktaConfig. I hadn't had to do this before, while still getting an access token.

image
image

It's really hard to tell what the problem is because I'm also having live-reload issues with IE. Developing for IE is pretty freaking painful! I have to restart the browser a lot anyway.

The issue seems to have calmed down a little bit, but I'm still seeing the issue occasionally. It has to be something I'm doing timing-wise...

@jmelberg-okta
Copy link
Contributor

You're most likely running into the race condition described in okta-auth-js. Since this library uses it under the hood, that issue is propagated up into the integrations.

You can validate this by waiting for your accessToken to expire (~1 hour) and refreshing the page. The isAuthenticated method will return true, but the token you're using to hit the /userinfo endpoint via getUser() is invalid - hence the error.

Our team is working on getting this fixed as soon as possible, so I apologize for the inconvenience this is creating.

@jakehockey10
Copy link
Author

Ah that sounds like it could be it. Thanks for helping me curb the feeling of insanity!

@jakehockey10
Copy link
Author

@jmelberg-okta , I'm pulling in okta-auth-js and trying to use the TokenManager to listen to the refresh event and try to getUser again. Is this how you would get around this issue until the fix comes in? It doesn't seem to fire the event on app load, but I get subsequent refresh firings...

@jakehockey10
Copy link
Author

Here is my authentication.service.ts file that abstracts away okta-angular so that I have a central place to deal with the race condition:

import { Injectable } from '@angular/core';
import { Router } from '@angular/router';
import { OktaAuthService, UserClaims } from '@okta/okta-angular';
import * as OktaAuth from '@okta/okta-auth-js';
import { Subject } from 'rxjs/Subject';

import { environment } from '../../environments/environment';

@Injectable()
export class AuthenticationService {
  private _oktaAuth: OktaAuth;
  private _user: UserClaims;

  tokenRefreshed = new Subject<UserClaims>();
  authenticationState = new Subject<boolean>();

  constructor(private _router: Router, private _okta: OktaAuthService) {
    this._oktaAuth = new OktaAuth({
      url: environment.oktaURL.split('/oauth2/')[0],
      clientId: environment.oktaClientID,
      issuer: environment.oktaURL,
      redirectUri: environment.rootURI
    });
    this._oktaAuth.tokenManager.on(
      'refreshed',
      async (key, newToken, oldToken) => {
        await this.setUser();
        this.tokenRefreshed.next(this._user);
      }
    );
    this._oktaAuth.tokenManager.on('error', error => {
      console.error(error);
    });
    this._okta.$authenticationState.subscribe(async isAuthenticated => {
      this._user = null;
      if (isAuthenticated) {
        await this.setUser();
      }
      this.authenticationState.next(isAuthenticated);
    });
  }

  async isAuthenticated(): Promise<boolean> {
    try {
      /**
       * This returns whether they have an idToken or an accessToken.
       * I want `isAuthenticated` to mean whether or not I have
       * both an idToken AND an accessToken (preferably not expired).
       * I've had this return true when all I had was an accessToken
       * (I'm not sure how I got into that state).
       */
      const isAuthenticated = await this._okta.isAuthenticated();
      const idToken = await this._okta.getIdToken();
      return isAuthenticated && !!idToken;
    } catch (e) {
      console.error(e);
      return false;
    }
  }

  async getUser(): Promise<UserClaims> {
    if (await this.isAuthenticated) {
      if (!this._user) {
        await this.setUser();
      }
    } else {
      this._user = null;
    }
    return this._user;
  }

  async isAdmin(): Promise<boolean> {
    try {
      if (await this.isAuthenticated) {
        return (
          (await this.getUser()
            .then<boolean>(
              userClaims =>
                userClaims &&
                userClaims['groups'] &&
                userClaims['groups'].includes('admin')
            )
            .catch(reason => {
              console.error(reason);
              return false;
            })) || false
        );
      } else {
        return false;
      }
    } catch (e) {
      console.error(e);
      return false;
    }
  }

  async getAccessToken() {
    return this._okta.getAccessToken();
  }

  login() {
    this._okta.loginRedirect(this._router.url, {
      scopes: ['groups', 'openid', 'profile', 'email', 'address', 'phone']
    });
  }

  logout(): Promise<void> {
    return this._okta
      .logout()
      .then(() => {
        this._router.navigate(['/home']);
      })
      .catch(reason => {
        localStorage.removeItem('okta-token-storage');
      });
  }

  private async setUser(): Promise<void> {
    try {
      this._user = await this._okta.getUser();
    } catch (e) {
      this._user = null;
      console.error(e);
    }
  }
}

Any suggestions?

@RaduGrama
Copy link

@jmelberg-okta, prior to calling await this.oktaAuth.getUser(), does a call to await this.oktaAuth.getAccessToken() or await this.oktaAuth.getIdToken() help? Does any of those two calls ensure the expired token is refreshed, or do they suffer from the same race condition issue?

@jmelberg-okta
Copy link
Contributor

This issue was related to a bug that has since been fixed with okta-auth-js@2.x.

All of our integration libraries have been updated to pull in this bug-fix, so please update you app accordingly.

If the issue persists, please feel free to reopen this issue.

@ShoaibShaukatSystems
Copy link

Issue still persist for edge browser? Please share if someone have solution for this?

@swiftone
Copy link
Contributor

swiftone commented Feb 7, 2019

@ShoaibShaukatSystems
Are you saying the issue is still occurring for Edge even though you are running the latest version of okta-auth-js or relevant SDK?

If so, can you share which SDKs and confirm the experience you see?

@ShoaibShaukatSystems
Copy link

@swiftone
We are using @okta/okta-angular 1.0.7.
For IE 11 and Chrome it is working fine. For edge Browser I'm still having issues with getUser:
TypeError: Unable to get property 'sub' of undefined or null reference

@constantinblanariu
Copy link

I'm still having this issue, on Chrome. But maybe it's something I'm doing wrong.
I'm using @okta/okta-angular@1.4.0

After about 1h if I refresh the page I'm getting

TypeError: Cannot read property 'name' of undefined

The code is below:

async ngOnInit() {
    this.isAuthenticated = await this.oktaAuth.isAuthenticated();
    if (this.isAuthenticated) {
      const oktaGetUser = this.oktaAuth.getUser().then(
        (userClaims) => this.displayName = userClaims.name
      )
    }
}

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

6 participants