Skip to content

AB#38606 graph requests#12

Merged
gracet9182 merged 10 commits into
interns/feature/ge-app-mode-proxy-endpointfrom
interns/gracet9182/38716-graph-requests
Jul 22, 2021
Merged

AB#38606 graph requests#12
gracet9182 merged 10 commits into
interns/feature/ge-app-mode-proxy-endpointfrom
interns/gracet9182/38716-graph-requests

Conversation

@gracet9182

@gracet9182 gracet9182 commented Jul 20, 2021

Copy link
Copy Markdown
Member

Overview

Demo

Postman.2021-07-20.11-09-07.mp4

Testing Instructions

  • Pull this branch
  • Fill in the TenantId, ClientId, ClientSecret and AzureConnectionString inside appsettings.json (Note that you need to have the necessary rsc permissions in the app to run the queries)
  • Open in Visual Studio
  • Run in IIS Express
  • The endpoint is https://localhost:44399/graphproxy/{tenantId}/{graph api call suffix}
  • What you add to your api call is what is passed to https://graph.microsoft.com/. Eg. GET https://localhost:44399/graphproxy/{tenantId}/v1.0/ will be calling GET `https://graph.microsoft.com/v1.0/ .

@gracet9182 gracet9182 self-assigned this Jul 20, 2021
@gracet9182 gracet9182 changed the title Interns/gracet9182/38716 graph requests AB#38606 graph requests Jul 20, 2021
@gracet9182
gracet9182 marked this pull request as draft July 20, 2021 18:49
@gracet9182
gracet9182 marked this pull request as ready for review July 21, 2021 00:44
@gracet9182
gracet9182 changed the base branch from interns/gracet9182/38715-output-app-token to interns/feature/ge-app-mode-proxy-endpoint July 21, 2021 17:21

[Route("api/[controller]/{*all}")]
[Route("graphproxy/{*all}")]
[Route("graphproxy/{tenantId}/{*all}")]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason for requiring tenantId to be in the query of every request?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was to have the users be able to change tenantId's as they wanted. I was following the format of other api calls by Microsoft

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a good idea and I agree that this might be an approach we might wanna take to support the teams app to be making the calls, but since we planned to be taking the tenantId from the user token that gets passed from authorization with Darrell, lets stick with that for now

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!! Didn't know that, thanks for letting me know :) 3794493

@gracet9182
gracet9182 requested a review from omersayshi July 22, 2021 16:51
[HttpPut]
public async Task<IActionResult> PutAsync(string all, [FromBody] object body, [FromHeader] string Authorization)
[AuthorizeForScopes(Scopes = new[] { "https://graph.microsoft.com/.default" })]
public async Task<IActionResult> PutAsync(string all, [FromBody] object body, string tenantId)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotta remove tenantId from the parameter of every function

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omersayshi omersayshi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides removing the tenantId from the functions and resolving conflicts, looks good! again great work!

@gracet9182
gracet9182 merged commit 8b156ad into interns/feature/ge-app-mode-proxy-endpoint Jul 22, 2021
@gracet9182
gracet9182 deleted the interns/gracet9182/38716-graph-requests branch August 12, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants