AzureAD / microsoft-authentication-library-for-js

Microsoft Authentication Library (MSAL) for JS
http://aka.ms/aadv2
MIT License
3.66k stars 2.65k forks source link

Defer configuration loading in dedicated method instead of controller #1665

Open vlafranca opened 4 years ago

vlafranca commented 4 years ago

1090 Library

Description

Hi everyone, In my situation, I need to load oidc configuration from a remote server, so I need to make an http call to retrieve it. I have read all the docs and tryed all the ways to load configurations but nothing suits my need and I think I won't be the only one looking for this. I am working on a security library which will be embedded in many other project, the goal is to obfuscate any auth security process within this lib which will expose only basic methods to hosts projects. The libs needs then to handle all Msal init (service instanciation, interceptors, etc) and retrieve the config url via an injection token injected by host app.

The only way to load asynchronously a config file from doc is to use platformDynamic, but I don't want to duplicate that Msal specific configuration scheme in all the applications that will use the library. Also the APP_INITIALIZER provider is not delaying the app bootstrap so the configuration is not loaded on time because MsalService load configuration in constructor.

What I needed to do is to build a custom wrapper service to handle remote configuration loading and manual MsalService instanciation (loading config from constructor). Then set a flag init to true, and use my inner MsalService instance.

It would be nice to have a configure(config) method to be able to manage the lib initilization flow entirely. Instead of init in constructor.

Here is the code I made to have an idea, I would be able to make a PR if you think it is interesting.

@Injectable({
  providedIn: 'root'
})
export class ConfigService {
  public msalService: MsalService;
  private http: HttpClient;
  public configInit = new Subject();
  constructor(httpClient: HttpClient, private router: Router, private broadcasService: BroadcastService) {
    this.http = httpClient;
  }

  configure(uri) {
    this.http.get(uri).pipe(map(res => res))
    .subscribe((value: any) => {
      msalConfig.auth.clientId = value.client_id;
      msalConfig.auth.postLogoutRedirectUri = value.post_logout_redirect_uri;
      msalConfig.auth.redirectUri = value.redirect_url;
      msalConfig.auth.authority = value.stsServer;
      msalAngularConfig.consentScopes = value.scope.split(" ");
      this.msalService = new MsalService(msalConfig, msalAngularConfig, this.router, this.broadcasService);
      this.configInit.next(true);
    },
      (error) => {
      });
  }
}
jasonnutter commented 4 years ago

@vlafranca Thanks for the feedback! This is an interesting use case. Feel free to open a PR that shows what you think this might look like, thanks!

vlafranca commented 4 years ago

@jasonnutter great ! I am working on it and will be soon able to make a PR

vlafranca commented 4 years ago

Hi @jasonnutter I juste submitted a PR #1715

Labeler / label (pull_request) checks fails, can you explain me why ?

jasonnutter commented 4 years ago

@vlafranca The labeler wasn't setup to work with forks, that will be fixed.

vlafranca commented 4 years ago

Ok thanks for your reply, is there any log output in azure to know why is the build failing ? It succeed on my machine ..

Le jeu. 28 mai 2020 à 22:01, Jason Nutter notifications@github.com a écrit :

@vlafranca https://github.com/vlafranca The labeler wasn't setup to work with forks, that will be fixed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/1665#issuecomment-635713021, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB2NUS357IATOZGZY43EGATRT4JQDANCNFSM4NBT5HWA .

vlafranca commented 4 years ago

Any news or estimation about this ?

jasonnutter commented 4 years ago

@vlafranca Not yet, sorry, been busy. We'll update the ticket when we get a chance, apologies for the delay.

vlafranca commented 4 years ago

Ok no problem

ursedaniel commented 4 years ago

Hello, any news into this? It's a really good functionality and we really need it for our project, thanks!

jasonnutter commented 4 years ago

We will look to support updating the MSAL configuration dynamically in MSAL Angular v2 (alpha available now).

vlafranca commented 3 years ago

Ok so this PR is abandoned or need to adapt for v2 ?

On Wed, Nov 11, 2020 at 9:35 PM Jason Nutter notifications@github.com wrote:

We will look to support updating the MSAL configuration dynamically in MSAL Angular v2 (alpha available now https://github.com/AzureAD/microsoft-authentication-library-for-js/releases/tag/msal-angular-v2.0.0-alpha.0 ).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/1665#issuecomment-725790093, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB2NUS6T3MDLCDNEXVAQVRDSPNCYPANCNFSM4NBT5HWA .

vinusorout commented 3 years ago

This can be achieved easily using the Factory providers with APP_INITIALIZER, check following sample https://github.com/vinusorout/msal-angular-dynamic-configuration-sample

vlafranca commented 3 years ago

Yes they brought this feature with MSAL 2.

On Wed, Mar 10, 2021 at 1:55 PM Vinay Sorout notifications@github.com wrote:

This can be achieved easily using the Factory providers with APP_INITIALIZER, check following repo https://github.com/vinusorout/msal-angular-dynamic-configuration-sample

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/1665#issuecomment-795910016, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB2NUS2HJHUW2FQZMSDYE3LTC66DDANCNFSM4NBT5HWA .

vinusorout commented 3 years ago

No, It was also working with MSAL 1.x , check following https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/1403

ozkoidi commented 3 years ago

@jo-arroyo any news on this? I'm using msal-angular 2.0.0 and msal-browser 2.14.2 and sometimes I get an error because the MSAL initialization starts before APP_INITIALIZER finishes reading the JSON configuration file:

image

My project uses PathLocationStrategy(like the angular10-sample-app) but with all routes guarded by MsalGuard, so the users are logged in as soon as they hit the app as explained in the documentation.

You can reproduce the problem in the sample provided by @vinusorout by adding the MsalGuard as I explained in this issue.

Any tip regarding how to make sure MSALInstanceFactory is executed after reading a file asynchronously would be much appreciated.

Depechie commented 2 years ago

hey @ozkoidi did you find a solution for this? I have the same issue.

ozkoidi commented 2 years ago

Hello @Depechie,

They may have fixed the issue in the meantime but in case it helps you, the workaround that worked for me is the following:

Disable the initial navigation in the routing module

@NgModule({
  imports: [
    RouterModule.forRoot(ROUTES, {
      initialNavigation: 'disabled'
    })
  ],

In app.module.ts pass the Router to the initializer method

  ...
  providers: [
    ...,
    {
      provide: APP_INITIALIZER,
      useFactory: initializeApp,
      deps: [Router],
      multi: true
    },
    ...

to call router.initialNavigation(); after reading the configuration file

  export const initializeApp = (router: Router) => async () => {
    await config.load(); <- This is where you load the config file
    router.initialNavigation(); <- This enables the navigation we disabled in the routing module
  };
Depechie commented 2 years ago

@ozkoidi thx for the details... but no matter what I try, as soon as I hit the http.get in my config.Load to read out the /assets/config.json, I see in the logging that other elements will still get called before the file is read.

Cfr screenshot: the config parsed is placed above the router.initialNavigation().

Screenshot (113) .

Is there any running example I can look at? Otherwise I can always try to put a test example on my github ofcourse.

Depechie commented 2 years ago

Got it working, by refactoring the original example by @vinusorout

sameerag commented 2 years ago

@jo-arroyo Are we still planning official support for this feature?

jasonnutter commented 2 years ago

We haven't decided on it, no. It looks like there are workarounds, though.

Gillardo commented 2 years ago

This is exactly what i need