microsoftgraph / microsoft-graph-toolkit

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

[BUG] [MGT-CHAT] Cannot easily set permission scopes #3120

Open plasne opened 4 months ago

plasne commented 4 months ago

Describe the bug It should be easy to make changes to the enabled features and then derive the appropriate permission scopes. Consider for instance, changes like this...

import { MgtPersonCardConfig } from "@microsoft/mgt-components";
MgtPersonCardConfig.sections.files = false;
MgtPersonCardConfig.sections.mailMessages = false;
MgtPersonCardConfig.sections.organization = { showWorksWith: false };
MgtPersonCardConfig.sections.profile = true;
MgtPersonCardConfig.useContactApis = false;
MgtPersonCardConfig.isSendMessageVisible = true;

That would need to run before the exported constant allChatScopes. Unfortunately, no matter how we set the dependencies to load (even using import module promises), we found inconsistent results. I should mention the implementation was also using Vite, so maybe there is something around packaging. By inconsistent results, I mean most users had MgtPersonCardConfig running first and it worked as expected, but not 100% of the time. A few users never worked.

I propose changing allChatScopes, allChatListScopes, etc. to functions, then it becomes really easy to control when they are computed.

To Reproduce Steps to reproduce the behavior:

  1. Create a file with changes
  2. Run the solution
  3. You may or may not get a smaller scope of permissions requested

Expected behavior 100% of all users get the same behavior 100% of the time.

Screenshots NA

Environment (please complete the following information):

Additional context I am a MSFT FTE and can be reached on Teams at pelasne.

gavinbarron commented 4 months ago

Vite does weird things with bundling in dev mode, and in fact just doesn't work when the ACS ui library components are imported, so that's really fun. So, I'd make sure that you're testing that scenario using a static build and serving the result.

The suggestion to move allChatScopes etc to be functions rather than object so that we can avoid hoisting/timing issues is a good one, thank you!