Prototype of new API with the Graph and Downstream web API services #431

jmprieur · 2020-08-11T13:48:03Z

Proposal for Microsoft.Graph support

Rename CallsWebApi in CallsWebApis, as this becomes a hub for all what calls a Web API. The alternative design would have been to have CallsMicrosoftGraph and CallsDownstreamApi under AddMicrosoftIdentityWebApp / WebApi, however, this would have obliged the developer =to pass-in the ConfidentialClientApplicationOptions to each of the services they want to call (that is several times if they call several services, for instance both Graph, and a DownstreamApi for ARM, and a Downstream for https://myWebApi.
This is now possible having several sections in the appsettings.json. See for instance the Blazor sample which call graph and another API (Graph really, but that would not matter)
Renamed CallsMicrosoftGraph into AddMicrosoftGraphServiceClient as GraphServiceClient this the type that the developer will use in the controller and is what they will look for in the documentation. This method is present under CallsWebApis, because otherwise we'd have to re-pass the initialization for CallsWebApis (the ConfidentialClientApplicationOptions)
Updates the tests to get a feel of the API (In particular WebAppCallsGraph and BlazorServerCallsGraph

Proposal for Downstream web API support

Downstream API support* folder contains options, service, extensions
Aligning the properties for Microsoft Graph options and Downstream API options
Updated the Blazor sample to use both the Graph service and the Downstream API service (BlazorServer calls graph)

Todo list sample

changed the namespace of the model (common to client and service). Ideally we'd put in a shared project. We might do that later.
Updated the TodoListService class to leverage the DownstreamWebApi service. The code is way simpler!!!
updated the AppSettings.json and the Startup.cs to leverage these services
Fixed the Controller of the Web API which was not using the display name when adding an item (list always empty)

-------------------------------------------- 1. Rename CallsWebApi in CallsWebApis 2. Rename CallsMicrosoftGraph into AddMicrosoftGraphServiceClient as this is what the developer will use in the controller, under CallsWebApis, because otherwise we'd have to re-pass the initialization for CallsWebApis (the ConfidentialClientApplicationOptions) 3. Updates the tests to get a feel of the API.

- Downstream API support folder contains options, service, extensions - Aligning the properties for Microsoft Graph options and Downstream API options Updated the Blazor sample to use both the Graph service and the Downstream API service (BlazorServer calls graph) Updated the WebAppCallsGraph service Todo list sample: - changed the namespace of the model (common to client and service). Ideally we'd put in a shared project. We might do that later. - Updated the TodoList service to leverage the DownstreamWebApi service. The code is way simpler!!! - updated the AppSettings.json and the Startup.cs to leverage these services - Fixed the Controller of the Web API which was not using the display name when adding an item.

jmprieur · 2020-08-11T14:02:29Z

tests/WebAppCallsMicrosoftGraph/Startup.cs

-                        .CallsWebApi()
+                        .CallsWebApis()
+                           .AddMicrosoftGraphServiceClient()
+                           .AddMicrosoftGraphServiceClient(Configuration.GetSection("GraphBeta"))


This is really one or the other of these 3 lines;

jmprieur · 2020-08-11T14:03:19Z

tests/WebAppCallsWebApiCallsGraph/Client/Controllers/TodoListController.cs

        {
-            _todoListService = todoListService;
+            _todoListService = new TodoListService(downstreamWebApi);


I'm not too satisfied it, but I did not yet manage to inject the DownstreamApiService.
If we decided to go with this design, we'll have to fix that.

jmprieur · 2020-08-11T14:04:19Z

tests/WebAppCallsWebApiCallsGraph/Client/Services/ITodoListService.cs

@@ -3,7 +3,7 @@

 using System.Collections.Generic;
 using System.Threading.Tasks;
-using TodoListService.Models;
+using TodoList.Models;


I renamed the namespace because:

it's common to the service and client (it's even the same file with a link)

There is was a naming conflict between TodoListService (the namespace for this models), and the TodoListService class in the TodoListClient.

jmprieur · 2020-08-11T14:04:40Z

tests/WebAppCallsWebApiCallsGraph/Client/Services/TodoListService.cs

        {
+            builder.AddDownstreamApiService(TodoListService.ServiceName, configuration);


Just leverage the generic DownstreamApiService

jmprieur · 2020-08-11T14:05:10Z

tests/WebAppCallsWebApiCallsGraph/Client/Services/TodoListService.cs

-            }
-
-            throw new HttpRequestException($"Invalid status code in the HttpResponseMessage: {response.StatusCode}.");
+            return await _downstreamWebApi.CallWebApiForUserAsync<Todo, Todo>(


use strongly typed wrappers and express just what is different. The code is way simpler and no copy/paste any longer.

jmprieur · 2020-08-11T14:05:39Z

tests/WebAppCallsWebApiCallsGraph/Client/TodoListClient.csproj

@@ -23,7 +23,6 @@

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.DataProtection.Abstractions" Version="3.1.1" />
-    <PackageReference Include="Microsoft.Graph" Version="1.16.0" />


Microsoft.Graph is now included in Microsoft.Identity.Web. We don't need to reference it any longer in the samples

jmprieur · 2020-08-11T14:05:53Z

tests/WebAppCallsWebApiCallsGraph/Client/Views/TodoList/Create.cshtml

@@ -1,4 +1,4 @@
-@model TodoListService.Models.Todo
+@model TodoList.Models.Todo


Consequence of the renaming of namespace ..

jmprieur · 2020-08-11T14:06:22Z

tests/WebAppCallsWebApiCallsGraph/TodoListService/Controllers/TodoListController.cs

@@ -76,7 +77,7 @@ public IActionResult Post([FromBody] Todo todo)
        {
            HttpContext.VerifyUserHasAnyAcceptedScope(scopeRequiredByApi);
            int id = TodoStore.Values.OrderByDescending(x => x.Id).FirstOrDefault().Id + 1;
-            Todo todonew = new Todo() { Id = id, Owner = HttpContext.User.Identity.Name, Title = todo.Title };
+            Todo todonew = new Todo() { Id = id, Owner = User.GetDisplayName(), Title = todo.Title };


Lol! we forgot this one when we updated the GetAsync()

jmprieur · 2020-08-11T14:07:04Z

tests/WebAppCallsWebApiCallsGraph/TodoListService/Startup.cs

@@ -33,7 +33,7 @@ public void ConfigureServices(IServiceCollection services)
            // Adds Microsoft Identity platform (AAD v2.0) support to protect this Api
            services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
                    .AddMicrosoftIdentityWebApi(Configuration, "AzureAd")
-                        .CallsWebApi()
+                        .CallsWebApis()


Renaming to express that this is a hub for downstream APIs.
Note that CallsWebApis() really provides the ITokenAcquisitionService .... but I don't think we want to write that?

jmprieur · 2020-08-11T14:08:48Z

tests/Microsoft.Identity.Web.Test/WebAppExtensionsTests.cs

@@ -284,7 +284,7 @@ public async Task AddMicrosoftWebAppCallsWebApi_WithConfigActionParameters()

            var builder = services.AddAuthentication()
                .AddMicrosoftWebApp(_configureMsOptions, null, OidcScheme)
-                .CallsWebApi(_configureAppOptions, initialScopes);
+                .CallsWebApis(_configureAppOptions, initialScopes);


See, in this case, if we had chosen the other design, we'd have to pass-in the _configureAppOptions in all the CallsMicrosoftGraph and CallsDownstreamApis, with a risk of errors (pass-in different things, whereas this would be the same service, really), and duplication of code.

jmprieur · 2020-08-11T14:09:46Z

tests/BlazorServerCallsGraph/appsettings.json

-        "ClientSecret": "secret-goes-here"
+        "ClientSecret": "fvHwfoLA8Zls6Huj5MBUQUDrJ5HXrauFQKDd2CacxJQ="
+    },
+    "GraphBeta": {


This is the case of a sample that calls several sevices:

Graph

And a downstream API (which turns out to be Graph v1.0, but could be something else.

jmprieur · 2020-08-11T14:13:03Z

tests/BlazorServerCallsGraph/Startup.cs

-                        .CallsWebApi(scopes)
+                        .CallsWebApis()
+                            .AddMicrosoftGraphServiceClient(Configuration.GetSection("GraphBeta"))
+                            .AddDownstreamApiService("CalledApi", Configuration.GetSection("CalledApi"))
                        .AddInMemoryTokenCaches();


That's the kind of code that you would have if you want to call several services.

If you want to use ITokenAquisition directly, just use .CallsWebApis()
Otherwise:

if you want to use the MicrosoftGraphServiceClient wrapper, use .AddMicrosoftGraphServiceClient under .CallsWebApis()

if you want to use one or several instances of the downstream API service (they have a name, which here, I've chosen the same as the section in the config file to make it simpler, but that's not absolutely necessary), you woud have one or several calls to .AddDownstreamApiService("NameOfTheService", Configuration.GetSection("SectionInConfig"))

Then in the controller, you'd use IDownstreamWebApi.CallsWebApiForUser("NameOfTheService", and have the possiblity to override the configuration from the config file with a delegate.

jmprieur · 2020-08-11T14:14:11Z

tests/BlazorServerCallsGraph/Pages/CallWebApi.razor


    protected override async Task OnInitializedAsync()
    {
        try
        {
-            apiResult = await downstreamAPI.CallWebApiAsync("me");
+            response = await downstreamAPI.CallWebApiForUserAsync(
+                    "CalledApi",


Here we call the service "CalledApi", on behalf of the user (this maps to some configuration for this service: see below), and we override the relative path to call the "me" endpoint. All the other configurations are overridable as well.

jmprieur · 2020-08-11T14:14:59Z

tests/BlazorServerCallsGraph/Pages/CallWebApi.razor

+            if (response.StatusCode == System.Net.HttpStatusCode.OK)
+            {
+                apiResult = await response.Content.ReadAsStringAsync();
+            }


Here we use the pattern recommended by Damien, but we could also use the strongly typed wrapper CallWebApiForUserAsync<InputType, ReturnType> which makes it even easier. It's used in the TodoListService wrapper in the TodoListClient project.

tests/BlazorServerCallsGraph/appsettings.json

- Cleaning-up renaming done with Damien on the types

src/Microsoft.Identity.Web/DownstreamApiSupport/CalledApiOptions.cs

jennyf19 · 2020-08-12T02:29:11Z

src/Microsoft.Identity.Web/DownstreamApiSupport/DownstreamWebApi.cs

+
+        /// <inheritdoc/>
+        public async Task<HttpResponseMessage> CallWebApiForUserAsync(
+            string optionsInstanceName,


question: should we stick w/consistency of the naming? MicrosoftIdentityCallWebApiForUserAysnc()....i don't like this much, but i think things should feel like they are part of MS identity web, or not...again, will need to think about it.

I kind of feel like I would leave this one as is.

My thought is that we should be careful adding prefixes in too many places since that can create a lot of noise (from my experience).

Here this is a verb (method) on a class that we own, not an extension method on a class that we don't own (like Service Collection). I don't think it's abiguous. I would stick to CallWebApiForUserAsync in this particular case

…ToPrepareServices

src/Microsoft.Identity.Web/DownstreamApiSupport/CalledApiOptions.cs

pmaytak · 2020-08-13T04:39:49Z

src/Microsoft.Identity.Web/DownstreamApiSupport/CalledApiOptions.cs

+        /// <summary>
+        /// Base URL for the called Web API. For instance <c>"https://graph.microsoft.com/beta/".</c>.
+        /// </summary>
+        public string BaseUrl { get; set; } = "https://graph.microsoft.com/v1.0";


If this is a general options class, should the BaseUrl default be an empty string instead then?

pmaytak · 2020-08-13T04:59:27Z

src/Microsoft.Identity.Web/DownstreamApiSupport/DownstreamWebApi.cs

+
+        /// <inheritdoc/>
+        public async Task<HttpResponseMessage> CallWebApiForUserAsync(
+            string optionsInstanceName,


I kind of feel like I would leave this one as is.

My thought is that we should be careful adding prefixes in too many places since that can create a lot of noise (from my experience).

pmaytak · 2020-08-13T05:12:40Z

src/Microsoft.Identity.Web/DownstreamApiSupport/DownstreamWebApi.cs

+                jsoncontent = new StringContent(jsonRequest, Encoding.UTF8, "application/json");
+// case of patch?               jsoncontent = new StringContent(jsonRequest, Encoding.UTF8, "application/json-patch+json");
+            }
+            else


I think this else is unnecessary, jsoncontent would still be null here.

I tried but the compiler was not happy line 131.

pmaytak · 2020-08-13T05:33:24Z

src/Microsoft.Identity.Web/DownstreamApiSupport/DownstreamWebApi.cs

+        }
+
+        /// <inheritdoc/>
+        public async Task<HttpResponseMessage> CallWebApiForAppAsync(


Do we also want to have a generic typed version for this method?

It's more work.

pmaytak · 2020-08-13T05:40:29Z

src/Microsoft.Identity.Web/DownstreamApiSupport/IDownstreamWebApi.cs

+        /// <returns>The value returned by the API.</returns>
+        public Task<TOutput> CallWebApiForUserAsync<TInput, TOutput>(
+            string optionsInstanceName,
+            TInput input,


input > requestContent?
Also maybe TInput, TOutput to TRequestContent, TResponseContent?

No sure about this one. The content is the json. Here input and output are object types (like Todo)

src/Microsoft.Identity.Web/DownstreamApiSupport/IDownstreamWebApi.cs

…ilderRefactoringToPrepareServices

jennyf19 · 2020-08-13T19:51:41Z

This can be closed, correct? @jmprieur you did two smaller PRs instead right?

jmprieur · 2020-08-13T20:16:18Z

No, there are the templates still

jmprieur added 2 commits August 11, 2020 10:34

jmprieur requested review from jennyf19 and pmaytak August 11, 2020 13:48

jmprieur commented Aug 11, 2020

View reviewed changes

jmprieur added 3 commits August 11, 2020 18:56

Updating templates for mvc2 and webapi2

9df4da4

Adding the webapp2 project template.

30ef667

Renaming

6317daf

This was referenced Aug 11, 2020

[Feature Request] Downstream Web APi services from the templates to the library #403

Closed

[Feature Request] CallsMicrosoftGraph in Startup.cs enables usage of the Graph SDK in controllers / Razor / Blazor pages #427

Closed

Committing what signed-off by Damien

e75e7c4

jmprieur commented Aug 11, 2020

View reviewed changes

tests/BlazorServerCallsGraph/appsettings.json Outdated Show resolved Hide resolved

jmprieur added 3 commits August 11, 2020 23:18

Update tests/BlazorServerCallsGraph/appsettings.json

510bcfe

Merging from jennyf/newApi

1a54ed9

- Fixing merge issues

6e3dbb1

- Cleaning-up renaming done with Damien on the types

jennyf19 reviewed Aug 12, 2020

View reviewed changes

src/Microsoft.Identity.Web/DownstreamApiSupport/CalledApiOptions.cs Outdated Show resolved Hide resolved

jennyf19 reviewed Aug 12, 2020

View reviewed changes

jmprieur marked this pull request as draft August 12, 2020 11:01

Sync from jennyf/newAPI branch

0d93993

jmprieur changed the base branch from jennyf/newAPI to jennyf/newApiPlusGraphService August 12, 2020 11:32

jmprieur added 2 commits August 12, 2020 13:35

Merge branch 'jennyf/newAPI' into jennyf/newAPIWithBuilderRefactoring…

755f10c

…ToPrepareServices

Merge

54d4062

pmaytak approved these changes Aug 13, 2020

View reviewed changes

jmprieur added 2 commits August 13, 2020 16:54

Merge branch 'jennyf/newApiPlusGraphService' into jennyf/newAPIWithBu…

a586841

…ilderRefactoringToPrepareServices

Addressing PR feedback

255e800

jmprieur closed this Aug 19, 2020

jmprieur deleted the jennyf/newAPIWithBuilderRefactoringToPrepareServices branch August 19, 2020 16:31

		{
		builder.AddDownstreamApiService(TodoListService.ServiceName, configuration);

		@@ -1,4 +1,4 @@
		@model TodoListService.Models.Todo
		@model TodoList.Models.Todo

Prototype of new API with the Graph and Downstream web API services #431

Prototype of new API with the Graph and Downstream web API services #431

Uh oh!

Conversation

jmprieur commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposal for Microsoft.Graph support

Proposal for Downstream web API support

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jennyf19 commented Aug 13, 2020

Uh oh!

jmprieur commented Aug 13, 2020

Uh oh!

Uh oh!

jmprieur commented Aug 11, 2020 •

edited

Loading