Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using localStorage not working as expected #321

Closed
jeroenheijmans opened this issue May 17, 2018 · 3 comments
Closed

Using localStorage not working as expected #321

jeroenheijmans opened this issue May 17, 2018 · 3 comments

Comments

@jeroenheijmans
Copy link
Collaborator

I'm trying to use localStorage as a backing store to make sure new tabs in the same browser have instant access to access tokens if available. I find however that it always sends Authorization: bearer null.

I'm mostly mimicking what's in the example (configuration wise). Here's a repro:

# Angular 5 CLI so using 3.x version of the lib
ng new store-test --minimal 
npm i angular-oauth2-oidc@^3 --save

Add these to the module:

HttpClientModule,
OAuthModule.forRoot({
  resourceServer: {
    allowedUrls: ['https://httpbin.org/'],
    sendAccessToken: true
  }
})

And change the component to:

import { Component } from '@angular/core';
import { AuthConfig, OAuthService, JwksValidationHandler, OAuthErrorEvent } from 'angular-oauth2-oidc';
import { HttpClient } from '@angular/common/http';

@Component({
  selector: 'app-root',
  template: `<h1>Welcome!</h1>
    <p>
      <button (click)='login()'>login</button>
      <button (click)='logout()'>logout</button>
      <button (click)='ajax()'>ajax</button>
    </p>
    <h2>Your access token:</h2><p><code>{{accessToken}}</code></p>`,
  styles: []
})
export class AppComponent {
  constructor(private oauthService: OAuthService, private http: HttpClient) {
    this.oauthService.configure({
      issuer: 'http://localhost:5000',
      redirectUri: window.location.origin + '/',
      clientId: 'my-spa',
      scope: 'openid profile',
    });

    this.oauthService.tokenValidationHandler = new JwksValidationHandler();

    this.oauthService.setStorage(localStorage);  // <-- not the desired effect?

    this.oauthService.events.subscribe(e => { 
      if (e instanceof OAuthErrorEvent) { console.error(e); } 
      else { console.warn(e) } 
    });

    this.oauthService.loadDiscoveryDocumentAndLogin();
  }

  public get accessToken() { return this.oauthService.getAccessToken(); }

  login() { this.oauthService.initImplicitFlow(); }
  logout() { this.oauthService.logOut(); }
  ajax() { this.http.get("https://httpbin.org/get").toPromise(); } // <-- this call
}

I'm hosting the IdentityServer4 sample with implicit flow. The login/logout stuff works just fine. The ajax() call however will show up like this in Chrome:

GET /get HTTP/1.1
Host: httpbin.org
Origin: http://localhost:4200
Authorization: Bearer null
etc.

Even weirder, if I do setStorage(myStore) where I have this decorator:

const myStore: OAuthStorage = {
  getItem(key) {
    const data = localStorage.getItem(key);
    console.warn('get', key, data.substring(0, 25));
    return data; 
  },
  setItem(key, data) {
    console.warn('set', key, data.substring(0, 25));
    return localStorage.setItem(key,data);
  },
  removeItem(key) {
    console.warn('remove', key);
    return localStorage.removeItem(key);
  },
};

Then I see that all get calls properly return data, never null.

Am I configuring something incorrectly? Or have I found a bug?

@jeroenheijmans
Copy link
Collaborator Author

After some more searching I found that updating the @NgModule's providers: [...] array with this...

{ provide: OAuthStorage, useValue: localStorage },

...does the trick. This makes the setStorage(localstorage) call superfluous even, it seems.

This same fix (as well a provider for the OAuthModuleConfig constant) is needed to make the custom interceptor mentioned in the docs work properly at all.

Not sure what options are supposed to work, but either the code or the docs need a few updates. If you could provide some guidance here (and an answer for #317) then I'd be happy to fire up a PR to fix things.

@manfredsteyer
Copy link
Owner

Oh, this is a good finding. setStorage is a relect of older days, where we didn't have an interceptor. I think, we should deprecate it.

@manfredsteyer
Copy link
Owner

The next version will contain a depcreation information for this.

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

No branches or pull requests

2 participants