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

ServerSide Rendering #36

Closed
Crazyht opened this issue Jul 5, 2017 · 61 comments
Closed

ServerSide Rendering #36

Crazyht opened this issue Jul 5, 2017 · 61 comments

Comments

@Crazyht
Copy link

Crazyht commented Jul 5, 2017

Hi,

I used angular 4 with asp core server rendering (based on https://github.com/MarkPieszak/aspnetcore-angular2-universal). This library is compliant with server side rendering?

If not what you advise for ?

Thank

@damienbod
Copy link
Owner

Is should be but I have tested it. For the server side rendering you need to replace the sessionStorage with for example cookiestorage.

@damienbod
Copy link
Owner

This is supported with the setStorage function which needs to be called before the setupModule

@damienbod
Copy link
Owner

this.oidcSecurityService.setStorage(cookieStorage);

@damienbod
Copy link
Owner

If this doesn't work, you would have to edit the following class:

https://github.com/damienbod/angular-auth-oidc-client/blob/master/src/services/oidc.security.common.ts

@damienbod
Copy link
Owner

I will add this feature, test, example unless you would like to do it

@robisim74
Copy link
Contributor

robisim74 commented Jul 6, 2017

@damienbod You don't need to store the token in a cookie for Server side rendering. You only need to not manipulate the DOM and browser objects like cookies and storage. I added the support for SSR with this commit: 29a4901
Why was it removed?

@damienbod
Copy link
Owner

damienbod commented Jul 6, 2017

@robisim74 I removed this by mistake. I had to remove the provider from the constructor, but removed too much. Thanks for the info. I'll add it back today.

@damienbod
Copy link
Owner

damienbod commented Jul 6, 2017

@robisim74 fixed in version 1.1.4, thanks for the comment

@robisim74
Copy link
Contributor

robisim74 commented Jul 6, 2017

@damienbod no problem. But we should also test the library in the Mark Pieszak app: we still have also a lot of console that maybe could create problems if the logging is enabled.

@Crazyht
Copy link
Author

Crazyht commented Jul 6, 2017

hi,

console are not problem, probably navigation are (window.location). For managing this problem, I think best way is create INavService and provide 2 implementation (server & client)

@robisim74
Copy link
Contributor

@Crazyht You are right, also window.location could be a problem. @damienbod Before creating a new service, we should test if we can detecting it: after the bootstrap, it would not be a problem anymore.

@Crazyht
Copy link
Author

Crazyht commented Jul 6, 2017

You can detect client or server side by using PLATFORM_ID available by DI.

@astegmaier
Copy link
Contributor

I'd love to help make this happen. I created a test repo to try out the current state of things that I can update to help validate any changes that get made. It looks like @Crazyht was correct, because I get errors related to window being undefined when I turn on SSR--see the readme in the test repo for details.

@damienbod
Copy link
Owner

@Crazyht @astegmaier @robisim74 super thanks. The test repo is great.

TODO

  • find a way of repacing the window
  • do we need to replace the console logging for server side rendering?
  • ?
  • How does the storage work in the server rendering?

I would really like to get this working as well.

Greetings Damien

@astegmaier
Copy link
Contributor

astegmaier commented Jul 7, 2017

@damienbod I think I can help answer some of those questions:

  • console.log does seem to work fine in node (see documentation), so I don't think that will be a problem.
  • for storage, I think the design I've used in the test repo should work in general. That approach uses a storage service that is injected with different implementations for client-side rendering and server-side rendering (see storage-config.ts for details).
    • the client-side implementation uses bouzuya's cookie-storage so that tokens end up getting saved as cookies that get passed up to the server.
    • the server-side implementation uses memorystorage and automatically populates itself based on values from the cookies.
    • The benefit of doing things this way is that if you've previously logged in (which causes a cookie to be set by angular-auth-oidc-client through cookie-storage), this auth token will also be seamlessly available on the server, so that you can pre-render any protected content.
  • as to the best way to replace all window references, I'm not 100% sure--it sort of depends on what role they are playing. If they are not doing anything that's relevant to SSR, the best approach would probably be to gate them behind a check of PLATFORM_ID--Like @Crazyht suggested.

@robisim74
Copy link
Contributor

robisim74 commented Jul 8, 2017

@damienbod I pushed a new branch for SSR: https://github.com/damienbod/angular-auth-oidc-client/tree/SSR

Thanks to the test repo of @astegmaier I was able to narrow critical points, and to include them in PLATFORM_ID as suggested by @Crazyht with this commit: f473bb6#diff-3551bf70e40e99dfec0656111f37bf52R67

The problems are in oidcSecuritySilentRenew and in oidcSecurityCheckSession, where you have made extensive use of the DOM: so for the moment I think the only solution is to bypass them: in the end this is a client library and the important thing is that it works when the browser is running.
About authWellKnownEndpoints: I also excluded it, but it actually works even during SSR, just enough that the authentication server is running.

To test the branch, download it and run:

npm install
npm run build
npm run pack-lib

Then you can install it in the test repo:

npm install [path]angular-auth-oidc-client-1.1.5.tgz

@Crazyht
Copy link
Author

Crazyht commented Jul 8, 2017

usage of location is problem too. For read you can get information with request abstraction like Mark implement it. For write is more complex because no standard practice is provided by Angular, Little service can use window can client and ??? something on server, no standard way, probably something like transfer data to caller (.NET or nodejs) and server response with HttpRedirect like he want.

In my opinion your lib must provide interface which can be override/implement by project. You probably can't find perfect implementation for SSR usable on Node or .NET (without lot of doc on how tweak your server side code).

Tank for your reactivity, I test this branch tomorrow:)

@astegmaier
Copy link
Contributor

astegmaier commented Jul 8, 2017

That was fast--thanks! I'll try to test it out on my end to see if it's working. As far as whether to bypass functionality in angular-auth-oidc-client or try to rewrite it so it can work on the server, I think we can safely omit anything that requires user interaction (because there is none of that on the server). The things that (ideally) would be able to work would be:

  1. Retrieving a previously-stored auth token<-this way if the JavaScript on the server tries to make a call to a protected API to load data during the pre-rendering process, it will still work.
  2. Checking if the user is signed in <- that way, if you have any conditional UI that needs to be prerendered (e.g. a navigation link to a protected page), all the JavaScript checks that worked in the browser also work during SSR.
  3. Silently refreshing an expired token. Same as Add support for the id_token flow  #1, but if the token is expired, things still work.

@robisim74 do you think your changes as-is will work with these scenarios? I'm happy to help brainstorm /test/look deeper if you think not.

@damienbod
Copy link
Owner

@Crazyht @robisim74 @astegmaier Really big thanks! I'll test it against browser app. As soon as I get the OK from ye, I'll make the npm package.

Greetings Damien

@robisim74
Copy link
Contributor

@astegmaier As said, this library was thinked for the client, not for the server, and its name is angular-auth-oidc-client. However it is important that it does not generate errors during the SSR. If you need actions during the SSR, you should work server side: in theory the server knows and can store the token, it can decode the token, it knows if the token is still valid to know if the user is authenticated; but these things should be developed on the server side.

@damienbod

  • Maybe we can provide interfaces, as said by @Crazyht (I think it would be a good idea: we could provide class-interfaces that can be overwritten by Angular DI, as I did for storage in this library: https://github.com/robisim74/angular-l10n/blob/master/src/services/locale-storage.ts).
  • About silent renew and check session: why did you use a javascript approach? Why do you need to create an element in the DOM? I think we should rewrite those classes (using DI? using an Angular directive?), because they will give you other problems.

@damienbod
Copy link
Owner

@robisim74 interfaces sound good.
check session : The norm is specified like this.
silent renew : no reason, this could/should be re-written.

We need to maybe rethink the whole silent renew. Have also interesting issues about this in this repo:

https://github.com/damienbod/AspNet5IdentityServerAngularImplicitFlow/issues

@robisim74
Copy link
Contributor

robisim74 commented Jul 8, 2017

I see. Even if the norm is specified like this, I think it does not specify language, right? So we could use Angular Renderer2 class, which allows you to work on the DOM without incurring any errors.
Here another example on how I used Renderer2 in a Directive to support SSR: https://github.com/robisim74/angular-l10n/blob/master/src/models/base-directive.ts but many other examples can easily be found.

@damienbod
Copy link
Owner

yes. We can implement silent renew anyway we want. check session needs to follow the norm, but if we use Renderer2, it does not matter. Just the iframe,message is defined,

I'll release 1.1.5 as it is, and the interface for the storage and Renderer2 refactor in the next, later versions?

@robisim74
Copy link
Contributor

I'd rather wait for the test result of @astegmaier and @Crazyht for 1.1.5, and yes, then we can rewrite the classes.
I had problem with testing, because we would need to create a full test app (SSR + authentication server).

@damienbod
Copy link
Owner

damienbod commented Jul 8, 2017

I'll setup the test in this branch:
https://github.com/damienbod/angular-auth-oidc-client-SSR-test

I'll add you as admin, will wait for feedback anyway. @Crazyht @astegmaier

@damienbod
Copy link
Owner

@robisim74 repo is running with own STS. The window item causes problems. I'll replace the npm package with the src code from the npm.

@damienbod
Copy link
Owner

@astegmaier
Copy link
Contributor

@damienbod I took a look at your branch, and I like the changes - I merged them back into my repo, and made a few more to try to hook things up end-to-end. (Was your intention that we both work together on your branch? If so, let me know, and we can move there).

My initial results are that the changes @robisim74 made do fix the errors for the initial repo, but that when I try to do a more complete implementation, things break once again. Specifically, when I added an ngOnInit method to my controller receive the auth token (see home.component.ts):

    ngOnInit() {
        if (window.location.hash) {
            this.oidcSecurityService.authorizedCallback();
        }
    }

I get a new exception:

Exception: Call to Node module failed with error: Error: Uncaught (in promise): ReferenceError: window is not defined
ReferenceError: window is not defined
at HomeComponent.ngOnInit 

Another weird thing I'm seeing: it looks like the tokens are still being stored in localStorage, even though I have called the setStorage method with my injected Cookie/In-memory implementations.

@robisim74 Sorry this is a bit of a moving target (and thanks for your help so far!), but I suspect we'll uncover more issues as we flesh out a complete implementation of the pre-rendered client (since the current tests exercise only very basic functionality).

I think the right thing to do at this point might be to flesh out the test application with pre-rendering turned off (making sure to exercise all the major functionality of angular-auth-oidc-client). Then we can turn pre-rendering back on, and use it is a baseline to go see what else might be broken. @damienbod can you help me here? I'm a bit new to the library and oidc, so while I understand the basics, I'm not confident I will hit everything in the right way.

@damienbod
Copy link
Owner

Would you like to add the CustomStorage and the changes in the common in a pull request, otherwise I will add it, if it's ok with you?

@damienbod
Copy link
Owner

@Crazyht How do you solve the problem with the window item?

@Crazyht
Copy link
Author

Crazyht commented Jul 8, 2017

WIndow still in code, but server don't receive token so don't try refresh :)

I will try with cookie-store may be SSR try refresh if token come in cookie.

@Crazyht
Copy link
Author

Crazyht commented Jul 8, 2017

You can add CustomStorage no problem

@robisim74
Copy link
Contributor

robisim74 commented Jul 8, 2017

@damienbod The test app is not configured yet with authentication server: missing login etc.

Before change the library, we should do as said by @astegmaier:

I think the right thing to do at this point might be to flesh out the test application with pre-rendering turned off (making sure to exercise all the major functionality of angular-auth-oidc-client). Then we can turn pre-rendering back on, and use it is a baseline to go see what else might be broken.

And I think that's what @Crazyht is doing in his repository.

However for now the library (1.1.5) doesn't break the SSR: but it will be important to provide the ability to overwrite the storage class: #37

@MarkPieszak
Copy link

MarkPieszak commented Jul 8, 2017

Sorry I'm late to the party :)
Do we know where it's erroring out during a server render?

Also is the code being wrapped with isPlatformBrowser() and the service grabbed via the Injector?
An example below: (Demo of how AppInsightsService is injected browser-only)

@Component({ /* etc */ })
export class ExampleComponent {
    private AIService: AppInsightsService;
    private isBrowser: boolean;

    constructor(@Inject(PLATFORM_ID) private platformId, private injector: Injector) {
        this.isBrowser = isPlatformBrowser(this.platformId);
    }

    ngOnInit() { // <-- 
        if (this.isBrowser) { // <-- only run if isBrowser
            this.AIService = <AppInsightsService>this.injector.get(AppInsightsService); // <-- using the Injector, get the Service
            this.AIService.trackEvent('Testing', { 'user': 'me' });
        } 
    }
}

Not familiar with this library here, but are you trying to ignore it completely during server-renders? That's the idea with the code above.

As for Storage on the server, you could just use some D.I. and swap it out for another Class with getItem/setItem methods on it to be used during the render?

Anyway hope that helps!

@robisim74
Copy link
Contributor

Hi @MarkPieszak, thank you for your interest! At the moment we applied the logic above in the library, and we have no more errors during the SSR.

The issue has now moved on how to communicate the access token between server and client without the SSR process interfering.

@damienbod
Copy link
Owner

damienbod commented Jul 8, 2017

@robisim74 I'm having trouble getting the test app to run. once it runs, I'll add the login logic. The STS server is configured for this.

@robisim74
Copy link
Contributor

Which problems do you have?

However now the problem is not the SSR: they are trying to access to the token from node, and this is not possible: Storage and cookies live in the browser.

After I'll refactored the storage, they will able to override it with a custom implementation, so that they will access to the token from node. But this is another issue.

@damienbod
Copy link
Owner

I have runtime errors in the client app, I'll get this running and add a login

@damienbod
Copy link
Owner

in test version 1.2.0

@astegmaier
Copy link
Contributor

I made a few tweaks to damien's test fork of the test repo so that I can test out the latest changes end-to-end. As far as server runtime errors, things look great. Thanks so much for addressing this!

(I had some comments/concerns about the storage interface changes, but I'll post them on on #37).

@k-schneider
Copy link

Wondering what the status of this is since there hasn't been any activity here in a while. Does this library work with Universal? Thanks!

@damienbod
Copy link
Owner

@astegmaier has a running example, correct?

Greetings Damien

@lgoudriaan
Copy link

I didn't get it to work.

I have tried to get it to work in my own test repository here: https://github.com/lgoudriaan/oidc-ssr-example

All default angular functionalities break after login, does anyone have an idea what I am doing wrong?

@astegmaier
Copy link
Contributor

I never actually got it working end-to-end. However, I was able to follow Priyantha Lankapura's excellent example to add and configure the angular-oauth2-oidc package. I tried it out with the new angular-latest Angular-Cli-based templates, and finally got everything working in this repository.

I would bet that it would be relatively easy to use the same approach for Damien's library, but I haven't tried it out yet.

@damienbod
Copy link
Owner

@lgoudriaan @astegmaier
I'll create an example then.

Greetings Damien

@esoderquist
Copy link

I encountered the same issues described by @lgoudriaan after adding angular-auth-oidc-client into a project based originally on aspnetcore-angular2-universal. Traced the issue to the preboot@5.x library. Migrating preboot to version 6.0.0-beta.1 resolved the issue for me.

This preboot commit removes an ApplicationRef isStable check that led to preboot never completing on page load if underlying setTimeout or similar existed (as is the case with this library background timer() calls for token refresh).

@lgoudriaan
Copy link

lgoudriaan commented Jan 7, 2018

Hi, @esoderquist, Can you please make a quick sample? That will help me a lot! I am still doing something wrong I think 😢 Nvm got It working, had to add it to app module. Thanks for the hint!

@chriseugenerodriguez
Copy link

chriseugenerodriguez commented Feb 8, 2018

@lgoudriaan can you paste your solution to this with example please?

@lgoudriaan
Copy link

@premiumwd set preboot to ^6.0.0-beta.1 then remove all preboot references. After that add the import import { PrebootModule } from 'preboot'; to your app.module.ts and PrebootModule.withConfig({ appRoot: 'app' }), to your imports.

That's it :)

@chriseugenerodriguez
Copy link

chriseugenerodriguez commented Feb 9, 2018

@lgoudriaan getting errors. core.es5.js:1084 ERROR Error: No provider for InjectionToken DocumentToken! Unhandled Promise rejection: No provider for InjectionToken DocumentToken! ; Zone: ; Task: Promise.then ; Value: Error: No provider for InjectionToken DocumentToken!

@lgoudriaan
Copy link

I can't help you with this error, you can look at the answer of @astegmaier for starter repositories with SSR. Then Install this library, and to fix the server side rendering upgrade preboot to 6.0 using my instructions above. Hope that helps!

@chriseugenerodriguez
Copy link

chriseugenerodriguez commented Feb 12, 2018

@lgoudriaan no thanks, built static auth file, and updated my authguard. got it working.

@okanasl
Copy link
Contributor

okanasl commented Jul 29, 2018

I've an app using angular 6.0.9 with angular-auth-oidc-client 6.0.2. I have no problem with browser but, even I set my own cookie store implementation as storage, default storage as sessionStorage brokes SSR with error.

  sessionStorage is not defined
    at new OpenIDImplicitFlowConfiguration (.../dist/server.js:131896:24)

Here is the relevant code executed by node server which I am not related any of these.


var OpenIDImplicitFlowConfiguration = /** @class */ (function () {
    function OpenIDImplicitFlowConfiguration() {
        this.stsServer = 'https://localhost:44318';
        this.redirect_url = 'https://localhost:44311';
        this.client_id = 'angularclient';
        this.response_type = 'id_token token';
        this.resource = '';
        this.scope = 'openid email profile';
        this.hd_param = '';
        this.post_logout_redirect_uri = 'https://localhost:44311/unauthorized';
        this.start_checksession = false;
        this.silent_renew = true;
        this.silent_renew_url = 'https://localhost:44311';
        this.silent_renew_offset_in_seconds = 0;
        this.silent_redirect_url = 'https://localhost:44311';
        this.post_login_route = '/';
        this.forbidden_route = '/forbidden';
        this.unauthorized_route = '/unauthorized';
        this.auto_userinfo = true;
        this.auto_clean_state_after_authentication = true;
        this.trigger_authorization_result_event = false;
        this.log_console_warning_active = true;
        this.log_console_debug_active = false;
        this.max_id_token_iat_offset_allowed_in_seconds = 3;
        this.storage = sessionStorage;
    }
    return OpenIDImplicitFlowConfiguration;
}());

@rcanpahali
Copy link

@DooMachine Is it possible for you toe share your test project with us?

@okanasl
Copy link
Contributor

okanasl commented Nov 29, 2018

@DooMachine Is it possible for you toe share your test project with us?

https://github.com/DooMachine/MicroStarter

@damienbod
Copy link
Owner

closing this, please open if you still have an issue

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

No branches or pull requests