OneDrive / onedrive-sdk-dotnet-msa-auth-adapter

Other
26 stars 22 forks source link

OnlineIdAuthenticator support #9

Closed ericpapamarcos closed 7 years ago

ericpapamarcos commented 8 years ago

In a UWP (14393 SDK), calling this code shows a sign in dialog:

var msaAuthProvider = new MsaAuthenticationProvider(clientId, returnUrl, scopes);
await msaAuthProvider.AuthenticateUserAsync();

image

The previously recommended authentication approach (below) didn't show a sign in dialog, but rather a confirmation dialog that the app would get access to the specified scopes.

var client = await OneDriveClientExtensions.GetAuthenticatedClientUsingOnlineIdAuthenticator(scopes);
await client.AuthenticateAsync();

How can we automatically use the device's already signed in MSA with MsaAuthenticationProvider to avoid the full sign in dialog? This is blocking my adoption of the new MsaAuthenticationProvider from the previous approach.

cdmayer commented 8 years ago

Unfortunately, this has not been added to this library yet. Porting should be straightforward - take the OnlineIdAuthenticationProvider from v1,2 of the SDK and implement an analogous class that extends MsaAuthenticationProvider. You would want to create a method similar to GetAccountSessionAsync() using OnlineIdAuthenticator to get the account details, and you would call that from an override of AuthenticateUserAsync.

If you end up creating this class, please submit a Pull Request. Otherwise I am not sure how long until it gets built.

tgckpg commented 7 years ago

Cool! As duplicate, commenting to participate.

Noemata commented 7 years ago

What is the ETA for OnlineIdAuthenticator? It's a rather big whole in the current implementation. As stated by ericpapamarcos, removing these sorts of features blocks progress. A breaking change is acceptable provided functionality is preserved. The V2 API has been a regression. Given UWP is sitting on top of the OS, it's a bit baffling to me that OneDrive features are so obfuscated. All this should be directly accessible through StorageFolder without having to jump through hoops. I would rather see this as a user opt in capability that they control, with the default being to opt in. I am whining about this because a lot of my previous code is broken with the changes in V2. Yet again UWP takes a strange step backwards as time moves forward???

cdmayer commented 7 years ago

Working on an implementation in #18 . Anyone who can test out the implementation or review the code would be appreciated.

tgckpg commented 7 years ago

Just tested, looks good. I don't see the prompt anymore. Can do both PutAsync & GetAsync.

Just one more thing, could the ClientId be retrieved automatically rather than provided explicitly? Just asking, not a must tho... Tracing the code, the ClientId and ReturnUri could both be null when using OnlineIdAuthenticator. Advice to add an overload to just accepting the string[] Scope as the only argument.

Noemata commented 7 years ago

I tried adapting the PhotoBrowser code as follows:

                OnlineIdAuthenticationProvider osaAuthProvider = new OnlineIdAuthenticationProvider(
                    this.oneDriveConsumerClientId,
                    this.oneDriveConsumerReturnUrl,
                    this.scopes
                    );

                authTask = osaAuthProvider.RestoreMostRecentFromCacheOrAuthenticateUserAsync();
                app.OneDriveClient = new OneDriveClient(this.oneDriveConsumerBaseUrl, osaAuthProvider);
                app.AuthProvider = osaAuthProvider;

            try
            {
                await authTask;

                app.NavigationStack.Add(new ItemModel(new Item()));
                this.Frame.Navigate(typeof(MainPage), e);
            }
            catch (ServiceException exception)
            {
                // Swallow the auth exception but write message for debugging.
                Debug.WriteLine(exception.Error.Message);
            }

But I suppose the RestoreMostRecentFromCacheOrAuthenticateUserAsync() does not apply here. What is the correct way to do this?

tgckpg commented 7 years ago

@Noemata You don't need to RestoreMostRecentFromCacheOrAuthenticateUserAsync here. Just do AuthenticateAsync and you are all good.

Here is how I do it, hope this help:)

...
public OneDriveSync()
{
    MSAuth = new OnlineIdAuthenticationProvider(
        null
        , null
        , new string[] { "onedrive.appfolder", "wl.signin" }
    );
}

public async Task Authenticate()
{
    if ( !Properties.ENABLE_ONEDRIVE ) return;

    if ( Authenticated ) return;

    try
    {
        await MSAuth.AuthenticateUserAsync();

        OneDriveClient Client = new OneDriveClient( "https://api.onedrive.com/v1.0", MSAuth );
        this.Client = Client;
        Logger.Log( ID, "Signed In", LogType.INFO );
    }
    catch ( ServiceException ex )
    {
        Logger.Log( ID, ex.Error.Message, LogType.WARNING );
    }
}
...
Noemata commented 7 years ago

Thank you for the clarification tgckpg. I am getting the same exception as before:

"The application requesting authentication tokens is either disabled or incorrectly configured. (Exception from HRESULT: 0x80860003)"

My revised code in the PhotoBrowser app is as follows:

                OnlineIdAuthenticationProvider osaAuthProvider = new OnlineIdAuthenticationProvider(
                    null,
                    null,
                    this.scopes
                    );

                try
                {
                    await osaAuthProvider.AuthenticateUserAsync();

                    app.OneDriveClient = new OneDriveClient(this.oneDriveConsumerBaseUrl, osaAuthProvider);
                    app.AuthProvider = osaAuthProvider;
                    app.NavigationStack.Add(new ItemModel(new Item()));
                    this.Frame.Navigate(typeof(MainPage), e);
                }
                catch (ServiceException exception)
                {
                    // Swallow the auth exception but write message for debugging.
                    Debug.WriteLine(exception.Error.Message);
                }

See InitializeClient() method in:

https://github.com/OneDrive/onedrive-sample-photobrowser-uwp/blob/master/OneDrivePhotoBrowser/AccountSelection.xaml.cs

I replaced the entire "else" clause with the above code and removed the try/catch that followed the else.

tgckpg commented 7 years ago

@Noemata A quick duckduckgo reveals that you need to associate your app with Store.

Noemata commented 7 years ago

Here's the entire code blob. I think this is correct, although it does not work:

[ AccountSelection.xaml]

<Page x:Class="OneDrivePhotoBrowser.AccountSelection" xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:local="using:OneDrivePhotoBrowser" xmlns:d="http://schemas.microsoft.com/expression/blend/2008" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" mc:Ignorable="d" >

<Grid Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">
    <StackPanel Orientation="Vertical" VerticalAlignment="Center">
        <Button x:Name="MsaButton" Margin="20" Content="Log in to MSA" HorizontalAlignment="Center" VerticalAlignment="Center" Click="MsaButton_Click"  />
        <Button x:Name="AadButton" Margin="20" Content="Log in to AAD" HorizontalAlignment="Center" VerticalAlignment="Center" Click="AadButton_Click"  />
        <Button x:Name="OsaButton" Margin="20" Content="Log in to OSA" HorizontalAlignment="Center" VerticalAlignment="Center" Click="OsaButton_Click"  />
    </StackPanel>
</Grid>

[AccountSelection.cs ]

// ------------------------------------------------------------------------------ // Copyright (c) 2015 Microsoft Corporation // // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal // in the Software without restriction, including without limitation the rights // to use, copy, modify, merge, publish, distribute, sublicense, and/or sell // copies of the Software, and to permit persons to whom the Software is // furnished to do so, subject to the following conditions: // // The above copyright notice and this permission notice shall be included in // all copies or substantial portions of the Software. // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, // FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. // ------------------------------------------------------------------------------

// The Blank Page item template is documented at http://go.microsoft.com/fwlink/?LinkId=234238

namespace OneDrivePhotoBrowser { using Microsoft.Graph; using Microsoft.OneDrive.Sdk; using Microsoft.OneDrive.Sdk.Authentication; using Models; using System; using System.Diagnostics; using System.Threading.Tasks; using Windows.UI.Xaml; using Windows.UI.Xaml.Controls;

/// <summary>
/// An empty page that can be used on its own or navigated to within a Frame.
/// </summary>
public sealed partial class AccountSelection : Page
{
    private enum ClientType
    {
        Business,
        Consumer,
        Online
    }

    // Set these values to your app's ID and return URL.
    private readonly string oneDriveForBusinessClientId = "Insert your OneDrive for Business client id";
    private readonly string oneDriveForBusinessReturnUrl = "http://localhost:8080";
    private readonly string oneDriveForBusinessBaseUrl = "https://graph.microsoft.com/";

    private readonly string oneDriveConsumerClientId = "Insert your OneDrive Consumer client id";
    private readonly string oneDriveConsumerReturnUrl = "https://login.live.com/oauth20_desktop.srf";
    private readonly string oneDriveConsumerBaseUrl = "https://api.onedrive.com/v1.0";
    private readonly string[] scopes = new string[] { "onedrive.readonly", "wl.signin", "offline_access" };

    public AccountSelection()
    {
        this.InitializeComponent();
        this.Loaded += AccountSelection_Loaded;
    }

    private async void AccountSelection_Loaded(object sender, RoutedEventArgs e)
    {
        var app = ((App) Application.Current);

        if (app.OneDriveClient != null)
        {
            var msaAuthProvider = app.AuthProvider as MsaAuthenticationProvider;
            var adalAuthProvider = app.AuthProvider as AdalAuthenticationProvider;
            if (msaAuthProvider != null)
            {
                await msaAuthProvider.SignOutAsync();
            }
            else if (adalAuthProvider != null)
            {
                await adalAuthProvider.SignOutAsync();
            }

            app.OneDriveClient = null;
        }

        // Don't show AAD login if the required AAD auth values aren't set
        if (string.IsNullOrEmpty(this.oneDriveForBusinessClientId) || string.IsNullOrEmpty(this.oneDriveForBusinessReturnUrl))
        {
            this.AadButton.Visibility = Visibility.Collapsed;
        }
    }

    private void AadButton_Click(object sender, RoutedEventArgs e)
    {
        this.InitializeClient(ClientType.Business, e);
    }

    private void MsaButton_Click(object sender, RoutedEventArgs e)
    {
        this.InitializeClient(ClientType.Consumer, e);
    }

    private void OsaButton_Click(object sender, RoutedEventArgs e)
    {
        this.InitializeClient(ClientType.Online, e);
    }

    private async void InitializeClient(ClientType clientType, RoutedEventArgs e)
    {
        var app = (App) Application.Current;
        if (app.OneDriveClient == null)
        {
            Task authTask;

            if (clientType == ClientType.Business)
            {
                var adalAuthProvider = new AdalAuthenticationProvider(
                    this.oneDriveForBusinessClientId,
                    this.oneDriveForBusinessReturnUrl);
                authTask = adalAuthProvider.AuthenticateUserAsync(this.oneDriveForBusinessBaseUrl);
                app.OneDriveClient = new OneDriveClient(this.oneDriveForBusinessBaseUrl + "/_api/v2.0", adalAuthProvider);
                app.AuthProvider = adalAuthProvider;
            }
            else if (clientType == ClientType.Consumer)
            {
                var msaAuthProvider = new MsaAuthenticationProvider(
                    this.oneDriveConsumerClientId,
                    this.oneDriveConsumerReturnUrl,
                    this.scopes,
                    new CredentialVault(this.oneDriveConsumerClientId));
                authTask = msaAuthProvider.RestoreMostRecentFromCacheOrAuthenticateUserAsync();
                app.OneDriveClient = new OneDriveClient(this.oneDriveConsumerBaseUrl, msaAuthProvider);
                app.AuthProvider = msaAuthProvider;
            }
            else
            {
                var osaAuthProvider = new OnlineIdAuthenticationProvider(
                    null,
                    null,
                    this.scopes
                    );

                authTask = osaAuthProvider.AuthenticateUserAsync();
                app.OneDriveClient = new OneDriveClient(this.oneDriveConsumerBaseUrl, osaAuthProvider);
                app.AuthProvider = osaAuthProvider;
            }

            try
            {
                await authTask;

                app.NavigationStack.Add(new ItemModel(new Item()));
                this.Frame.Navigate(typeof(MainPage), e);
            }
            catch (ServiceException exception)
            {
                // Swallow the auth exception but write message for debugging.
                Debug.WriteLine(exception.Error.Message);
            }
        }
        else
        {
            this.Frame.Navigate(typeof(MainPage), e);
        }
    }
}

}

Noemata commented 7 years ago

Just noticed your reply tgckpg. That makes sense although it was not required with the previous API. I'll give that a try.

Noemata commented 7 years ago

You are correct @tgckpg, associating the app with the Store resolved the issue. I did notice a problem with Msa login after an Online login. Perhaps @cdmayer can clean up the Picture Browser sample so all login variations play nice with each other. It should be possible to do an Online login and still leverage the cached Msa login information after you logout. I also agree with @tgckpg's suggestion of setting default params to null for the Online login.

cdmayer commented 7 years ago

Thanks @Noemata and @tgckpg for the feedback. Merged #18. Check out the README!

@Noemata, I will take a look at the sample app, too. I am thinking it would be nice to have 3 buttons: MSA, OnlineId, and AAD (just so everyone can see how the code is different). Again, thanks for the feedback.

tgckpg commented 7 years ago

Mr Mayer, any plans to release the update in nuget?

cdmayer commented 7 years ago

Yes. That should be coming next week.

jaycdave88 commented 7 years ago

Has this been rolled out yet?