Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lockelost
Copy link

new provoder for Daemon type apps.

Copy link
Contributor

@cdmayer cdmayer left a 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

{
retry = true;
retryCount++;
Thread.Sleep(3000);
Copy link
Contributor

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)
Copy link
Contributor

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
Copy link
Contributor

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);
Copy link
Contributor

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);
Copy link
Contributor

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)
Copy link
Contributor

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
Copy link
Contributor

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(
Copy link
Contributor

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;
Copy link
Contributor

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.
// ------------------------------------------------------------------------------

@lockelost
Copy link
Author

Hi,

Sorry I was busy with my Job.
Just quick question.

I tried to apply your suggestions, but the main problem is that if I use '
AuthenticationContextWrapper' and call 'AcquireTokenSilentAsync()' to work
as a Daemon, (with no user interaction) it throws an exception it failed,
to use ''AcquireTokenAsync()' instead.

Is it because of the OneDrive SDK I installed from NugetPackage?

Best regards.,
Sunghyun wang

On Tue, Oct 25, 2016 at 8:19 PM, Chris Mayer [email protected]
wrote:

@cdmayer requested changes on this pull request.

Please address all requested changes.

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •    public AuthenticationContext authContext;
    
  •    ClientCredential clientCredential;
    
  •    // 'applicationId' : Your Application ID
    
  •    // 'applicationKey' : Your Application Key
    
  •    // 'tenant' : is usually a domain name for your Office365 service. Like 'yourcompany.onmicrosoft.com'
    
  •    public AdalDaemonAuthenticationProvider(
    
  •        string applicationId,
    
  •        string applicationKey,
    
  •        string tenant)
    
  •    {
    
  •        clientId = applicationId;
    
  •        clientKey = applicationKey;
    
  •        string authority = String.Format(CultureInfo.InvariantCulture, "https://login.microsoftonline.com/{0}", tenant);
    
  •        authContext = new AuthenticationContext(authority);
    

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

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •        do
    
  •        {
    
  •            retry = false;
    
  •            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);
    
  •            }
    
  •            catch (AdalException ex)
    
  •            {
    
  •                if (ex.ErrorCode == "temporarily_unavailable")
    
  •                {
    
  •                    retry = true;
    
  •                    retryCount++;
    
  •                    Thread.Sleep(3000);
    

await Task.Delay(3000);

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  • public class AdalDaemonAuthenticationProvider : IAuthenticationProvider
  • {
  •    public AccountSession CurrentAccountSession { get; internal set; }
    
  •    string clientId;
    
  •    string clientKey;
    
  •    public AuthenticationContext authContext;
    
  •    ClientCredential clientCredential;
    
  •    // 'applicationId' : Your Application ID
    
  •    // 'applicationKey' : Your Application Key
    
  •    // 'tenant' : is usually a domain name for your Office365 service. Like 'yourcompany.onmicrosoft.com'
    
  •    public AdalDaemonAuthenticationProvider(
    
  •        string applicationId,
    
  •        string applicationKey,
    
  •        string tenant)
    

string applicationId,
string applicationKey,

Please use clientId and clientSecret like the rest of the library. Change

throughout this file.

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

+using System.Globalization;
+using System.Threading;
+
+namespace Microsoft.OneDrive.Sdk.Authentication.Business
+{

  • public class AdalDaemonAuthenticationProvider : IAuthenticationProvider
  • {
  •    public AccountSession CurrentAccountSession { get; internal set; }
    
  •    string clientId;
    
  •    string clientKey;
    
  •    public AuthenticationContext authContext;
    
  •    ClientCredential clientCredential;
    
  •    // 'applicationId' : Your Application ID
    
  •    // 'applicationKey' : Your Application Key
    

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>

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •    public async Task AuthenticateUserAsync(string serviceResourceId)
    
  •    {
    
  •        AuthenticationResult result = null;
    
  •        result = null;
    
  •        int retryCount = 0;
    
  •        bool retry = false;
    
  •        do
    
  •        {
    
  •            retry = false;
    
  •            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);
    

Again, AuthenticationContextWrapper

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •            {
    
  •                if (ex.ErrorCode == "temporarily_unavailable")
    
  •                {
    
  •                    retry = true;
    
  •                    retryCount++;
    
  •                    Thread.Sleep(3000);
    
  •                }
    
  •                Console.WriteLine(
    
  •                    String.Format("An error occurred while acquiring a token\nTime: {0}\nError: {1}\nRetry: {2}\n",
    
  •                    DateTime.Now.ToString(),
    
  •                    ex.ToString(),
    
  •                    retry.ToString()));
    
  •            }
    
  •        } while ((retry == true) && (retryCount < 3));
    

retryCount < 3

Make this a constant at the top of the file instead of an embedded literal.

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •    public async Task AuthenticateRequestAsync(HttpRequestMessage request)
    
  •    {
    
  •        if (this.CurrentAccountSession == null)
    
  •        {
    
  •            throw new ServiceException(
    
  •                new Error
    
  •                {
    
  •                    Code = OAuthConstants.ErrorCodes.AuthenticationFailure,
    
  •                    Message = "Please call one of the AuthenticateUserAsync...() methods to authenticate the user before trying to authenticate a request.",
    
  •                });
    
  •        }
    
  •        if (this.CurrentAccountSession.IsExpiring)
    
  •        {
    
  •            throw new ServiceException(
    

Refresh the token, similar to this
https://github.com/OneDrive/onedrive-sdk-dotnet-msa-auth-adapter/blob/master/src/OneDrive.Sdk.Authentication.Common/Business/AdalAuthenticationProviderBase.cs#L130

.

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •                            Message = ""
    
  •                        });
    
  •        }
    
  •        var accessTokenType = string.IsNullOrEmpty(this.CurrentAccountSession.AccessTokenType)
    
  •            ? OAuthConstants.Headers.Bearer
    
  •            : this.CurrentAccountSession.AccessTokenType;
    
  •        var uri = new UriBuilder(request.RequestUri);
    
  •        if (string.IsNullOrEmpty(uri.Query))
    
  •            uri.Query = string.Format("client_secret={0}", clientKey);
    
  •        else
    
  •            uri.Query = uri.Query.TrimStart('?') + string.Format("&client_secret={0}", clientKey);
    
  •        request.RequestUri = uri.Uri;
    

Extra whitespace

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •    // 'tenant' : is usually a domain name for your Office365 service. Like 'yourcompany.onmicrosoft.com'
    
  •    public AdalDaemonAuthenticationProvider(
    
  •        string applicationId,
    
  •        string applicationKey,
    
  •        string tenant)
    
  •    {
    
  •        clientId = applicationId;
    
  •        clientKey = applicationKey;
    
  •        string authority = String.Format(CultureInfo.InvariantCulture, "https://login.microsoftonline.com/{0}", tenant);
    
  •        authContext = new AuthenticationContext(authority);
    
  •        clientCredential = new ClientCredential(clientId, clientKey);
    
  •    }
    

Only 1 extra line

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •                {
    
  •                    retry = true;
    
  •                    retryCount++;
    
  •                    Thread.Sleep(3000);
    
  •                }
    
  •                Console.WriteLine(
    
  •                    String.Format("An error occurred while acquiring a token\nTime: {0}\nError: {1}\nRetry: {2}\n",
    
  •                    DateTime.Now.ToString(),
    
  •                    ex.ToString(),
    
  •                    retry.ToString()));
    
  •            }
    
  •        } while ((retry == true) && (retryCount < 3));
    

Extra whitespace

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •        {
    
  •            throw new ServiceException(
    
  •                        new Error
    
  •                        {
    
  •                            Code = OAuthConstants.ErrorCodes.AuthenticationFailure,
    
  •                            Message = ""
    
  •                        });
    
  •        }
    
  •        var accessTokenType = string.IsNullOrEmpty(this.CurrentAccountSession.AccessTokenType)
    
  •            ? OAuthConstants.Headers.Bearer
    
  •            : this.CurrentAccountSession.AccessTokenType;
    
  •        var uri = new UriBuilder(request.RequestUri);
    
  •        if (string.IsNullOrEmpty(uri.Query))
    
  •            uri.Query = string.Format("client_secret={0}", clientKey);
    

Add braces around if and else statement blocks

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •            : this.CurrentAccountSession.AccessTokenType;
    
  •        var uri = new UriBuilder(request.RequestUri);
    
  •        if (string.IsNullOrEmpty(uri.Query))
    
  •            uri.Query = string.Format("client_secret={0}", clientKey);
    
  •        else
    
  •            uri.Query = uri.Query.TrimStart('?') + string.Format("&client_secret={0}", clientKey);
    
  •        request.RequestUri = uri.Uri;
    
  •        request.Headers.Authorization = new AuthenticationHeaderValue(
    
  •            accessTokenType,
    
  •            this.CurrentAccountSession.AccessToken);
    
  •    }
    
  •    protected AccountSession ConvertAuthenticationResultToAccountSession(AuthenticationResult authenticationResult)
    

Use the existing implementation in AdalAuthenticationProviderBase

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

@@ -0,0 +1,134 @@
+using System;
+using System.Threading.Tasks;
+using Microsoft.Graph;
+using Microsoft.IdentityModel.Clients.ActiveDirectory;
+using System.Net.Http;
+using System.Net.Http.Headers;
+using System.Globalization;
+using System.Threading;
+
+namespace Microsoft.OneDrive.Sdk.Authentication.Business
+{

  • public class AdalDaemonAuthenticationProvider : IAuthenticationProvider

Is there a reason this doesn't extend AdalAuthenticationProviderBase?

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

  •            retry = false;
    
  •            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);
    
  •            }
    
  •            catch (AdalException ex)
    
  •            {
    
  •                if (ex.ErrorCode == "temporarily_unavailable")
    
  •                {
    
  •                    retry = true;
    
  •                    retryCount++;
    
  •                    Thread.Sleep(3000);
    
  •                }
    
  •                Console.WriteLine(
    

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.

In src/OneDrive.Sdk.Authentication.Desktop/Business/
AdalDaemonAuthenticationProvider.cs
#25 (review)
:

@@ -0,0 +1,134 @@
+using System;

Add the license

// ------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All Rights Reserved. Licensed under the MIT License. See License in the project root for license information.
// ------------------------------------------------------------------------------


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#25 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQ0OnqnlZSfkdM3MlPWYkggcDtM9d58ks5q3pybgaJpZM4Kcl2c
.

Humble S/W developer, Always.
Bruce Wang (왕성현)

@cdmayer
Copy link
Contributor

cdmayer commented Oct 26, 2016

Are you saying that calling AuthenticationContext.AcquireTokenSilentAsync works when you call it directly, but not when you call it with the context wrapper?

@lockelost
Copy link
Author

Oh, I was assuming that 'AcquireTokenSilentAsync()' will do the job.
Then I think I have to add new function for Daemon app case.
Let me add following new function to IAuthenticationContextWrapper and it's
child.

Task AcquireDaemonTokenSilentAsync(string resource,
ClientCredential clientCredential);

Will send you another pull request.

Thanks.

On Wed, Oct 26, 2016 at 6:07 PM, Chris Mayer [email protected]
wrote:

Are you saying that calling AuthenticationContext.AcquireTokenSilentAsync
works when you call it directly, but not when you call it with the context
wrapper?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#25 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQ0Ou_JVEzZz2Z4CnaX4cPGnWkkQYTmks5q388MgaJpZM4Kcl2c
.

Humble S/W developer, Always.
Bruce Wang (왕성현)

- 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;
Copy link
Contributor

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);
Copy link
Contributor

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;
Copy link
Contributor

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)
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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?

Copy link
Author

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)
Copy link
Contributor

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);
Copy link
Contributor

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)
Copy link
Contributor

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 :(

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.

2 participants