-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new provoder for Daemon type apps. #25
base: master
Are you sure you want to change the base?
Conversation
new provoder for Daemon type apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await Task.Delay(3000);
public AdalDaemonAuthenticationProvider( | ||
string applicationId, | ||
string applicationKey, | ||
string tenant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add braces around if
and else
statement blocks
this.CurrentAccountSession.AccessToken); | ||
} | ||
|
||
protected AccountSession ConvertAuthenticationResultToAccountSession(AuthenticationResult authenticationResult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the existing implementation in AdalAuthenticationProviderBase
|
||
namespace Microsoft.OneDrive.Sdk.Authentication.Business | ||
{ | ||
public class AdalDaemonAuthenticationProvider : IAuthenticationProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this doesn't extend AdalAuthenticationProviderBase
?
Thread.Sleep(3000); | ||
} | ||
|
||
Console.WriteLine( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 [email protected]
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 [email protected]
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.authContextWrapper
|
||
string authority = String.Format(CultureInfo.InvariantCulture, "https://login.microsoftonline.com/{0}", tenant); | ||
authContextWrapper = authenticationContextWrapper; | ||
clientCredential = new ClientCredential(_clientId, _clientKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.clientCredential
string _clientKey; | ||
|
||
public IAuthenticationContextWrapper authContextWrapper; | ||
ClientCredential clientCredential; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
/// <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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.clientCredential = new ClientCredential(this._clientId, this._clientKey);
Please look for this everywhere in the file. Instance members and methods should always have this.
@@ -61,7 +61,7 @@ public ITokenCache TokenCache | |||
/// <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) | |||
public async Task<IAuthenticationResult> AcquireTokenSilentAsync(string resource, ClientCredential clientCredential) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.