AB#38712 Check user resource ownership by omersayshi · Pull Request #14 · LokiLabs/microsoft-graph-devx-api

omersayshi · 2021-07-28T20:56:30Z

Overview

Using the user's passed in Graph delegated token, I retrieved the tenantId and user id from the token. The purpose of this PR is to use the token and the user id to make a graph call to check if the resource (teams or chat) that the client is trying to make a request on is owned by the user.

Testing Instructions

NOTE: Alot of overhead is required to test this PR. to make it easier, ping me on Teams for a ClientId and ClientSecret to avoid registering an app and installing a Teams app on a test tenant with the same ClientId.

Open the file appsettings.json
In ClientId, paste the clientId of your app registration
In ClientSecret, paste the client secret of your app registration
Head over to Graph Explorer
Go to Access token and copy the access token
Run the branch locally and open Postman
Add your access token that was copied from GE into the Authorization tab as a 'Bearer Token'
Make a call to the API using the endpoint /graphproxy/{ADD YOUR GRAPH QUERY HERE}. Example: /v1.0/teams/{team-id}/channels
Make sure its a Graph call on a Teams resource (Team or Chat) that you are a owner of and have the GE Teams app (with the same client Id as the you pasted in appsettings.json) installed on (in order for the Graph request to work)

Expected behavior

If you have the teams app installed on the resource you are trying to call AND you are the owner of that resource, you will receive a 200 OK with the response
If you have the teams app installed on the resource you are trying to call AND you are the NOT owner of that resource, you will receive a 403 forbidden with an empty response
If you dont have the teams app installed on the resource you are trying to call, you will receive a 403 forbidden with an error message from Graph.

Merges dev into master

* Add telemetry diagnostic tracing for samples * Add trace info for permissions * Update telemetry trace capture points * Install Application insights for trace telemetry tracking * Adds trace messages to log diagnostic events at service level * Track exceptions including stack traces at controller level * Add Telemetry lib. to OpenAPIService * Add OpenApiService as Singleton * Use static property to set TelemetryClient * Use parameter to set telemetry client in OpenApiService * Adds the ChangesService class as a singleton on startup and refactor code * Modify the ChangesService from a static class to a singleton and add trace telemetry for tracking purposes * Update value of the trace properties dictionary * Initialize a telemetry client and use it to track exceptions * Use a static property to set the telemetry client and use it to track trace telemetry * Install ApplicationInsights * Revert "Adds the ChangesService class as a singleton on startup and refactor code" This reverts commit 36bb74a. * Revert "Modify the ChangesService from a static class to a singleton and add trace telemetry for tracking purposes" This reverts commit 71eeb6b. * Reverts changes for ChangesService class and adds trace telemetry * Edit value for the trace properties dictionary * Remove unnecessary line * Remove whitespaces * Add null check * Track exceptions thrown for better inspection * Update OpenApiService and controller trace information and properties * Message update * Rename the trace properties dictionaries * Further trace telemetry message and property updates * Add log messages to track trace events and exceptions * Install Application Insights * Edit the trace message * Add count properties and prevent them from being sanitized * Minor edit * Create UtilityConstants class that will have constants * Add UtilityService project dependencies * Use UtilityService constants * Add TelemetryClientUtility class and update its implementations Also, some class renames and code simplifications. Remove unnecessary code as well. * Code formatting * Transfer TelemetryClientUtility to a dedicated library; delete unnecessary packages and code * Remove unnecessary code and imports; code simplification * Auto stash before merge of "feature/mk/instrument-changes-service" and "origin/feature/is/code-instrumentation" * Add a reference to the TelemetryClientWrapper * Add a reference for the TelemetryClientWrapper * Use a singleton instance of telemetry client to track telemetry across multiple services and controllers * Add a Changes property key for storing count data to avoid unnecessary numerical sanitization * Clean up file * Add a trace message to trigger an alert when our permission data source is empty * Fix indentation * Remove whitespace * Create a joining service. * Refactor startup file * Inject IServiceProvider and use it to get an IPermissionsStore instance * Use the TelemetryClient available from our DI container to track telemetry * Remove optional param * Add/remove packages * Mock an IServiceProvider object and use it to call a mocked instance of IPermissionsStore * Simplify null check * Remove files * Create interfaces and initialize the services as singletons * Implement the interface * Add OpenApi telemetry property key as a constant * Replace string with changes telemetry property key constant * Add project reference * Code cleanup * Add property to prevent graph uri Version from sanitization * Add abstract methods to interfaces and refactor code * Code cleanup * Rename dictionary value to PermissionsStore and delete the dictionary afterwards * Rename dictionary value * Use nameof() operator to get the class names * Use defensive programming Co-authored-by: Irvine Sunday <irochand@microsoft.com> Co-authored-by: Vincent Biret <vibiret@microsoft.com> Co-authored-by: George <gendungu@microsoft.com>

…tionals Fix: Use null-conditionals when capturing count of IEnumerable types in TrackTrace telemetry

The new metadata with annotations has some annotation discrepancies that needs to be looked into before being accepted into prod.

…urce Fix: Revert metadata source

* Add telemetry diagnostic tracing for samples * Add trace info for permissions * Update telemetry trace capture points * Install Application insights for trace telemetry tracking * Adds trace messages to log diagnostic events at service level * Track exceptions including stack traces at controller level * Add Telemetry lib. to OpenAPIService * Add OpenApiService as Singleton * Use static property to set TelemetryClient * Use parameter to set telemetry client in OpenApiService * Adds the ChangesService class as a singleton on startup and refactor code * Modify the ChangesService from a static class to a singleton and add trace telemetry for tracking purposes * Update value of the trace properties dictionary * Initialize a telemetry client and use it to track exceptions * Use a static property to set the telemetry client and use it to track trace telemetry * Install ApplicationInsights * Revert "Adds the ChangesService class as a singleton on startup and refactor code" This reverts commit 36bb74a. * Revert "Modify the ChangesService from a static class to a singleton and add trace telemetry for tracking purposes" This reverts commit 71eeb6b. * Reverts changes for ChangesService class and adds trace telemetry * Edit value for the trace properties dictionary * Remove unnecessary line * Remove whitespaces * Add null check * Track exceptions thrown for better inspection * Update OpenApiService and controller trace information and properties * Message update * Rename the trace properties dictionaries * Further trace telemetry message and property updates * Add log messages to track trace events and exceptions * Install Application Insights * Edit the trace message * Add count properties and prevent them from being sanitized * Minor edit * Create UtilityConstants class that will have constants * Add UtilityService project dependencies * Use UtilityService constants * Add TelemetryClientUtility class and update its implementations Also, some class renames and code simplifications. Remove unnecessary code as well. * Code formatting * Transfer TelemetryClientUtility to a dedicated library; delete unnecessary packages and code * Remove unnecessary code and imports; code simplification * Auto stash before merge of "feature/mk/instrument-changes-service" and "origin/feature/is/code-instrumentation" * Add a reference to the TelemetryClientWrapper * Add a reference for the TelemetryClientWrapper * Use a singleton instance of telemetry client to track telemetry across multiple services and controllers * Add a Changes property key for storing count data to avoid unnecessary numerical sanitization * Clean up file * Add a trace message to trigger an alert when our permission data source is empty * Fix indentation * Remove whitespace * Create a joining service. * Refactor startup file * Inject IServiceProvider and use it to get an IPermissionsStore instance * Use the TelemetryClient available from our DI container to track telemetry * Remove optional param * Add/remove packages * Mock an IServiceProvider object and use it to call a mocked instance of IPermissionsStore * Simplify null check * Remove files * Create interfaces and initialize the services as singletons * Implement the interface * Add OpenApi telemetry property key as a constant * Replace string with changes telemetry property key constant * Add project reference * Code cleanup * Add property to prevent graph uri Version from sanitization * Add abstract methods to interfaces and refactor code * Code cleanup * Rename dictionary value to PermissionsStore and delete the dictionary afterwards * Rename dictionary value * Use nameof() operator to get the class names * Use defensive programming Co-authored-by: Irvine Sunday <irochand@microsoft.com> Co-authored-by: Vincent Biret <vibiret@microsoft.com> Co-authored-by: George <gendungu@microsoft.com> Co-authored-by: Maggie Kimani <maggiekim42@gmail.com> Co-authored-by: Irvine Sunday <irochand@microsoft.com> Co-authored-by: Vincent Biret <vibiret@microsoft.com> Co-authored-by: George <gendungu@microsoft.com>

Merges dev into master

* Add test for validating setting explode property * Visit Parameters to set Explode property to false * Add parameters to test * Move parameter visitor to separate class to update entire OpenApi doc. * Nit fix on comment * Use instance variable to make calls to the service * Update OpenAPIService/OperationSearch.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Update OpenAPIService/OperationSearch.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Update OpenAPIService/OperationSearch.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Update OpenAPIService/OperationSearch.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Update OpenAPIService.Test/OpenAPIDocumentCreatorMock.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Update OpenAPIService.Test/OpenAPIDocumentCreatorMock.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Update OpenAPIService/OperationSearch.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Auto stash before merge of "fix/is/set-explode-false" and "microsoft-graph-devx-api/fix/is/set-explode-false" * Refactor class; import LINQ library * Update OpenAPIService/OperationSearch.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Add default locale for requests with unspecified Accept-Language Unrelated to the overall work item but is important for servicing /sample requests which don't provide an Accept-Language value in the header. Co-authored-by: Irvine Sunday <irochand@microsoft.com> Co-authored-by: Vincent Biret <vibiret@microsoft.com>

Merges dev into master

* Update how permissions values are fetched * Update path to the new permissions files container * Update test files structure * Update file path * Make linq query more readable * Refactor code Co-authored-by: Irvine Sunday <irochand@microsoft.com> Co-authored-by: Irvine Sunday <40403681+irvinesunday@users.noreply.github.com>

* add issue creation templates * add pull request template

[skip ci]

…re-pipeline Set up CI with Azure Pipelines

Bumps [Microsoft.AspNetCore.Authorization](https://github.com/dotnet/aspnetcore) from 5.0.7 to 5.0.8. - [Release notes](https://github.com/dotnet/aspnetcore/releases) - [Commits](dotnet/aspnetcore@v5.0.7...v5.0.8) --- updated-dependencies: - dependency-name: Microsoft.AspNetCore.Authorization dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

…t.AspNetCore.Authorization-5.0.8 Bump Microsoft.AspNetCore.Authorization from 5.0.7 to 5.0.8

Bumps [Microsoft.AspNetCore.Mvc.NewtonsoftJson](https://github.com/dotnet/aspnetcore) from 5.0.7 to 5.0.8. - [Release notes](https://github.com/dotnet/aspnetcore/releases) - [Commits](dotnet/aspnetcore@v5.0.7...v5.0.8) --- updated-dependencies: - dependency-name: Microsoft.AspNetCore.Mvc.NewtonsoftJson dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

…t.AspNetCore.Mvc.NewtonsoftJson-5.0.8 Bump Microsoft.AspNetCore.Mvc.NewtonsoftJson from 5.0.7 to 5.0.8

Bumps [Microsoft.AspNetCore.Authentication.JwtBearer](https://github.com/dotnet/aspnetcore) from 5.0.7 to 5.0.8. - [Release notes](https://github.com/dotnet/aspnetcore/releases) - [Commits](dotnet/aspnetcore@v5.0.7...v5.0.8) --- updated-dependencies: - dependency-name: Microsoft.AspNetCore.Authentication.JwtBearer dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

…t.AspNetCore.Authentication.JwtBearer-5.0.8 Bump Microsoft.AspNetCore.Authentication.JwtBearer from 5.0.7 to 5.0.8

* Refactor permissions fetching and update fetch-all permissions algo. * Add null checks; reorder params * Spacing * Variable renaming; add XML comment * Update permission tests Capture multiple scopes for a given endpoint/method to test whether distinct scopes gets returned * Add null checks; get distinct scopes; code refactoring * Update telemetry message * Update GraphExplorerPermissionsService/Services/PermissionsStore.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * PR review suggestions fixes * Update GraphExplorerPermissionsService/Services/PermissionsStore.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * Update GraphExplorerPermissionsService/Services/PermissionsStore.cs Co-authored-by: Vincent Biret <vibiret@microsoft.com> * PR review suggestion fixes * Further PR review suggestion fixes - LINQ Select Co-authored-by: Irvine Sunday <irochand@microsoft.com> Co-authored-by: Vincent Biret <vibiret@microsoft.com>

Bumps [Microsoft.Extensions.DependencyInjection](https://github.com/dotnet/runtime) from 5.0.1 to 5.0.2. - [Release notes](https://github.com/dotnet/runtime/releases) - [Commits](dotnet/runtime@v5.0.1...v5.0.2) --- updated-dependencies: - dependency-name: Microsoft.Extensions.DependencyInjection dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

…t.Extensions.DependencyInjection-5.0.2 Bump Microsoft.Extensions.DependencyInjection from 5.0.1 to 5.0.2

Added teamsAppSampleQueries in model and extended ordering functionality

…ple-queries

…e-sample-queries AB#39171 Added teamsAppSampleQueries in model and extended ordering functionality

gracet9182

This is what I tested:
-I ran a query with a teams channel that I created which didn't have any tabs added and it gave me a 403 error. This is as expected since it didn't have permissions.

-Then, I ran a GET query in that same teams channel with my app added. It gave me a response; awesome, it checks for owner

-Then, I changed myself from an owner to a member and got a 403 error; yay! That's what we want ╰(°▽°)╯

-Then, I created a chat with 2 people without adding the app and ran a query. It gave a 403 error as expected.

-Then, I added the app to the chat with 2 people and ran the query but still gave me a 403 error. I attached a screenshot below. I ran the query in GE with delegated permissions just to make sure the query was valid after. From the error message I thought it was an issue on my side but when I ran the same query on my deployed endpoint that doesn't check for ownership, the api call went through. I also think the error message might be misleading. I noticed in the first scenario that I tested, there was no error message but just a 403. You could do the same with this one or maybe craft a better error message but not sure how feasible it is with the amount of time we have left.

-I tried the same thing again but it was a chat with a single person. It gave a 403 error. Are they considered owners if it's a OneOnOne chat?

It would be really great to see expected behavior so we know what has been considered

omersayshi · 2021-07-30T01:13:11Z

@gracet9182 That is also expected behaviour, From the screenshot the call was done using Graph v1.0 and currently RSC permissions for chats (correct me if I am wrong) are mostly in beta. So this 403 error you are receiving is directly from Graph.
Also my apologies for not including expected behaviour in the PR details, will add them.

gracet9182 · 2021-07-30T17:14:07Z

Talked to Omer about a token error. The solution was to input a random guid into appsettings for tenantId - might be something that can be fixed as a bug later but this seems to work! ╰(°▽°)╯amazing work

…yshi/38712-check-ownership

PR cleanup

csyan5 · 2021-08-04T16:46:05Z

+3. Scroll down to '....'
+4. See error
+
+**Expected behavior**


you can also add a section called actual behavior to understand the bug more easily and clearly

These changes are from an upstream merge and not mine, so I cant change it.

csyan5 · 2021-08-04T17:33:30Z

+        /// <param name="scopes">The target list of scopes.</param>
+        /// <param name="scopeType">The type of scope from which to retrieve the scopes information for.</param>
+        /// <returns>A list of <see cref="ScopeInformation"/>.</returns>
+        private static List<ScopeInformation> GetScopesInformation(IDictionary<string, IDictionary<string, ScopeInformation>> scopesInformationDictionary,


if the inputs will not be modified, make them as read only such as IReadOnlyDictionary and IReadOnlyList

These changes are from an upstream merge and not mine, so I cant change it.

MaggieKimani1 and others added 30 commits June 9, 2021 14:54

Merge pull request #598 from microsoftgraph/dev

b8845d6

Merges dev into master

Merge pull request #606 from microsoftgraph/dev

dae70db

Merges dev into master

Use null-conditionals when capturing count of enumerables

ab21557

Subtle change to trigger build

8d8da95

Merge pull request #623 from microsoftgraph/fix/is/include-null-condi…

7fe617a

…tionals Fix: Use null-conditionals when capturing count of IEnumerable types in TrackTrace telemetry

Revert metadata source

39f44e7

The new metadata with annotations has some annotation discrepancies that needs to be looked into before being accepted into prod.

Merge pull request #624 from microsoftgraph/fix/is/revert-metadata-so…

3828ef3

…urce Fix: Revert metadata source

Merge branch 'master' into dev

c17c1c7

Merge pull request #625 from microsoftgraph/dev

359b96e

Merges dev into master

Merge branch 'master' into dev

f28bb50

Merge pull request #627 from microsoftgraph/dev

86727cf

Merges dev into master

Merge branch 'master' into dev

a8eed5b

Task: add GitHub templates (#634)

ed4b4c1

* add issue creation templates * add pull request template

Added .yml file for CI/CD pipeline on interns' fork

c0317db

Set up CI with Azure Pipelines

ac2f1ee

[skip ci]

Merge pull request #1 from LokiLabs/interns/jerryxihe/38984-added-azu…

512e5d9

…re-pipeline Set up CI with Azure Pipelines

Merge pull request #639 from microsoftgraph/dependabot/nuget/Microsof…

0003a64

…t.AspNetCore.Authorization-5.0.8 Bump Microsoft.AspNetCore.Authorization from 5.0.7 to 5.0.8

Merge pull request #640 from microsoftgraph/dependabot/nuget/Microsof…

0d3174a

…t.AspNetCore.Mvc.NewtonsoftJson-5.0.8 Bump Microsoft.AspNetCore.Mvc.NewtonsoftJson from 5.0.7 to 5.0.8

Revert "Set up CI with Azure Pipelines"

b245bf7

Merge pull request #642 from microsoftgraph/dependabot/nuget/Microsof…

fb6c6b4

…t.AspNetCore.Authentication.JwtBearer-5.0.8 Bump Microsoft.AspNetCore.Authentication.JwtBearer from 5.0.7 to 5.0.8

Merge pull request #641 from microsoftgraph/dependabot/nuget/Microsof…

78ec6b8

…t.Extensions.DependencyInjection-5.0.2 Bump Microsoft.Extensions.DependencyInjection from 5.0.1 to 5.0.2

jerryxihe and others added 9 commits July 15, 2021 12:08

Added master and dev to interns pipeline triggers

151fc60

Merge branch 'dev' into interns/dev

4ac9ef9

5f0f758

Added teamsAppSampleQueries in model and extended ordering functionality

Fixed small error

d60609a

Merge branch 'interns/dev' into interns/omersayshi/39171-app-mode-sam…

e99db79

…ple-queries

Fixed another small typo

44611b2

Merge pull request #13 from LokiLabs/interns/omersayshi/39171-app-mod…

9474f45

…e-sample-queries AB#39171 Added teamsAppSampleQueries in model and extended ordering functionality

Added ownership check functions

a924fc0

Finalized tenant and client id retrieval from token

79c0986

omersayshi requested review from acchiang, ayubun, csyan5, gracet9182, jerryxihe, lhao03 and v-mschramm July 28, 2021 20:56

omersayshi self-assigned this Jul 28, 2021

Cleanup

9d70e8f

csyan5 reviewed Jul 29, 2021

View reviewed changes

Comment thread GraphExplorerAppModeService/Services/GraphService.cs Outdated

Comment thread GraphExplorerAppModeService/Services/GraphService.cs

Comment thread GraphExplorerAppModeService/Services/GraphService.cs

gracet9182 reviewed Jul 29, 2021

View reviewed changes

gracet9182 approved these changes Jul 30, 2021

View reviewed changes

Comment thread GraphWebApi/Controllers/GraphExplorerAppModeController.cs Outdated

omersayshi added 2 commits August 3, 2021 14:47

Merge remote-tracking branch 'origin/interns/dev' into interns/omersa…

da84f62

…yshi/38712-check-ownership

0e43543

PR cleanup

csyan5 reviewed Aug 4, 2021

View reviewed changes

v-mschramm approved these changes Aug 4, 2021

View reviewed changes

Added PR changes and fixed error to return JSON

294cdbd

csyan5 approved these changes Aug 4, 2021

View reviewed changes

omersayshi merged commit 9a47865 into interns/feature/ge-app-mode-proxy-endpoint Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

AB#38712 Check user resource ownership - #14

AB#38712 Check user resource ownership #14
omersayshi merged 50 commits into
interns/feature/ge-app-mode-proxy-endpointfrom
interns/omersayshi/38712-check-ownership

omersayshi commented Jul 28, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gracet9182 left a comment

Uh oh!

omersayshi commented Jul 30, 2021 •

edited

Loading

Uh oh!

gracet9182 commented Jul 30, 2021 •

edited

Loading

Uh oh!

Uh oh!

csyan5 Aug 4, 2021

Uh oh!

omersayshi Aug 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

csyan5 Aug 4, 2021

Uh oh!

omersayshi Aug 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Uh oh!

Conversation

omersayshi commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Testing Instructions

Expected behavior

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gracet9182 left a comment

Choose a reason for hiding this comment

Uh oh!

omersayshi commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gracet9182 commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

csyan5 Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

omersayshi Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

csyan5 Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

omersayshi Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

omersayshi commented Jul 28, 2021 •

edited

Loading

omersayshi commented Jul 30, 2021 •

edited

Loading

gracet9182 commented Jul 30, 2021 •

edited

Loading