microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
2.83k stars 194 forks source link

Customization of auth providers and plugging in Kiota authproviders into Graph SDK #1006

Closed nikithauc closed 2 years ago

nikithauc commented 2 years ago

Authentication Abstractions in Kiota:

interface AuthenticationProvider {
    authenticateRequest:void;
}
abstract class BaseBearerTokenAuthenticationProvider {
    public authenticateRequest{
        //
    }
    abstract public getAuthorizationToken();
}
class AzureIdentityAuthenticationProvider extends BaseBearerTokenAuthenticationProvider{
    public getAuthorizationToken(){
        // return the access token; 
    }
}

Authentication Abstractions in current Graph JS SDK:

class AuthenticationHandler {
    authProvider: AuthProvider
    middleware.execute() or authenticateRequest(){ 
        if(Graph Specific Condition){
            const token = authProvider.getAccessToken();
            appendToHeader("Authorization", token);
        }
        else {
            deleteAuthHeader();
        }
    }
}
class AuthProvider
{ 
    getAccessToken();
}

Currently the Graph JS SDK uses a composition pattern. The AuthenticationHandler which is a part of the middleware verifies if the request url is a Graph URL or a custom host passed by the user and appends the authorization headers.

Goal :

Reuse Kiota AuthProviders such as the AzureIdentityAuthenticationProvider class and authentication abstractions as is in Graph SDKs.

Limitations:

Adding the customizations: The authenticateRequest in BaseBearerTokenAuthenticationProvidershould append the authorization headers based on the custom conditions. Thereby, the custom logic should also get extended into the child classes such as the AzureIdentityAuthenticationProvider. This limits reusing the existing BaseBearerTokenAuthenticationProvider and the auth providers as is.

Solution:

zengin commented 2 years ago

@nikithauc, can you elaborate more on the solution? Where will these implementations live? Could you also share how the graph SDK customers consume these proposed auth providers with an example?

baywet commented 2 years ago

@nikithauc Thanks for reaching out. To be more specific, the AuthenticationProvider interface doesn't return void, but Promise https://github.com/microsoft/kiota/blob/75a07e3ee63a0ecdae3b1ebb7778799b9a6e3ab8/abstractions/typescript/src/authentication/authenticationProvider.ts#L10

Similar comment for Base Auth provider https://github.com/microsoft/kiota/blob/75a07e3ee63a0ecdae3b1ebb7778799b9a6e3ab8/abstractions/typescript/src/authentication/baseBearerTokenAuthenticationProvider.ts#L27

(don't hesitate to get permalinks from the repo, and paste them straight into a comment, GitHub will generate a preview of the code for you)

The authenticateRequest in BaseBearerTokenAuthenticationProvidershould append the authorization headers based on the custom conditions. Thereby, the custom logic should also get extended into the child classes such as the AzureIdentityAuthenticationProvider. This limits reusing the existing BaseBearerTokenAuthenticationProvider and the auth providers as is.

I'm not sure I understand the issue here, Base Auth Provider implement the Auth Provider interface by calling the GetAuthToken from the child type and adding it to the abstract request as authorization bearer header. Which avoids code duplication. Now if I want to implement an SPFx auth provider for Kiota, all I need to do is derive from base auth provider, and implement only the part that gets the token.

In the case of JS core (for the part that allows arbitrary request), all you need to add is a wrapper:

Here are a few reasons why we diverged away from using a middleware to handle authentication:

Hopefully this clears some of the confusion around design decisions that were made a while ago.

nikithauc commented 2 years ago

@zengin I have tried to explain the solution below. Please let me know if you have more questions.

My suggestion is that we use composition pattern to maintain two interfaces/abstractions separating the:

  1. getAuthorizationToken - This function returns the access token that is received from an authentication library such as MSAL and azure-browser.
  2. authenticateRequest - This function calls the getAuthorizationToken to get the access token and appends the token in the request headers for Authorization.

The structures would be something as follows:

interface AuthProvider {
    getAccessToken();
}

interface Authenticator {
    authenticateRequest();
}

abstract class BaseTokenAuthenticator() {
    private authProvider : AuthProvider;

    constructor(authProvider){};

    public authenticateRequest(){
       const token = authProvider.getAccessToken();
       appendToHeader("Authorization", "token");
    }
}

Example of the implementations:

import {TokenCredential} from "@azure/identity";

class AzureAuthProvider implements AuthProvider {
    getAccessToken(){
        return TokenCredential.getToken();
    }
}

class GraphAuthenticator extends BaseTokenAuthenticator {
    private authProvider : AuthProvider;

    constructor(authProvider){};

    authenticateRequest(){
        if( //some graph condition ){
            const token = authProvider.getAccessToken();
            appendToHeader("Authorization", "token");
        }
        else
        {
         deleteAuthHeader();   
        }
    }
}

A consumer of the Graph JS SDK would provide the AuthProvider instance

import Client from "Graph library";
import AzureAuthProvider from "Kiota AuthProvider";

const graphClient = Client.init({
    authProvider: new AzureAuthProvider();
});

The reason to suggest the separation is that the composition pattern allows to easily customize any function without having to rewrite or reduplicate the child classes.

For example with inheritance if I a user wants to add custom logic in the authenticationRequest for BaseBearerTokenAuthenticationProvider then they would create a MyCustomBaseBearerTokenAuthenticationProvider. But as the AzureTokenAuthenticationProvider is tightly with the coupled the BaseBearerTokenAuthenticationProvider the user would need to rewrite the AzureTokenAuthenticationProvider. This makes Kiota which aims for a larger set of APIs more customizable.

https://github.com/microsoft/kiota/blob/fcc58327b29dd7a3de2c4aa2f9e79dc679550c98/abstractions/typescript/src/authentication/baseBearerTokenAuthenticationProvider.ts#L5

https://github.com/microsoft/kiota/blob/fcc58327b29dd7a3de2c4aa2f9e79dc679550c98/authentication/typescript/azure/src/azureIdentityAuthenticationProvider.ts#L4

@baywet

In the case of JS core (for the part that allows arbitrary request), all you need to add is a wrapper

I think you are referring to the composition pattern here which I have explained above. Is JS core the Graph JS SDK?

Forked API surface, you need to pass the auth provider both to the Graph client and the middleware if you're customizing the middleware, Java SDK used to suffer from that.

Graph JS SDK has a accepts the authprovider as a configuration while creating the client and uses a factory to create the client and delegate it to the middleware.

It mixes concerns: the authentication layer is tied to the http layer, which is an anti-pattern. By separating concerns, we can have a number of different authentication provider implementations work with a number of different http request adapter implementations (typically different native clients).

Agree with this. I do not suggest mixing the two.

baywet commented 2 years ago

Encapsulation over inheritance, I like the suggestion :)

So effectively we'd have

interface AuthenticationProvider { //This doesn't change and exists in Kiota today
    authenticateRequest: (request: RequestInformation) => Promise<void>;
}

interface AccesTokenProvider { // We'd add this interface definition in Kiota abstractions
   getAccessToken: (url: URL| string) => Promise<string>;
}

abstract class BaseBearerTokenAuthenticationProvider() {// already exist in Kiota
    constructor(public readonly accessTokenProvider: AccesTokenProvider ){}; // we're adding this constructor and field

    public authenticateRequest = async (request: RequestInformation) : Promise<void> => {} // already exists today, it'd call the authProvider instead of the abstract method

   // we'd remove the abstract method definition
}

// in each authentication package we'd have two things I'm going to take the example of Azure identity

export class AzureIdentityAuthenticationProvider extends BaseBearerTokenAuthenticationProvider {
    /**
     *
     */
    public constructor(credentials: TokenCredential, scopes: string[] = ['https://graph.microsoft.com/.default']) {
        super(new AzureIdentityAccessTokenProvider(credentials, scopes));
    }
}

export class AzureIdentityAccessTokenProvider implement AccessTokenProvider { // this is new 
    public constructor(private readonly credentials: TokenCredential, private readonly scopes: string[] = ['https://graph.microsoft.com/.default']) {
        if(!credentials) {
            throw new Error('parameter credentials cannot be null');
        }
        if(!scopes || scopes.length === 0) {
            throw new Error('scopes cannot be null or empty');
        }
    }

    public getAuthorizationToken = async (_: URL|string) : Promise<string> => {
        const result = await this.credentials.getToken(this.scopes);
        return result?.token ?? '';
    }
}

In terms of consumption the user would only have to do:

import Client from "Graph library";
import AzureIdentityAuthenticationProvider from "Kiota AuthProvider";

const graphClient = Client.init({
    authProvider: new AzureIdentityAuthenticationProvider(deviceCodeCredentials, scopes),
});

And the client could use the authentication provider as is for the fluent API or the access token provider for the existing arbitrary API. (by accessing the accessTokenProvider property on the BaseBearerTokenAuthenticationProvider)

Now, this adds a level of complexity but adds the advantage of making the providers reusable for both Kiota and the current JS SDK, (as well as other surfaces like MGT etc...)

@nikithauc did I capture properly what you were expressing? @andrueastman do you think we should apply that for all languages? I'm tempted to say yes for uniformity even if it means a bit more work. @sebastienlevert for the experience side of things and the impact on MGT.

sebastienlevert commented 2 years ago

That also relates to a conversation we're having with @waldekmastykarz on the "Bring your own JS SDK" for different scenarios. If we were to go with this approach for the V4 of the SDK, we could have something strong that would decouple and allow getting a V4 client from any authenticated providers (v1, v2, v3). We would own the responsibility to upgrade the packages when breaking changes would happen on the partner teams (TeamsFx, SPFx, etc.) but we feel this might be a minor issue.

Here are a couple of samples

SPFx

import { Client } from "@microsoft/microsoft-graph-client";
import { SharePointAuthenticationProvider } from "@micrososoft/kiota-authentication-sharepoint";

export default class HelloWorldWebPart extends BaseClientSideWebPart<IHelloWorldWebPartProps> { 
  public render(): void {    
    const graphClient = Client.init({
        authProvider: new SharePointAuthenticationProvider(this.context)
    });

    const me = await graphClient.me.get();
  } 
}

TeamsFx

import { 
  loadConfiguration, 
  TeamsUserCredential 
} from '@microsoft/teamsfx'; 

import { Client } from '@microsoft/microsoft-graph-client'; 
import { TeamsFxAuthenticationProvider } from "@micrososoft/kiota-authentication-teamsfx";

loadConfiguration({ 
  authentication: { 
    initiateLoginEndpoint: process.env.REACT_APP_START_LOGIN_PAGE_URL, 
    simpleAuthEndpoint: process.env.REACT_APP_TEAMSFX_ENDPOINT, 
    clientId: process.env.REACT_APP_CLIENT_ID, 
  }, 
}); 

const credential = new TeamsUserCredential(); 
const graphClient = Client.init({
    authProvider: new TeamsFxAuthenticationProvider(credential, ["User.Read"])
});

const profile = await graphClientV3.api("/me").get(); 

This feels very natural (as it is right now with our providers in MGT) and provides huge value for developers in the future.

nikithauc commented 2 years ago

@baywet you have captured my suggestions well :) I like the name AccesTokenProvider!

@baywet @sebastienlevert @zengin

How about maintaining a separate BaseTokenAuthenticationProvider implementation in the Graph JS SDKs and the other Graph SDKs as follows?

export class GraphAuthenticationProvider extends BaseBearerTokenAuthenticationProvider

and the client initiailization using the Graph JS SDK would look like:

const graphClient = Client.init({
    accessTokenProvider: new AzureIdentityAccessTokenProvider();
});

The Client.init static factory method will take care of passing the instance of AzureIdentityAccessTokenProvider to the GraphAuthenticationProvider. This is close to how we wrap the auth provider in the AuthHandler today.

The Graph user will also have the choice to pass an implementation of the AuthenticationProvider.

The reason I am suggesting a separate GraphAuthenticationProvider is to allow any Graph specific modifications and extensions in the future. We can flexibly make fixes in the GraphAuthenticationProvider in case of any bugs or issues reported by a Graph user.

As for a Kiota user, introducing AzureIdentityAuthenticationProvider and implementations something like MSALAuthenticationProvider is good suggestion.

andrueastman commented 2 years ago

@andrueastman do you think we should apply that for all languages? I'm tempted to say yes for uniformity even if it means a bit more work.

Yeah. I think we should definitely do it if we can. There are definitely advantages here that would benefit all the SDKs across the board.

waldekmastykarz commented 2 years ago

@sebastienlevert, I'd suggest that we avoid introducing a SharePointAuthenticationProvider or TeamsFxAuthenticationProvider in Kiota because it tightly couples Kiota with toolchains and leads to a matrix of versions we'd need to maintain in case the logic to get a token in teamsfx or SPFx changes or in case the shape of our provider changes. Also, whenever someone else builds a new toolchain, we'd either need to support it in Kiota or have developers use a different way to get an authenticated Graph client for that toolchain, which would be confusing to them.

Assuming that each toolchain already returns an authenticated Graph client, I'd suggest that we use the Graph client as the input to upgrade to a newer version of the client and not assume any knowledge of toolkits external to Kiota and Graph JS SDK.

baywet commented 2 years ago

@waldekmastykarz I'm curious about those assumptions. Surely the developer knows in which "surface" their code lives (SPA, SPFx, TeamsFx...). So if we tell them "when in SPFx, use that, when in teamsFx, use this" should be straightforward to them. Anyway the rest of the code will change (how to access the context/token etc...). It feels like we wouldn't be pushing extra burden on them. And the benefit for us would be to have small, dedicated layers instead of a mess of probing logic in a single provider (most likely with any all over the place). Should SPFx "break something", we can major rev just the package for SPFx, no impact on other audiences. Lastly I'd argue that having those providers in Kiota is a good thing, since ultimately people could be generating SDKs with kiota for LOBs APIs behind MIP, Dynamics and more. And call those from SPFx, teamsFX etc... Using the same providers for Graph, they'd be able to call those APIs, which sounds like a huge added benefit overall.

nikithauc commented 2 years ago

Should SPFx "break something", we can major rev just the package for SPFx, no impact on other audiences.

@sebastienlevert This is something keep in mind and discuss about when considering the same version numbers for the packages.

waldekmastykarz commented 2 years ago

Surely the developer knows in which "surface" their code lives (SPA, SPFx, TeamsFx...). So if we tell them "when in SPFx, use that, when in teamsFx, use this" should be straightforward to them.

If you think about guidance for each toolchain, like use the TeamsFxAuthenticationProvider in teamsfx or use the SharePointAuthenticationProvider in SPFx then it makes perfect sense and would absolutely work. The problem is when you add into equation a new toolchain that's not supported in Kiota (either built by us at Microsoft or in the community). At that point we'd need to say: use our Kiota providers for x and y but when in z you're on your own, which isn't a great message, because it requires developers to learn multiple ways to do the same thing.

Lastly I'd argue that having those providers in Kiota is a good thing, since ultimately people could be generating SDKs with kiota for LOBs APIs behind MIP, Dynamics and more.

This is a good point that changes the discussion slightly. Because we're talking about custom SDKs that aren't exposed by a toolchain, here it makes perfect sense to have providers that you can use to pass the toolchain's context to instrument an instance of SDK client. I'd argue that it would be better to keep providers together with toolchains rather than in Kiota, because then everything related to the toolchain is in one place and Kiota doesn't need to know about any of the projects using it. I'd also make a point that we should strive for consistency. Today, toolchains expose an authenticated Graph client to let developers call Graph easily. But that's just Graph. If we think about creating custom SDKs with Kiota, why introduce two ways to use them (you can get Graph directly from us, but for your own SDK you need to instantiate it yourself)? Why not use the same approach for all SDKs, where the toolchain exposes a provider that developers can pass to any Kiota-based SDK in a consistent manner?

sebastienlevert commented 2 years ago

Adding @AJIXuMuK from the ODSP team for his opinion on this approach for SPFx. This could help with both providing more flexibility for SPFx developers and reducing the burden on the ODSP team.

AJIXuMuK commented 2 years ago

My opinion is having access token/auth provider in toolchain seems natural (there are other concerns how SPFx and Graph SDK should communicate, but it's not related to the topic). The only thing in mind - public interfaces for the provider should not change inside major release. Otherwise toolchain's dev teams will be forced to update providers prior to the next release of the Graph SDK.