Update to MSAL 2.7 and use of Microsoft.Graph SDK by andrueastman · Pull Request #19 · microsoftgraph/msgraph-sample-deltaquery

andrueastman · 2019-02-07T15:30:54Z

Essentially, this PR makes the following updates: -

the MSAL dependencies are update to v2.7 from 1.1.4.
Updage Graph delta query calls to used the Graph SDKs(v1.1.12) rather than use Webclient calls Update to use Graph SDK #18
Helpers classes added to project to be similar to other console examples here
Fix for repeated Auth Prompts when running the example Repeated auth prompts #16
Minor improvements to use async rather than blocking calls and reading all configs at app start with Readme clarifications

…idelines specified in https://docs.microsoft.com/en-us/dotnet/csharp/async :)

- This checks deltas in mailFolders rather than drive - Updated Readme for clarifications - Removed Webclient dependencied to use the Graph API -

…untime - also abstracted away some hard coded values to config files.

msftclas · 2019-02-07T15:31:06Z

All CLA requirements met.

jthake · 2019-02-07T16:17:15Z

@jmprieur could you review this from an MSAL dotnet perspective. This is the first one we're updating from old MSAL and addig that helper classes to allow us to update to next version easily next time.

…rompts

jmprieur

I've left a few suggestions.
@andrueastman, would you mind giving me access to your repo (I'm assuming this is a private repo, as I was not able to see it):

I'd like to see the code in Visual Studio
I could propose a PR on top of yours

Regards
Jean-Marc

jmprieur · 2019-02-08T14:56:13Z

+
+        public MsalAuthenticationProvider(PublicClientApplication clientApplication, string[] scopes) {
+            _clientApplication = clientApplication;
+            _scopes = scopes;


It's weird that you pass the scopes in the MsalAuthenticationProvider? what will happen if your app wants to do a call to another Graph API (and therefore this requires more scopes) ?

The idea would be to create a new GraphServiceClient and in effect MsalAuthenticationProvider objects with the right scopes and therefore make the calls to the other Graph API.

@jmprieur In our actual AuthenticationProviders we allow scopes to be passed to the AuthenticationProvider via a "Request context" object. In .Net the request context is stored in the HttpRequestMessage.Properties dictionary and flows down the middleware pipe and is extracted by the AuthorizationProvider.

This will allow us to do incremental consent. There will be a fluent helper method on our GraphServiceClient to make it easy to pass scopes explicitly via the request and when we finally get permissions in the metadata we will embed these per request scopes into our code generated request builder.

jmprieur · 2019-02-08T14:58:59Z

+        /// </summary>
+        public async Task<string> GetTokenAsync()
+        {
+            if (_accessToken == null)


Please never keep the access token in a variable
the right patter is to do:

try { authResult = _clientApplication.AcquireTokenSilentAsync(…) } catch(MsalUiRequiredException) { authResult = _clientApplication.AcquireTokenAsync() }

The reason is if you keep the token and there is a need for incremental consent or conditional access (for example multiple factor auth), your GetTokenAsync will always fail.

This makes sense to me. Will action and resolve.

@jmprieur Our goal here is not to try and support scenarios such as incremental consent and conditional access, but simply to provider a placeholder for the real AuthorizationProviders when they are ready. However, I agree that holding onto the token is not the best way of doing even this simple scenario.

Out of curiosity, is there an advantage to doing the try/catch over doing something like,

if (_account == null) { authResult = _clientApplication.AquireTokenSilentAsync(_account); } else { authResult = _clientApplication.AquireTokenAsync(); _account = authResult.Account }

@darrelmiller : there are many reasons why AcquireTokenSilentAsync would throw: that is if interaction is required from the user (password expired, conditional access needed, advanced security decides the user needs to re-sign-in etc …)

jmprieur · 2019-02-08T15:00:25Z

-1. In Solution Explorer, select the **App.config** project.
+1. In Solution Explorer, select and edit the **App.config** in the ConsoleApplication project.

 	a. For the `AppPrincipalId` key, replace `To be filled in` with the application ID of your registered Azure application.


would that be the clientID?
Don't we want to be consistent with the authentication samples?

Is the suggestion to use the word clientID rather than AppPrincipalId ??

Yes :-) This sample seems really old and a little confused. Sorry, I didn't realize we were throwing such a convoluted sample at you as your first one.

We might want to have a look at the https://github.com/Azure-Samples/active-directory-coding-exercises which was done for the London event in Oct. @dkershaw10 : do you agree?

jmprieur · 2019-02-08T15:02:57Z

+            if (_accessToken == null)
+            {
+                AuthenticationResult authResult = null;
+                authResult = await _clientApplication.AcquireTokenAsync(_scopes);


Also this can throw. we need to handle the exception;

My proposal: let's work together on a class that you could reuse in all the samples.

Also as you suggested, I'm happy to work together on this to make this class reusable.

@jmprieur If we build an AuthorizationProvider class that works for all the samples, at what point are we reimplementing the classes we are doing in https://github.com/microsoftgraph/msgraph-sdk-dotnet-auth ? Perhaps we should focus on getting those completed faster, so the samples can take advantage of them?

I agree @darrelmiller

andrueastman · 2019-02-08T17:06:58Z

@jmprieur I have added you as collaborator but the repo is not private and can be found here.

Thanks for the comments. Working on resolving them.

jmprieur · 2019-02-08T17:08:33Z

thanks @andrueastman
For some reason, I had not found it :)

darrelmiller · 2019-02-08T20:11:22Z

-            // - retry mechanism to be implemented
-            //
-            operation();
+            PublicClientApplication clientApplication = new PublicClientApplication(this.appPrincipalId, authority);


We should rename appPrincipalId to clientId. And is appPrinicpalPassword used? This sample seems to use Interactive flow, not ClientCredentials flow.

darrelmiller · 2019-02-08T20:23:40Z

        /// <returns>Result from the Delta Query service.</returns>
-        public DeltaQueryResult DeltaQuery(
-            string entitySet,
+        public async Task<DeltaQueryResult> DeltaQueryAsync(


We can talk about this on Monday. The original sample is actually trying to be more than just a sample and it is trying to create a generic client that can query any arbitrary entityset for deltaquery results. That type of functionality belongs in our core SDK, not in a sample. So, I agree with what you are doing here by using graphServiceClient to demo doing a DeltaQuery on a single entityset. However, to reduce the confusion, I think we should lift this code out of the Client class into the program class.

agreed, this sample is a little too confusing.

jthake

We will talk about this on Monday. But the readme instructions to get this working need updating and likely require some screen shots. So that people know what to do in portal.azure.com with Azure Active Directory to register the application.

- renamed AppPrincipalId to ClientId - fixed issue of storing the token in the AuthProvider to properly acquire it using the AcquireTokenSilentAsync function

- handled exceptions - updated config file to rename AppPrincipalId to clientID - changed Auth provider to handle local cache appropiately.

- Also made updated based on PR feedback.

jasonjoh · 2019-02-11T15:54:51Z

-    <add key="TenantDomainName" value="To be filled in"/>
     <!--Service principal credentials-->  
-    <add key="AppPrincipalId" value="edd4904c-ac06-4f13-b8b5-9ec799e82bd3"/>
+    <add key="AppPrincipalId" value="To be filled in"/>


Move this to a separate .config file that is not checked in to the repo (added to .gitignore). This is to prevent inadvertently leaking your app ID.

Look at https://github.com/microsoftgraph/msgraph-training-aspnetmvcapp/tree/master/Demos/03-add-msgraph/graph-tutorial. In that sample, a separate PrivateSettings.config is used to hold OAuth settings, and is merged into Web.config by using this line:

<appSettings file="PrivateSettings.config">

Then, a copy of PrivateSettings.config, named PrivateSettings.config.example is added to the repo with placeholders, so devs can just rename the file and plug in their own app ID/secret.

Thanks for this @jasonjoh,
I have made updates to this where the Readme has been updated to instruct the user to first rename the appsettings.json.example file and the appsettings.json been added to the .gitignore to prevent leaking of ones appID.

jmprieur

a few more comments

jmprieur · 2019-02-12T08:44:13Z

+        /// </summary>
+        public async Task<string> GetTokenAsync()
+        {
+            if (_accessToken == null)


@darrelmiller : there are many reasons why AcquireTokenSilentAsync would throw: that is if interaction is required from the user (password expired, conditional access needed, advanced security decides the user needs to re-sign-in etc …)

- used GraphServiceFactory - eliminated the client project as we are making graph calls directly now. - moved stuff around to factor in changes.

jmprieur

LGTM.
I've made a few suggestions for improvements.
You probably want to fix the README.md, and discuss which headers you want to have.

jmprieur · 2019-02-12T13:40:25Z

@@ -0,0 +1,230 @@
+// <copyright file="DeltaQuery.cs" company="Microsoft">


You probably want to have the same copyright header on all files?
probably the MIT one? @jthake-msft ?

andrueastman · 2019-02-13T08:00:07Z

Hey guys,

I have made some updates to the PR. Please check them out when you can :).

Thanks,
Andrew.

@jthake-msft @jmprieur @jasonjoh @darrelmiller

jmprieur · 2019-02-13T15:48:00Z

LGTM
Thanks for addressing the PR comments, @andrueastman !

Andrew Omondi added 4 commits February 6, 2019 12:44

Updated source to use MSAL 2.7 and renamed async functions to meet gu…

861c74d

…idelines specified in https://docs.microsoft.com/en-us/dotnet/csharp/async :)

Working version of the console application.

b39c9a2

- This checks deltas in mailFolders rather than drive - Updated Readme for clarifications - Removed Webclient dependencied to use the Graph API -

Cleanup to read configs at entry point and pass in as object during r…

f302986

…untime - also abstracted away some hard coded values to config files.

Cleanup changes to remove blocking calls and use await and tasks

cafbc22

jthake requested review from jasonjoh and jmprieur February 7, 2019 16:16

jthake requested review from darrelmiller and jthake February 7, 2019 16:17

Andrew Omondi added 2 commits February 8, 2019 13:53

Fix for handling of deletion changes being perceived as updates.

e16a318

Update helper to keep local cache of toke to prevent multiple login p…

be30e3a

…rompts

jmprieur suggested changes Feb 8, 2019

View reviewed changes

andrueastman closed this Feb 8, 2019

andrueastman reopened this Feb 8, 2019

Update to use IEnumerable rather than string array

1b0f8e3

darrelmiller reviewed Feb 8, 2019

View reviewed changes

jthake suggested changes Feb 8, 2019

View reviewed changes

Andrew Omondi added 3 commits February 11, 2019 11:04

Some changes from PR feedback.

ce35112

- renamed AppPrincipalId to ClientId - fixed issue of storing the token in the AuthProvider to properly acquire it using the AcquireTokenSilentAsync function

Review updates

518634d

- handled exceptions - updated config file to rename AppPrincipalId to clientID - changed Auth provider to handle local cache appropiately.

Updated project to use standard configuration reads.

462f0a8

- Also made updated based on PR feedback.

jasonjoh requested changes Feb 11, 2019

View reviewed changes

jmprieur reviewed Feb 12, 2019

View reviewed changes

Andrew Omondi added 3 commits February 12, 2019 13:31

Project refactoring.

625f909

- used GraphServiceFactory - eliminated the client project as we are making graph calls directly now. - moved stuff around to factor in changes.

Readme fixes and updates.

a3ecc27

Merge branch 'pr_feedback_changes' into use-graph-sdk-and-update-msal

be1fb06

jmprieur reviewed Feb 12, 2019

View reviewed changes

andrueastman and others added 4 commits February 12, 2019 23:04

Consistent MIT headers in source and minor tweaks to Readme

b6df5a5

Updates to used the default scopes in MS graph calls

eea56ad

Readme updates to clarify the setting up of the application.

5588a87

Merge branch 'pr_feedbac_changes' into use-graph-sdk-and-update-msal

f289438

andrueastman closed this Feb 13, 2019

andrueastman reopened this Feb 13, 2019

darrelmiller and others added 5 commits February 24, 2019 15:56

Removed the generic DeltaQuery notion

2f5572d

Forgot to same csproj

3a6ac27

Merge branch 'dm/delta-suggest' into use-graph-sdk-and-update-msal

364ede1

XML comment additions to source

5d2999a

Added app creation scripts to the sample and updates readme.

25b5039

darrelmiller approved these changes Feb 26, 2019

View reviewed changes

jasonjoh approved these changes Mar 15, 2019

View reviewed changes

darrelmiller merged commit bafa885 into microsoftgraph:master Mar 15, 2019

		@@ -0,0 +1,230 @@
		// <copyright file="DeltaQuery.cs" company="Microsoft">

Conversation

andrueastman commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jthake commented Feb 7, 2019

Uh oh!

jmprieur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

andrueastman commented Feb 8, 2019

Uh oh!

jmprieur commented Feb 8, 2019

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!

jthake left a comment

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!

jmprieur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmprieur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrueastman commented Feb 7, 2019 •

edited

Loading

msftclas commented Feb 7, 2019 •

edited

Loading

darrelmiller Feb 8, 2019 •

edited

Loading