microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph 🦒
https://docs.microsoft.com/graph/toolkit/overview
Other
943 stars 303 forks source link

[BUG] No longer displays mgt-person after upgrade #3159

Closed grosch-intl closed 6 months ago

grosch-intl commented 6 months ago

Describe the bug

After upgrading package versions, the mgt-person component no longer shows anything. In previous versions it was working, but after upgrade it's not.

To Reproduce Steps to reproduce the behavior:

<mgt-person view="twolines" show-presence person-card="hover" avatar-size="small" user-id="my.email@here.com"></mgt-person>

Expected behavior See the person item like I used to.

Environment (please complete the following information):

Additional context

Package Working Version New Version
@azure/msal-angular 3.0.10 3.0.16
@azure/msal-browser 3.7.0 3.13.0
@microsoft/mgt 3.1.3 4.2.1
@microsoft/mgt-msal2-provider 3.1.3 4.2.1
gavinbarron commented 6 months ago

did you add the necessary registerMgtPeronComponent() function call?

Note the script block here: https://learn.microsoft.com/en-us/graph/toolkit/get-started/overview?tabs=package#use-microsoft-graph-toolkit

These function calls were added to enable tree shaking over our library

grosch-intl commented 6 months ago

@gavinbarron Thank you, I didn't see that. I updated my APP_INITIALIZER to call those methods. It now shows the person picker, but if I hover over it, and the card pops up, then a moment later it sends a new token request and the entire page reloads.

This is what my initializer looks like:

export const provideAzure = () => makeEnvironmentProviders([
    {
        provide: APP_INITIALIZER,
        useFactory: (msalService: MsalService, msalBroadcastService: MsalBroadcastService, destroyRef: DestroyRef) => async () => {
            registerMgtMsal2Provider()
            registerMgtPersonComponent()
            registerMgtPersonCardComponent()

            await publicClientApplication.initialize()

            const redirect = await msalService.instance.handleRedirectPromise()

            if (redirect?.account)
                msalService.instance.setActiveAccount(redirect.account)
            else {
                let accounts = msalService.instance.getAllAccounts()

                if (accounts.length === 0) {
                    await firstValueFrom(msalService.loginRedirect())
                    accounts = msalService.instance.getAllAccounts()
                }

                msalService.instance.setActiveAccount(accounts[0])
            }

            msalBroadcastService.msalSubject$
                .pipe(
                    filter((msg: EventMessage) => msg.eventType === EventType.ACQUIRE_TOKEN_FAILURE),
                    takeUntilDestroyed(destroyRef)
                )
                .subscribe(() => msalService.loginRedirect())

            Providers.globalProvider = new Msal2Provider({publicClientApplication})

            MgtPersonCardConfig.sections = {
                files: false, mailMessages: false, organization: {showWorksWith: false}, profile: true
            }
        },
        deps: [MsalService, MsalBroadcastService, DestroyRef],
        multi: true
    },
    {provide: HTTP_INTERCEPTORS, useClass: MsalInterceptor, multi: true},
    MsalService,
    MsalGuard
])
gavinbarron commented 6 months ago

The register component function handle ensuring dependencies are registered too, so you can drop either the person or person card registration call.

At a guess the new issue might be related to the permissions that the user has consented to, we did make some changes in this space that might impact the person card. Specifically, we added a check to see what permissions have already been consented to for each request and compare it to the allowed set before initiating a new consent.

We also made a change to the way that the config for the person card is provided to help with tree shaking which might be at play here depending on the timing of when code runs.

grosch-intl commented 6 months ago

At a guess the new issue might be related to the permissions that the user has consented to, we did make some changes in this space that might impact the person card.

All of the API permissions have been explicitly admin granted so that end users don't get asked.

We also made a change to the way that the config for the person card is provided to help with tree shaking which might be at play here depending on the timing of when code runs.

That code runs at app startup. Suggestions on what I can do? It's not just a single reload either. Every time I hover it reloads the page again.

gavinbarron commented 6 months ago

Have you set the MSAL2 Provider to disable incremental consent? Because if you're doing upfront consent then you really should.

If you have a standalone reproduction then we may be able to investigate. We have not encountered this issue and a reproduction would speed up diagnosis.

grosch-intl commented 6 months ago

@gavinbarron Thanks. I disabled that and it resolved the reload cycles, but now that led to new errors. However, I don't want to keep changing this one so I'll open a separate item for that.