microsoftgraph / microsoft-graph-toolkit

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

Providers should know what scopes to use based on the component type or name #1011

Open nmetulev opened 3 years ago

nmetulev commented 3 years ago

Parent: #720

Description

Add ability for a developer to use the component name or type whenever scopes are needed. For example:

Rationale

Developer are required to know what scopes components need before they can use them in the providers. It's not always clear and is error prone. By allowing our Providers to be "smart" to know what scopes components need, a developer will be able to simply list the components they use to leverage the scopes.

Preferred Solution

In HTML

<mgt-msal-provider client-id="id" scopes="mgt-person, mgt-login, mail.read"></mgt-msal-provider>

In JS/TS

import {Providers, MsalProvider, MgtPeoplePicker} from '@microsoft/mgt';

Providers.globalProvider = new MsalProvider({clientId: bla, scopes: ['mgt-person', MgtPeoplePicker, 'mail.read']});

Note, here we should also be able to use the type of the component, not just the tag name, which helps in type safety

Potential implementation

To do this, we can leverage window.customElements.get to get the constructor of an element by using its tag name. The requiredScopes getter added in (#999) is the other piece that is needed for this to work. Here is a simlified helper function that we could add and leverage in the providers:

function parseScopes(scopes: (string | typeof MgtBaseComponent)[]) {
  let finalScopes = [];
  for (let scope of scopes) {
    if (typeof scope === 'function') {
      if ((scope as typeof MgtBaseComponent).requiredScopes) {
        finalScopes = [...finalScopes, ...(scope as typeof MgtBaseComponent).requiredScopes];
      }
    } else if (scope.startsWith('mgt')) {
      if (window.customElements.get) {
        const type = window.customElements.get(scope);
        if (type && type.requiredScopes) {
          finalScopes = [...finalScopes, ...(type as typeof MgtBaseComponent).requiredScopes];
        }
      }
    } else {
      finalScopes.push(scope);
    }
  }

  return finalScopes; // but first should remove duplicates
}

This function can be used anywhere where scopes are required by the providers, including getAccessToken methods

ghost commented 3 years ago

Hello nmetulev, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

sebastienlevert commented 3 years ago

What about this :

HTML <mgt-msal-provider client-id="id" scopes="mail.read" components="mgt-person, mgt-login"></mgt-msal-provider>

JS/TS Providers.globalProvider = new MsalProvider({clientId: bla, scopes: ['mail.read'], components: [MgtPerson, MgtLogin]});

This would remove the confusion of a scope called "mgt-person" that doesn't really say anything about the actual scopes. By defining the components you are using, it feels more natural.

At build time (or when in a DEV build) it would be great to log the effective scopes used to offer the developer the capability to know exactly what is used if they want to bring it to a proper app registration.

nmetulev commented 3 years ago

Great suggestion, I agree this would be more clear - make sense.

waldekmastykarz commented 1 year ago

I looked into this a while back and the conclusion then was, that it would be challenging, because the scopes that components need depend on their configuration (see for example the scopes required by the people component). Unfortunately, component's configuration is known only after it rendered, which is too late to instantiate the provider. While we could use what we consider the default set of scopes, there's a chance it wouldn't match what the app needs. Alternatively, we could expose all possible scope with the risk of over requesting permissions.

gavinbarron commented 1 year ago

Thanks @waldekmastykarz, agreed, there are a number of challenges with establishing the scopes which are needed based on the configuration of the component that make it impractical.

As a comprise to this @sebastienlevert and I have been discussing this and think that each component will have two sets of scopes, a minimal and a default, the default would be the maximum scopes. Effectively we'd be refactoring the scope lists out of the graph clients and making them available via some helpers.

HTML

<mgt-msal-provider client-id="id" scopes="mail.read" components="mgt-agenda.minimal, mgt-login.minimal"></mgt-msal-provider>`

JS/TS

Providers.globalProvider = new MsalProvider({clientId: bla, scopes: ['mail.read'], components: [MgtAgenda.Minimal, MgtLogin.Minimal]});

This would be coupled with the development of a scope trimming component which would allow us to control rendering of fragments of our components based on the consented scopes when we are in a situation where incremental consent is disabled

waldekmastykarz commented 1 year ago

Do we have some telemetry data about how MGT components are typically configured that would help us understand if the minimal and default sets would be sufficient for real-world MGT usage?

As an alternative, could we extend the MGT playground with an additional section that would display the necessary scope based on the contents of the HTML field? If we could then bring the playground into VSCode, I wonder if that wouldn't be a more flexible solution for our developers.