Generic Microsoft Graph API Wrapper

[pri-kise]

This is a first draft for a generic microsoft graph api wrapper as mentioned in issue microsoft/BCApps#329

Dummy Script for Testing

    internal procedure FetchGroups()
    var
        TenantInformation: Codeunit "Tenant Information";
        ResponseContent: Text;
        TempBlob: Codeunit "Temp Blob";
        MicrosoftGraphClient: Codeunit "Microsoft Graph Client";
        MicrosoftGraphAuth: Codeunit "Microsoft Graph Auth.";
        MicrosoftGraphAuthorization: Interface "Microsoft Graph Authorization";
        TenantId: Text;
        ClientId: Text;
        ClientSecret: Text;
        ResponseInStream: InStream;
    begin
        //TODO: Replace with Setup
        // TenantId := TenantInformation.GetTenantId();
        // if TenantId = '' then
        TenantId := '';
        ClientId := '';
        ClientSecret := 'xxx';


        MicrosoftGraphAuthorization := MicrosoftGraphAuth.CreateAuthorizationWithClientCredentials(TenantId, ClientId, ClientSecret, 'https://graph.microsoft.com/.default');
        MicrosoftGraphClient.Initialize(Enum::"Microsoft Graph API Version"::"v1.0", MicrosoftGraphAuthorization);
        ResponseInStream := TempBlob.CreateInStream();
        if not MicrosoftGraphClient.Get('groups', ResponseInStream) then
            Error(MicrosoftGraphClient.GetDiagnostics().GetResponseReasonPhrase());
        ResponseInStream.Read(ResponseContent);
        Message(ResponseContent);
    end;

[pri-kise]

@jwikman after our discussions on the sharepoint pull request, I created a small draft for a generic microsoft graph API wrapper.
I know there are missing additional query parameters. Maybe this could be implemented similiar to the azure blob storage by providing an additional codeunit for optional parameters.

Maybe you can take a look at the current draft

[PeterConijn]

Looks good! I am certainly looking forward to when its fully implemented.

[pri-kise]

Looks good! I am certainly looking forward to when its fully implemented.
@PeterConijn I was still thinking if I should implement some kind of optional parameter codeunit pattern for the request methdos to enable a more simplified pattern to provide query paramters and maybe later some header parameters.
I thought that I could implement somehting like the following codeunit 9047 "ABS Optional Parameters"....

And I could need some help/idea/draft in writing tests, since I don't know how to write helpful tests for such a generic API wrapper.

[pri-kise]

@darjoo could you maybe take a short look at this PR and evaluate if such a module could be accepted in general and if you agree with the basic architecture, before I start improving the code for BC22 and fixing the open comments

[darjoo]

@pri-kise Yeah, we will accept the module.

Few things that pop up:

• The events, OnBeforeSendRequest and OnBeforeGetToken should be removed. OnBeforeSendRequest would introduce a security issue allowing one to overwrite the outgoing request. I'm not sure what scenario OnBeforeGetToken is really used for.
• Are the modules split because the authorization can be used with others like Sharepoint etc?
• We'll want to decide on a common prefix for this module's object names. Not as important right now since we can easily do renamings.
• CreateAuthorizationWithClientCredentials, that should only be required for OnPrem, and I think we have an AAD setup page etc somewhere that allows you to set your AAD creds. OnSaaS, GetAccessToken should always work, fx

  ALAppExtensions/Apps/W1/Email - Outlook REST API/app/src/EmailOAuthClient.Codeunit.al

  Lines 27 to 53 in 68d0212

  	local procedure TryGetAccessTokenInternal(var AccessToken: Text)
  	var
  	AzureAdMgt: Codeunit "Azure AD Mgt.";
  	UrlHelper: Codeunit "Url Helper";
  	EnvironmentInformation: Codeunit "Environment Information";
  	OAuthErr: Text;
  	begin
  	Initialize();
  	ClearLastError();
  	if EnvironmentInformation.IsSaaSInfrastructure() then begin
  	AccessToken := AzureAdMgt.GetAccessToken(UrlHelper.GetGraphUrl(), '', false);
  	if AccessToken = '' then begin
  	Session.LogMessage('000040Z', CouldNotAcquireAccessTokenErr, Verbosity::Error, DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher, 'Category', EmailCategoryLbl);
  	if OAuth2.AcquireOnBehalfOfToken('', Scopes, AccessToken) then;
  	end;
  	end else
  	if (not OAuth2.AcquireAuthorizationCodeTokenFromCache(ClientId, ClientSecret, RedirectURL, GetOAuthAuthorityUrl(), Scopes, AccessToken)) or (AccessToken = '') then
  	OAuth2.AcquireTokenByAuthorizationCode(ClientId, ClientSecret, GetOAuthAuthorityUrl(), RedirectURL, Scopes, Enum::"Prompt Interaction"::None, AccessToken, OAuthErr);
  	if AccessToken = '' then begin
  	if AzureADMgt.GetLastErrorMessage() <> '' then
  	Error(AzureADMgt.GetLastErrorMessage());
  	Error(CouldNotGetAccessTokenErr);
  	end
  	end;

Overall it looks good besides the top few for now. If you need further discussions/reviews, ping Jesper and he'll find someone on the team. I'll be away for the next two weeks :)

[pri-kise]

I copied some logic of the other API Modules.
The event OnBeforeSendRequest should only be used for Mock some dummy tests similiar to the sharepoint module.

I've split the modules, to enable later usage for the sharepoint module. Currently the sharepoint module makes use of the old Rest Api, but I'd like to make it possible to create a sharepoint module v2 that uses the graph api module.

Could you maybe elaborate why 'CreateAuthorizationWithClientCredentials' should only be used OnPrem?
I prefer to use this for App Registration to be able to use a api via job queu without user interaction. But I'm no expert in security.

[darjoo]

Understandable, we're trying to get away from adding events as a method to just test/mock and leaving those events in prod. I don't have the exact way we should go about testing this at the moment, but I'm sure we can come up with something better. Which has been a very bad habit of doing things especially from BaseApp days. Test specific events, which inturn can be used also in prod. Especially for OnBeforeSendRequest, that wouldn't pass a security review just due to the possibility of any extension overwriting the contents.
Similarly to the Azure Blob Storage which uses Azurite for testing, maybe we can do the same here.. Found this but it's still in preview https://github.com/microsoftgraph/msgraph-developer-proxy/

Sounds good, probably not a v2 of the module but updating the internals to use this (if possible).

As far as I know, the way we've integrated with graph has been to use the token directly provided by platform. Is there reason to be using another tenant/client id & secret? (There may be a valid case, I just don't know what it is) Whereas, OnPrem doesn't sign in with graph automatically, and as such will need to provide their own client Id, client secret.

[pri-kise]

I don't have the exact way we should go about testing this at the moment, but I'm sure we can come up with something better.
Me neither. That's one of the reason why i marked this as draft currently.

As far as I know, the way we've integrated with graph has been to use the token directly provided by platform. Is there reason to be using another tenant/client id & secret? (There may be a valid case, I just don't know what it is) Whereas, OnPrem doesn't sign in with graph automatically, and as such will need to provide their own client Id, client secret.

I was thinking about providing the possibility for other partners to create specific app registrations for any specific scenario where they want to assign a limited group of API permissions in the specific app registration.

[jwikman]

As far as I know, the way we've integrated with graph has been to use the token directly provided by platform. Is there reason to be using another tenant/client id & secret? (There may be a valid case, I just don't know what it is) Whereas, OnPrem doesn't sign in with graph automatically, and as such will need to provide their own client Id, client secret.

@darjoo as discussed in #12960, no-one else but Microsoft should use the tokens provided by platform.
So if we want this to work with AppSource apps and PTEs (and yes, we do! 🙂), the apps would need to provide Client Id and Client Secrets and/or Certificates.

It also gives us the possibility to add the permissions needed for the specific scenario, as @pri-kise writes.

[pri-kise]

@darjoo could you now take a deeper look into the module please?

I removed the events as requested.

If you want to have a prefix I can replace any name with "Microsoft Graph" with Mg e.g.

[JesperSchulz]

Let's try to get this ready for showtime with 2023 Wave 2.

[PeterConijn]

Looks good! I am certainly looking forward to when its fully implemented.
@PeterConijn I was still thinking if I should implement some kind of optional parameter codeunit pattern for the request methdos to enable a more simplified pattern to provide query paramters and maybe later some header parameters.
I thought that I could implement somehting like the following codeunit 9047 "ABS Optional Parameters"....

And I could need some help/idea/draft in writing tests, since I don't know how to write helpful tests for such a generic API wrapper.

I am sorry, @pri-kise; the update emails got dropped in my spam filter and I completely lost track. You have done some really nice restructuring, which is being reviewed by MS, as far as I can see.

As far as tests go, you cannot actually test the requests, but what often happens is that on the one hand we create a request body, intercept it and verify its structure and information, and on the other we mimic a response and test the handling of the responses.

For response testing, you can use OperationResponse.SetHttpResponse(HttpResponseMessage); where OperationResponse is codeunit 9156 "Mg Operation Response" and the HttpResponseMessage is a message of your creation. This will need to encompass various scenarios including successful ones and likely failed ones (401, 403, 404, 500, etc.) to test exception handling.

Intercepting requests is trickier, since as @darjoo said, using events to intercept this presents security issues. Interface structures might be an option here; Vjekoslav Babic has some good examples, if you need them. That way, your test app can use injection to mimic the request and run tests on it. Depending on how you implement it, this might expose similar security issues, so it might require some extra thought.

[pri-kise]

@PeterConijn, @darjoo I tried to add some Basic Tests for this generic module.
I added Interface for HttpClient to mock the sending.

Maybe you can take a look if this is the right direction

[PeterConijn]

@PeterConijn, @darjoo I tried to add some Basic Tests for this generic module. I added Interface for HttpClient to mock the sending.

Maybe you can take a look if this is the right direction

That looks good. With the implementations in your tests that pretend to send back responses, you can test whether the module handles those responses in the expected manner. I love it!

[JesperSchulz]

Hi @pri-kise,

what do you think? Is this one ready to get processed? Should we make an attempt to get it checked in?

[pri-kise]

Yes I'd like to make an attempt - I'll be on vacation during the first 3 weeks of september.

[JesperSchulz]

I am putting this one on hold, until we've got a "HttpClient" wrapper module draft from @ajkauffmann, as this PR too contains a wrapper. I want to see if cannot use AJs. We should get a PR any day now!

[pri-kise]

Okay.
Maybe someone else of the community or @ajkauffmann can pick up this PR then otherwise this will make it only to BC23.2, since I'll be on vacation the next 3 weeks and will be only able to contribute starting on october.

I was hoping that this module will be there for directions..
Now this might not be possible.

[ajkauffmann]

As @JesperSchulz said, it will be there very soon.
Happy to look at this PR as well. I have written an integration with Graph for creating application registration etc., so maybe things can be combined?

[JesperSchulz]

Update: The Rest Client module has been checked into the master branch. Once this repository is switched to master, you can uptake it in your module.
We can then backport both to 23.x, if need be.

[pri-kise]

I udpated my PR with the Rest Client Module.

Addtionally i tried to add an OAuth Authentitaction with ClientCredentials to the Rest Client Module.

@JesperSchulz @ajkauffmann I could need some feedback.

[pri-kise]

Closed in favour of PR microsoft/BCApps#318