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

Adding WindowsProvider #69

Conversation

shweaver-MSFT
Copy link
Member

@shweaver-MSFT shweaver-MSFT commented Feb 9, 2021

Fixes #

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

What is the new behavior?

I've written a new WindowsProvider and some supporting code to enable authentication through the native Windows APIs, instead of via MSAL.

This also introduces a new pattern for consuming provider configuration from XAML:

<providers:Graph.Config>
    <!--<msal:MsalConfig ClientId="YOUR_CLIENT_ID_HERE" Scopes="User.Read,User.ReadBasic.All,People.Read,Calendars.Read,Mail.Read,Group.Read.All,ChannelMessage.Read.All" />-->
    <!--<uwp:WindowsConfig ClientId="YOUR_CLIENT_ID_HERE" Scopes="User.Read,User.ReadBasic.All,People.Read,Calendars.Read,Mail.Read,Group.Read.All,ChannelMessage.Read.All" />-->
    <providers:MockConfig />
</providers:Graph.Config>

This method uses attached dependency properties instead of behaviors, allowing us to remove the behaviors based implementation in the future. Although, it does come with some minor changes in behavior (no pun intended). The previous implementation allowed the GlobalProvider to be set way before the rest of the page gets parsed. In the new method, the Graph config values get set a little later on. This means that any GlobalProvider based logic needs to be resilient and detect changes in the presence of the GlobalProvider. This is probably good practice anyway, so expect some bugs in the meantime.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

Known issues:

  • Token refresh doesn't work yet. When the token expires Graph calls will stop working. Sign out and in again to fix it.
  • Using the WindowsProvider requires an association with the store. to work Need to add some guidance for this.

@shweaver-MSFT shweaver-MSFT self-assigned this Feb 9, 2021
@ghost
Copy link

ghost commented Feb 9, 2021

Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost assigned Kyaa-dost Feb 9, 2021
@shweaver-MSFT
Copy link
Member Author

@michael-hawker, do you know what NBGV is? It seems to be causing the CI to fail because it won't install. Any ideas?

@azchohfi
Copy link
Collaborator

azchohfi commented Feb 10, 2021

@shweaver-MSFT NBGV is the git version tool we use. NerdBankGitVersioning (https://github.com/dotnet/Nerdbank.GitVersioning)
The build is failing due to the fact that this nuget.config is still pointing to our old myget. Remove the "MyGet" feed entry on the nuget.config, and add a reference to the new one:
<add key="AzureLatest" value="https://pkgs.dev.azure.com/dotnet/WindowsCommunityToolkit/_packaging/WindowsCommunityToolkit-MainLatest/nuget/v3/index.json" protocolVersion="3" />

@shweaver-MSFT

This comment has been minimized.

@michael-hawker
Copy link
Member

@shweaver-MSFT yeah I had opened #68 for updating our config file too. 😉 Awesome to see this coming together! I wonder if this would make it easier though to grab all the scopes required of controls being loaded and better handle passing those to the Provider on initialization? 🤔

@azchohfi a Lottie build was failing earlier too with a similar error about Cake, did something in cake update again and break us? ☹

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Couple initial comments and questions. Will have to take it for a spin soon! 🙂

@@ -280,8 +283,8 @@
</StackPanel>
<wgt:GraphPresenter Grid.Row="1"
IsCollection="True"
RequestBuilder="{x:Bind local:MainPage.GetTeamsChannelMessagesBuilder('02bd9fd6-8f93-4758-87c3-1fb73740a315', '19:d0bba23c2fc8413991125a43a54cc30e@thread.skype'), Mode=OneWay}"
Copy link
Member

Choose a reason for hiding this comment

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

Ah, so the late bind provider causes the Provider Instance to be null in the function, eh?

Hmm, will have to think about how we can have dynamic binding here for other scenarios. The Teams scenario is a bit odd as an example for this one. May need another helper or something... Eventually we'll want to be able to have folks have some way of playing with this live in the Sample App, but we can worry about that later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, the new setup broke that binding pattern. I'm sure we can come up with a helper for this that is aware of GlobalProvider changes.

@shweaver-MSFT shweaver-MSFT added the DO NOT MERGE ⚠️ Don't merge this label Feb 10, 2021
@michael-hawker
Copy link
Member

@shweaver-MSFT Alex looked and saw cake updated and broke us again, we have a config file on the main repo here: https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/master/build/tools/packages.config

We just need a copy of that in this repo for now until we figure out upgrading to the new Cake version.

@shweaver-MSFT shweaver-MSFT removed the DO NOT MERGE ⚠️ Don't merge this label Feb 12, 2021
shweaver-MSFT and others added 2 commits February 18, 2021 15:37
Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>
@shweaver-MSFT shweaver-MSFT added the DO NOT MERGE ⚠️ Don't merge this label Mar 1, 2021
@shweaver-MSFT
Copy link
Member Author

Re-adding the DO NOT MERGE label. I think this PR is too big now. We can keep chatting here, but I would like to introduce the WindowsProvider separately from the other work to remove behaviors. Once we settle on a plan, I'll close and open up new PRs.

@michael-hawker michael-hawker marked this pull request as draft March 8, 2021 20:14
@michael-hawker
Copy link
Member

@shweaver-MSFT to clarify, this'll be redone on top of #76 once we settle on that?

Trying to get context all-up, so going to look at this again quick.

}

/// <inheritdoc />
public override Task AuthenticateRequestAsync(HttpRequestMessage request)
Copy link
Member

Choose a reason for hiding this comment

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

I think the root/crux of our issues is that this method is required from IAuthenticationProvider which we only get from pulling in the whole Graph SDK (well auth package currently, but eventually), eh?

That means even if we wanted to use the Windows Provider without Graph we're still pulling in Graph? Like what other scenarios can we use the provider for outside of graph? (Or I guess if you decided to use this to add the header to general rest call still without the graph sdk explicitly?)

@shweaver-MSFT @nmetulev I think this is the big issue we need to understand how to make everything work now and in the future together with @darrelmiller.

@shweaver-MSFT
Copy link
Member Author

I've made some fundamental changes to my approach. Closing this and will reopen another with accurate details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE ⚠️ Don't merge this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants