Conversation
new provoder for Daemon type apps.
cdmayer
left a comment
There was a problem hiding this comment.
Please address all requested changes.
| clientKey = applicationKey; | ||
|
|
||
| string authority = String.Format(CultureInfo.InvariantCulture, "https://login.microsoftonline.com/{0}", tenant); | ||
| authContext = new AuthenticationContext(authority); |
There was a problem hiding this comment.
authContext = new AuthenticationContext(authority);
Use AuthenticationContextWrapper: https://github.com/OneDrive/onedrive-sdk-dotnet-msa-auth-adapter/blob/master/src/OneDrive.Sdk.Authentication.Desktop/Business/AuthenticationContextWrapper.cs
| { | ||
| retry = true; | ||
| retryCount++; | ||
| Thread.Sleep(3000); |
| public AdalDaemonAuthenticationProvider( | ||
| string applicationId, | ||
| string applicationKey, | ||
| string tenant) |
There was a problem hiding this comment.
string applicationId,
string applicationKey,
Please use clientId and clientSecret like the rest of the library. Change throughout this file.
| ClientCredential clientCredential; | ||
|
|
||
| // 'applicationId' : Your Application ID | ||
| // 'applicationKey' : Your Application Key |
There was a problem hiding this comment.
Please use typical class comments like this example in AuthenticationContextWrapper
/// <summary>
/// Authenticates the user silently using <see cref="AuthenticationContext.AcquireTokenSilentAsync(string, string, UserIdentifier)"/>.
/// </summary>
/// <param name="resource">The resource to authenticate against.</param>
/// <param name="clientId">The client ID of the application.</param>
/// <param name="userIdentifier">The <see cref="UserIdentifier"/> of the user.</param>| try | ||
| { | ||
| // ADAL includes an in memory cache, so this call will only send a message to the server if the cached token is expired. | ||
| result = await authContext.AcquireTokenAsync(serviceResourceId, clientCredential); |
There was a problem hiding this comment.
Again, AuthenticationContextWrapper
|
|
||
| var uri = new UriBuilder(request.RequestUri); | ||
| if (string.IsNullOrEmpty(uri.Query)) | ||
| uri.Query = string.Format("client_secret={0}", clientKey); |
There was a problem hiding this comment.
Add braces around if and else statement blocks
| this.CurrentAccountSession.AccessToken); | ||
| } | ||
|
|
||
| protected AccountSession ConvertAuthenticationResultToAccountSession(AuthenticationResult authenticationResult) |
There was a problem hiding this comment.
Use the existing implementation in AdalAuthenticationProviderBase
|
|
||
| namespace Microsoft.OneDrive.Sdk.Authentication.Business | ||
| { | ||
| public class AdalDaemonAuthenticationProvider : IAuthenticationProvider |
There was a problem hiding this comment.
Is there a reason this doesn't extend AdalAuthenticationProviderBase?
| Thread.Sleep(3000); | ||
| } | ||
|
|
||
| Console.WriteLine( |
There was a problem hiding this comment.
Shouldn't be writing to the console in the library. Remove this. If you want to log this stuff, you can log it at the client layer.
| @@ -0,0 +1,134 @@ | |||
| using System; | |||
There was a problem hiding this comment.
Add the license
// ------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All Rights Reserved. Licensed under the MIT License. See License in the project root for license information.
// ------------------------------------------------------------------------------
|
Hi, Sorry I was busy with my Job. I tried to apply your suggestions, but the main problem is that if I use ' Is it because of the OneDrive SDK I installed from NugetPackage? Best regards., On Tue, Oct 25, 2016 at 8:19 PM, Chris Mayer notifications@github.com
Humble S/W developer, Always. |
|
Are you saying that calling AuthenticationContext.AcquireTokenSilentAsync works when you call it directly, but not when you call it with the context wrapper? |
|
Oh, I was assuming that 'AcquireTokenSilentAsync()' will do the job. Task AcquireDaemonTokenSilentAsync(string resource, Will send you another pull request. Thanks. On Wed, Oct 26, 2016 at 6:07 PM, Chris Mayer notifications@github.com
Humble S/W developer, Always. |
- use clientId and clientSecret like the rest of the library - use typical class comments like this example in AuthenticationContextWrapper - Use constant, rather than hard wired value - Frefresh token, even if it never happens - remove extra white spaces - Add braces around if and else statement blocks - Use the existing implementation in AdalAuthenticationProviderBase - Modify base class to ;AdalAuthenticationProviderBase' - Remove console log
| _clientKey = clientSecret; | ||
|
|
||
| string authority = String.Format(CultureInfo.InvariantCulture, "https://login.microsoftonline.com/{0}", tenant); | ||
| authContextWrapper = authenticationContextWrapper; |
|
|
||
| string authority = String.Format(CultureInfo.InvariantCulture, "https://login.microsoftonline.com/{0}", tenant); | ||
| authContextWrapper = authenticationContextWrapper; | ||
| clientCredential = new ClientCredential(_clientId, _clientKey); |
| string _clientKey; | ||
|
|
||
| public IAuthenticationContextWrapper authContextWrapper; | ||
| ClientCredential clientCredential; |
There was a problem hiding this comment.
add access modifier, even if it doesn't need to be explicit.
| /// <param name="resource">The resource to authenticate against.</param> | ||
| /// <param name="clientCredential">The client credential of the application.</param> | ||
| /// <returns>The <see cref="IAuthenticationResult"/>.</returns> | ||
| public async Task<IAuthenticationResult> AcquireDaemonTokenSilentAsync(string resource, ClientCredential clientCredential) |
There was a problem hiding this comment.
Why add this when you can use AcquireTokenSilentAsync(string resource, ClientCredential clientCredential, UserIdentifier userIdentifier) with UserIdentifier.Any? It should be the same unless I'm missing something.
There was a problem hiding this comment.
That's what I asked you why AcquireTokenSilentAsync doesn't work. It throws exception.
Daemon process don't require end user ID anyway.
There was a problem hiding this comment.
Are you saying AcquireTokenSilentAsync throws an exception, but AcquireTokenAsync does not? Wouldn't it pop up a dialog? I thought the reason those methods were separate was because the first did not require user interaction.
Either way, I think adding this is fine, but perhaps the name should be different (since nothing about the token is inherently limited to daemons). Can you change the method name to AcquireTokenAsync?
| /// <param name="resource">The resource to authenticate against.</param> | ||
| /// <param name="clientCredential">The client credential of the application.</param> | ||
| /// <returns>The <see cref="IAuthenticationResult"/>.</returns> | ||
| public async Task<IAuthenticationResult> AcquireDaemonTokenSilentAsync(string resource, ClientCredential clientCredential) |
There was a problem hiding this comment.
Are you saying AcquireTokenSilentAsync throws an exception, but AcquireTokenAsync does not? Wouldn't it pop up a dialog? I thought the reason those methods were separate was because the first did not require user interaction.
Either way, I think adding this is fine, but perhaps the name should be different (since nothing about the token is inherently limited to daemons). Can you change the method name to AcquireTokenAsync?
| authContextWrapper = authenticationContextWrapper; | ||
| clientCredential = new ClientCredential(_clientId, _clientKey); | ||
| this.authContextWrapper = authenticationContextWrapper; | ||
| this.clientCredential = new ClientCredential(_clientId, _clientKey); |
There was a problem hiding this comment.
this.clientCredential = new ClientCredential(this._clientId, this._clientKey);
Please look for this everywhere in the file. Instance members and methods should always have this.
| /// <param name="clientCredential">The client credential of the application.</param> | ||
| /// <returns>The <see cref="IAuthenticationResult"/>.</returns> | ||
| public async Task<IAuthenticationResult> AcquireDaemonTokenSilentAsync(string resource, ClientCredential clientCredential) | ||
| public async Task<IAuthenticationResult> AcquireTokenSilentAsync(string resource, ClientCredential clientCredential) |
There was a problem hiding this comment.
Yikes sorry, this should actually be AcquireTokenAsync. It should match the call to AuthenticationContext to make it a little easier for the client to figure out what's happening under the covers. Sorry if I gave you the wrong name before :(
new provoder for Daemon type apps.