IdentityModel / oidc-client-js

OpenID Connect (OIDC) and OAuth2 protocol support for browser-based JavaScript applications
Apache License 2.0
2.43k stars 840 forks source link

If loaded after zone.js, oidc-client kills change detection in angular 2 rc5 #87

Closed j2jensen closed 8 years ago

j2jensen commented 8 years ago

This plunker shows a simple example of an Angular 2 app that changes in response to an observable.

This one only differs in that the script tag for oidc-client is moved after the script tag for zone.js, but angular no longer updates the display. The underlying data is still changing, and if you cause the change detection to fire through any other means then it'll show the correct state at that point in time.

For what it's worth, this issue first appeared when we upgraded angular to rc5.

brockallen commented 8 years ago

And can you determine what the cause is?

j2jensen commented 8 years ago

I haven't spent any time digging into oidc-client code; it took me the better part of two days to trace the problem to this library, and I wanted to get an issue logged with a minimum reproduceable case before I left work. Does oidc-client mess with any html5 shims? Maybe defining the Promise type or something like that?

By the way, this was causing issues in previous versions of Angular 2 but I had just assumed it was a shortcoming with their change detection strategy and I came up with a workaround. RC5 just made it so my workaround stopped working.

On Tue, Aug 23, 2016, 9:16 PM Brock Allen notifications@github.com wrote:

And can you determine what the cause is?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/IdentityModel/oidc-client-js/issues/87#issuecomment-241946401, or mute the thread https://github.com/notifications/unsubscribe-auth/AAbrdzWpaW8qL4ZjjGQQXz41pKLkaM7fks5qi7eSgaJpZM4JraXh .

brockallen commented 8 years ago

Yes, the ~/dist/oirc-client.js includes babel's polyfill: https://github.com/IdentityModel/oidc-client-js/blob/master/README.md

brockallen commented 8 years ago

Are you using a module loader? I guess the assumption is that with something more modern like Angular 2 you would be...

j2jensen commented 8 years ago

Yeah. I'm still feeling like a beginner with TypeScript and ES6 module loading, so it took me quite a while to figure out the right way to include oidc-client into my project. I was expecting it to be more along the lines of Angular, where the import statement clues the TypeScript compiler into the fact that this library is being used, and causes it to be included. I was expecting to need something like this:

import { UserManager } from "oidc-client";

Now I think I understand that even though I'm using a module loader I'm actually expected to bring oidc-client in as an ambient dependency. Then, I need to use a /// <reference> to the .d.ts file, to help TypeScript to know that things like UserManager exist. I don't need an import statement like the one above at all. Am I missing something?

brockallen commented 8 years ago

To be honest, I wish I could say.... I also don't use TS or Angular -- I had to have others who know how module loaders work to help me organize this project. Keep at it, and let me know what you find.

j2jensen commented 8 years ago

I was able to get things to work using the strategy I mentioned above, but it feels a little wrong to me. If I change oidc-client.d.ts to look like this:

export class AccessTokenEvents {

    load(container);

    unload();

    addAccessTokenExpiring(callback:(ev:null) => void);
    removeAccessTokenExpiring(callback:(ev:null) => void);

    addAccessTokenExpired(callback:(ev:null) => void);
    removeAccessTokenExpired(callback:(ev:null) => void);
}
export class InMemoryWebStorage {
    getItem(key: string);

    setItem(key: string, value: any);

    removeItem(key: string);

    key(index: number);

    length?: number;
}
export class Log {
    NONE();
    ERROR();
    WARN();
    INFO();

    reset();

    level(value);

    logger(value);

    info(...args);
    warn(...args);
    error(...args);
}

export class MetadataService {
    constructor (settings: any);
    getMetadata();

    getIssuer();

    getAuthorizationEndpoint();

    getUserInfoEndpoint();

    getCheckSessionIframe();

    getEndSessionEndpoint();

    getSigningKeys();
}
export class OidcClient {
    constructor (settings: OidcClientCtor);

    createSigninRequest(args?: any): Promise<any>;
    processSigninResponse(): Promise<any>;

    createSignoutRequest(args?: any): Promise<any>;
    processSignoutResponse(): Promise<any>;

    clearStaleState(stateStore: any): Promise<any>;
}

export interface OidcClientCtor {
    authority?: string;
    metadataUrl?: string;
    metadata?: any;
    signingKeys?: string;
    client_id?: string;
    response_type?: string;
    scope?: string;
    redirect_uri?: string;
    post_logout_redirect_uri?: string;
    prompt?: string;
    display?: string;
    max_age?: number;
    ui_locales?: string;
    acr_values?: string;
    filterProtocolClaims?: boolean;
    loadUserInfo?: boolean;
    staleStateAge?: number;
    clockSkew?: number;
    stateStore?: any;
    ResponseValidatorCtor?: any;
    MetadataServiceCtor?: any;
}

export class UserManager extends OidcClient {
    constructor (settings: UserManagerCtor);

    clearStaleState(): Promise<void>;

    getUser(): Promise<User>;
    removeUser(): Promise<void>;

    signinPopup(args?: any): Promise<User>;
    signinPopupCallback(url?: string): Promise<any>;

    signinSilent(args?: any): Promise<User>;
    signinSilentCallback(url?: string): Promise<any>;

    signinRedirect(args?: any): Promise<any>;
    signinRedirectCallback(url?: string): Promise<User>;

    signoutRedirect(args?: any): Promise<any>;
    signoutRedirectCallback(url?: string): Promise<any>;

    events: UserManagerEvents;
}
export class UserManagerEvents extends AccessTokenEvents {
    load(user: User);
    unload();

    addUserLoaded(callback:(ev:null) => void);
    removeUserLoaded(callback:(ev:null) => void);

    addUserUnloaded(callback:(ev:null) => void);
    removeUserUnloaded(callback:(ev:null) => void);

    addSilentRenewError(callback:(ev:null) => void);
    removeSilentRenewError(callback:(ev:null) => void);
}
export interface UserManagerCtor extends OidcClientCtor {
    popup_redirect_uri?: string;
    popupWindowFeatures?: string;
    popupWindowTarget?: any;
    silent_redirect_uri?: any;
    automaticSilentRenew?: any;
    accessTokenExpiringNotificationTime?: string;
    redirectNavigator?: any;
    popupNavigator?: any;
    iframeNavigator?: any;
    userStore?: any;
}
export class WebStorageStateStore {
    set(key: string, value: any);

    get(key: string);

    remove(key: string);

    getAllKeys();
}
export class User {
    id_token: string;
    session_state: any;
    access_token: string;
    token_type: string;
    scope: string;
    profile: any;
    expires_at: any;
    state: any;
    toStorageString(storageString?: any);
}

Then it allows me to treat it like an ES6 module in my TypeScript file, like this:

import { UserManager } from "oidc-client/oidc-client";

let manager = new UserManager({...});

I just need to set up my module loader to know where to find it. So in my case, I add some lines to the properties in system.config.js:

    var map = {
        'app': "client/dist/app",
        '@angular': "node_modules/@angular",
        'angular2-in-memory-web-api': "node_modules/angular2-in-memory-web-api",
        'rxjs': "node_modules/rxjs",
        "oidc-client": "node_modules/oidc-client/lib"
    };
    var packages = {
        'app': { main: "main.js", defaultExtension: "js" },
        'rxjs': { defaultExtension: "js" },
        'angular2-in-memory-web-api': { main: "index.js", defaultExtension: "js" },
        "oidc-client": {main: "oidc-client.js", defaultExtension: "js"}
    };

I don't need to have anything in my references.d.ts file, and theoretically if I had a part of my app that didn't need oidc-client, it wouldn't need to be included in the source for that page. This feels better to me, but I'd like to get some input from an expert. Do you know of anyone we could run this by?

brockallen commented 8 years ago

Sorry -- no idea. I'll close for now since you seem to have a workaround. Reopen if you need.

j2jensen commented 8 years ago

For future reference, the changes made in v1.2 now make it possible to do what I wanted.

brockallen commented 8 years ago

Ah, good to hear. Thanks.