Refactor storage to an interface about angular-auth-oidc-client

[robisim74]

@damienbod I completed the test of the branch.

In my opnion, it is ready to be published (v1.2.0 because there are new features).

Changelog:

• Add support for Server Side Rendering
• Add support for custom storage, for example for using cookies (provide class-interface OidcSecurityStorage): see README

BREAKING CHANGES

• Setting of storage (sessionStorage or localStorage) has been moved to AuthConfiguration (by default, however, sessionStorage is always used).

This version, in my opinion, fixes: #37, #36, #28.

[robisim74]

I think we should store userData using DI (only in memory), and not the Storage. Then if a dev wants to save them in a Storage it's his decision.

[robisim74]

Please stop commits. Later I'll do the new storage.

[robisim74]

I solved the problems with VS. Please wait to use the new branch because I'm still working on it.

[damienbod]

@robisim74 I made a test branch for storage using the example from crazyht. Haven't had any success with this yet. I think I will scrap it and start again.

[damienbod]

Testing with SSR and the 1.1.5 version. I get the following error:

Exception: Call to Node module failed with error: ReferenceError: sessionStorage is not defined at OidcSecurityCommon.setupModule

I believe we need to fix the storage before the SSR works.

[robisim74]

This error is not related to SSR. Where are you testing?

[robisim74]

If you are using the version with "custom storage", it's wrong.

[damienbod]

Not using the version with custom storage. Using the last version from the branch origin/SSR.

Created a new default Angular project using the ASP.NET Core 2.0 Angular template.
Added the auth module => error

I'll check it in if it would help. I need to read up on the SSR then.

Greetings Damien

[robisim74]

Damien, where can I find this project? I had no problems to run the client project using https://github.com/damienbod/angular-auth-oidc-client-SSR-test

In the meantime, I pushed a new branch: https://github.com/damienbod/angular-auth-oidc-client/tree/New_storage

That branch contains the support for SSR & the support for custom storage. For now I can't test it, not even in the normal test app, because I have VS crashed and I'm reinstalling everything.

Later I'll update you.

[damienbod]

thanks, funny I have the same problem with the https://github.com/damienbod/angular-auth-oidc-client-SSR-test, maybe it's something local.

[robisim74]

For me the problems started when I installed .net Core preview 2...

[damienbod]

Yes, I haven't had any success with .NET Core 2.0 either.

[robisim74]

For me it works only from the command line.

[damienbod]

Really big thanks! I test tomorrow, update the samples and release.

[damienbod]

released 1.2.0, tested the browser applications. Will test the SSR ones tomorrow

[astegmaier]

@damienbod and @robisim74 - thanks for attacking this issue--I really appreciate the time you're taking to try to make the SSR scenario work.

I was wondering, though, if it would be possible to use the standard JavaScript Storage interface instead of a custom one (OidcSecurityStorage) ? There already exist many libraries that implement this interface using a variety of underlying storage mechanisms (see, for example, cookie-storageand memorystorage). If angular-auth-oidc-client spoke this language, it would be much easier to simply plug in one of these implementations without a lot of ceremony or shimming code.

(this might make your life easier inside the class, too, since the Storage interface is exactly what localStorage and sessionStorage implement).

I forsee the need to swap out storage getting more popular as people increasingly adopt SSR, and I want to make it as easy as possible for people to do it right.

I'm happy to help out here, too, if you like the idea. (Although I'm not super familiar with the internal working of angular-auth-oidc-client, so any pointers would be appreciated.

[robisim74]

@astegmaier

if it would be possible to use the standard JavaScript Storage

By default, the library already uses this interface.

OidcSecurityStorage is only an abstract class, that you can customize using what you want (cookies, nodejs storage etc., also through other libraries), if you don't want use the default storage (sessionStorage or localStorage).

[astegmaier]

I think what I was suggesting was a tweak to the way that a consumer of angular-auth-oidc-client would provide a custom storage implementation: instead of needing to implement OidcSecurityStorage, you could just pull an implementation of Storage (e.g. cookie-storageand memorystorage) off the shelf, and plug it in directly. Something like this:

import { CookieStorage } from "cookie-storage";

    imports: [
        AuthModule.forRoot({storage: CookieStorage})
    ],

The thought is that would be easier for consumers of the library in many cases, since well-tested, complete implementations of Storage already exist.

(As I said before I'm happy to help out with the implementation if you like the idea).

[robisim74]

My opinion:

• Maybe you can find a compatibility with those libraries, but tomorrow another dev could ask us a compatibility with another library and so on
• You lose the possibility to implement another kind of storage, like Ionic storage or a database

The only improvement that I think is necessary is to make asynchronous abstract methods (especially if a user wants to use db as a storage) but this requires to make asynchronous the whole operation of the library.

[astegmaier]

Storage is a standard interface. You could create a Storage shim for anything you could imagine, including ionic. I sympathize with not wanting to accommodate a bunch of one-off requests (I said it before, and I'll say it again--I really appreciate your taking the time to engage with SSR). But it seems like conforming to a standard is better, not worse for this goal.

One idea: I think it would be possible to let Storage implementations be passed through if you simply renamed the read() and write() methods of OidcSecurityStorage to be getItem() and setItem() to match the same methods in Storage. (because of TypeScript duck-typing). I'll play around with this to get a PoC.

[damienbod]

@astegmaier @robisim74 I prefer the custom class to wrap the storage. Per default and for most users, this is not needed. I would prefer that the lib is independent of the Storage class.

[astegmaier]

@damienbod @robisim74 No problem--I can accomplish what I need to for SSR either way--it was more of an ease-of-use/aesthetic suggestion.

I updated my test repo to try things out end-to-end with the new OidcSecurityStorage interface. The client and server implementations are in storage-config.ts. Things are mostly working (awesome!).

(Also: I synced with the latest changes in @damienbod's fork and added both of you as collaborators on the main repo. Feel free to party there if you'd like).

There's two problems I hit, though:

1. The interface for OidcSecurityStorage mentioned in the readme calls for the parameters of the write() method to be of type string:

   public write(key: string, value: string): void {
      ...
   }
   

   But the actual interface in code allows for the value parameter to be any. I found through testing that the lib was actually sending a full unserialized Object in some cases (specifically, when it was setting userinfo data), so I had had to add some code to stringify and parse this (and handle edge cases like undefined). I was thinking it might be simpler for consumers if there was a tighter contract like the one suggested in the readme--less boilerplate, and less edge-cases where a person trying to use the library can mess up. Thoughts?

2. There seems to be some problems calling oidcSecurityService.getToken() and oidcSecurityService.getUserData() Specifically:

   a. They will work fine immediately after the user logs in, but if you hard-refresh and call them again, they return nothing
   b. the getUserData() method does not work unless you wait a while (~2-3 seconds) after oidcSecurityService.authorizedCallback() starts before calling it. I might be doing something wrong, but if there is some kind of asynchronous dependency for getUserData(), maybe it should return a promise or observable?

   Both (a) and (b) appear to be unrelated to the storage used (I see the same problem if I turn off prerendering and use the default sessionStorage), so I can file a new issue with more details if you'd like.

[astegmaier]

Actually, after reading #41, I think I may have solved issue 2(b) above by changing the home component to:

    ngOnInit() {
        if (typeof location !== "undefined" && window.location.hash) {
            this.oidcSecurityService.authorizedCallback();
        }
        this.oidcSecurityService.onUserDataLoaded.subscribe(() => {
            this.checkUserInfo();
        });
        this.checkToken();
    }

I'd love to hear your thoughts about (1) and (2)(a), though.

[robisim74]

The type string in README (and in BrowserStorage class) is a my fault: later I'll fix it.
@damienbod I have a doubt: do we need to store objects?

[damienbod]

@robisim74 thanks.

I don't think we should store objects, this would/could cause a security issue.

[robisim74]

Fixed. Uhm... but at the time we are storing two objects:

• userData
• wellknownendpoints

[damienbod]

wellknownendpoints does not matter, this is public.

userData, yes, need to think about this.