[local_auth] Windows support. #4806

azchohfi · 2022-02-11T20:54:25Z

Adds Windows support for the local_auth plugin, using the Windows Hello APIs.

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#70270

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-02-11T20:54:27Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

azchohfi · 2022-02-15T21:02:31Z

No idea why these are failing.

# Conflicts: # packages/local_auth/local_auth/CHANGELOG.md # packages/local_auth/local_auth/pubspec.yaml

azchohfi · 2022-02-18T18:09:13Z

@stuartmorgan ios-platform_tests CHANNEL:master PLUGIN_SHARDING:--shardIndex 2 --shardCount 4: completed (failure)
Any help here? I have not touched the ios platform.

stuartmorgan-g · 2022-02-24T16:06:23Z

Any help here? I have not touched the ios platform.

It was likely flake (I'm not seeing the failure in the commit history, so I can't check.)

stuartmorgan-g

Very high-level notes:

Our policy for adding new platforms to a plugin in this repository is that we federate it first. local_auth is being federated right now, so this will need to be reworked as a new package once that lands.
This will need native unit tests so that we have test coverage of the C++ code. See #4156 for an example of what that looks like to add.
New implementation packages should follow the conclusion of https://docs.google.com/document/d/1BSrDA_FjLU-3T0H774RocJ4IYyc0ytMl937rAimGJdg and use its own internal method channel, so you'll need Dart code as well. While you could copy the existing one, since this is a completely new implementation I'd encourage you to adjust it to match the Windows implementation. In general we're trying to do more code in Dart rather than native code (for the reasons explained in the doc), so things like translating from the Windows API about availability to the list of strings expected by the platform interface would be good to move to the Dart side.

Some specific notes from a high-level review of the code below, but I haven't reviewed in detail since it'll need significant reworking for the above.

stuartmorgan-g · 2022-02-24T16:17:35Z

packages/local_auth/local_auth/windows/CMakeLists.txt

+set(PLUGIN_NAME "${PROJECT_NAME}_plugin")
+
+FetchContent_Declare(nuget
+  URL "https://dist.nuget.org/win-x86-commandline/v6.0.0/nuget.exe"


Should we consider instead making NuGet a doctor-recommended package, and referencing the VS copy instead? /cc @cbracken for thoughts on this.

VS doesn't include its own version of NuGet.exe, so it needs to be downloaded somewhere. It would be beneficial to have NuGet.exe inside Flutter's installation, yes, but not sure where. Also, I do not know the work required in Flutter to add an exe as a dependency. IMO, this should not be an error, but only a warning. Also, if we go this route, it would be great if doctor checked if nuget.exe is in the path already, and use that as the correct path, allowing developers to override the nuget exe's version.

VS doesn't include its own version of NuGet.exe, so it needs to be downloaded somewhere.

Ah, I just assumed it was since there's UI for installing packages with it in the IDE. If it's not bundled, this seems like the best approach; we generally don't ship toolchains.

We might want to add a doctor suggestion for developers to install it themselves and put it in their path though (so that this wouldn't need to run most of the time).

Visual Studio uses "MSBuild.exe /restore", not nuget.exe. AFAIK, VS's UI edits the csproj (in case of C# projects) and calls the restore, basically.
CMake 3.15 does support adding nuget package references, which are restored by msbuild, but there are issues with vcxprojs (C++) and PackageReferences (that is why vcxprojs use a packages.config file), so this doesn't actually work (unfortunately). https://stackoverflow.com/a/61910170/1272111

stuartmorgan-g · 2022-02-24T16:18:57Z

packages/local_auth/local_auth/windows/CMakeLists.txt

+endif()
+
+execute_process(COMMAND
+    ${NUGET} install Microsoft.Windows.CppWinRT -Version ${CPPWINRT_VERSION} -OutputDirectory packages


Will this install be completely localized to the plugin build, or will it have machine-level effects?

The only machine-level side effect of this is the download of CppWinRT's nuget file to Nuget's cache (~.nuget\packages), and the unzipping of it to that same folder, but it should be that, and only that: a cache. Each Flutter project that is built will have its own plugin folder, which will have its own copy of that cached folder locally (that is what the -OutputDirectory parameter is doing there).

stuartmorgan-g · 2022-02-24T16:24:24Z

packages/local_auth/local_auth/windows/local_auth_plugin.cpp

+#include <ppltasks.h>
+#include <windows.h>
+#include <winrt/Windows.Foundation.h>
+#include <winrt/Windows.Security.Credentials.UI.h>


We should clearly document the Windows version requirements in the README of the new package.

Since I'm not familiar with how WinRT linking works: what are the implications of the dependency here on support for older versions of Windows? Will a project that includes local_auth completely fail to launch on, e.g., Windows 7, or will it only be a problem if the plugin code is called?

(Pre-Windows-10 support definitely isn't required, I just want to make sure we know what will happen.)

packages/local_auth/local_auth/windows/local_auth_plugin.cpp

stuartmorgan-g · 2022-02-24T16:29:35Z

packages/local_auth/local_auth/windows/local_auth_plugin.cpp

+using namespace winrt;
+
+template <typename T>
+T GetArgument(const std::string arg, const EncodableValue* args, T fallback) {


Please make sure to document all functions and methods, per the C++ style guide.

stuartmorgan-g · 2022-02-24T16:31:31Z

packages/local_auth/local_auth/windows/local_auth_plugin.cpp

+                               UserConsentVerifierAvailability::Available) {
+      biometrics.push_back(EncodableValue("fingerprint"));
+      biometrics.push_back(EncodableValue("face"));
+      biometrics.push_back(EncodableValue("iris"));


This list is clearly not accurate. What actual information do we get about supported authentication options? We're in the process of changing the API (as part of the federation PRs) to also list strong and weak to match the Android options. Does one or both of those work instead? Is there a different set of things we should consider adding?

The existing Windows API being used here does not let us ask for a specific auth method, so it is up to the user to choose one that is available on their machine. That can be a combination of Fingerprint, Face, and or PIN.
I will ask internally here at Microsoft if there is guidance on querying those methods, but I'm assuming it would require some complex device GUID querying work, with likely false positives/negatives, but again, just an assumption.

If there's any reasonable correspondence with either the "weak" or "strong" designations that Android uses now we could just return that.

If we can't get any information an option would be to add something like "unknown" to the set of possible options in the API. Then clients can decide how they want to handle it.

I've started an internal thread on how this should be handled, and I'll get back to you with the recommendations, but I believe this is either gonna be a new value that needs to be created only for windows or, for better compatibility, 'weak'.

stuartmorgan-g · 2022-02-24T16:37:55Z

packages/local_auth/local_auth/windows/local_auth_plugin.cpp

+    std::unique_ptr<MethodResult<EncodableValue>> result) {
+  auto reasonW = s2ws(GetArgument<std::string>(
+      "localizedReason", method_call.arguments(), std::string()));
+


You are ignoring biometricOnly, which would be a serious security bug for some clients. We either need to support it, or if it's not possible with the available Windows APIs, error out when it's set to true.

I'm failing when this is true on Windows now.

stuartmorgan-g · 2022-03-25T19:34:28Z

Our policy for adding new platforms to a plugin in this repository is that we federate it first. local_auth is being federated right now, so this will need to be reworked as a new package once that lands.

FYI, this has been taking longer than expected to get finishing and landed, but is still happening.

stuartmorgan-g · 2022-04-08T00:36:15Z

The person working on the local_auth federation has picked up the PRs again recently, and they are looking very close, so hopefully this will be unblocked next week.

stuartmorgan-g · 2022-04-15T16:05:44Z

We've now completed and published the fully federated version of local_auth, so converting this to a local_auth_windows is unblocked. Sorry that took longer than expected!

stuartmorgan-g

I did another pass through the non-C++ parts; it looks like the C++ code still has some open comments from the last round so I'll wait on those.

packages/local_auth/local_auth_windows/example/README.md

packages/local_auth/local_auth_windows/windows/local_auth_plugin.cpp

packages/local_auth/local_auth_windows/lib/types/auth_messages_windows.dart

packages/local_auth/local_auth_windows/pubspec.yaml

packages/local_auth/local_auth_windows/test/local_auth_test.dart

azchohfi · 2022-05-13T22:14:50Z

@stuartmorgan I think I fixed it all. Let me know if there is anything I missed.

cbracken

Overall LGTM on the latest pass modulo a few pedantic nits around the doc comments.

packages/local_auth/local_auth_windows/windows/local_auth.h

packages/local_auth/local_auth_windows/windows/local_auth_plugin.cpp

packages/local_auth/local_auth_windows/windows/local_auth.h

packages/local_auth/local_auth_windows/windows/local_auth_plugin.cpp

azchohfi · 2022-05-13T22:38:20Z

@cbracken fixed them all. I wish the flutter_plugin_tools' format command fixed this.

cbracken · 2022-05-13T22:41:34Z

It would definitely be nice -- or at least try to call them out. The tricky part is code snippets etc. in comments and ASCII art. This is also why for example the dart format command won't rewrap comments.

stuartmorgan-g · 2022-05-13T23:32:40Z

I wish the flutter_plugin_tools' format command fixed this.

FYI for C++ it's just running clang-format. The tool isn't a formatter, it's just a wrapper to call the right formatter on the right files.

azchohfi · 2022-05-16T21:27:24Z

Anything else needed? I've merged the main branch again just to keep it up-to-date.

stuartmorgan-g

LGTM! Thanks so much for contributing this implementation, and for the patience with the restructuring of the plugin.

stuartmorgan-g · 2022-05-17T17:11:18Z

It's been published now, so is ready for an endorsement PR :)

azchohfi · 2022-05-17T17:49:02Z

@stuartmorgan #5776

flutter-dashboard bot added the needs tests label Feb 11, 2022

github-actions bot added p: local_auth platform-windows labels Feb 11, 2022

azchohfi force-pushed the local_auth_windows branch 2 times, most recently from e2e611c to 6836c3a Compare February 11, 2022 21:56

[local_auth] Windows support.

7ccd968

azchohfi force-pushed the local_auth_windows branch from 6836c3a to 7ccd968 Compare February 11, 2022 22:08

stuartmorgan-g self-requested a review February 17, 2022 21:35

Merge branch 'main' into local_auth_windows

3906da5

# Conflicts: # packages/local_auth/local_auth/CHANGELOG.md # packages/local_auth/local_auth/pubspec.yaml

Merge branch 'main' into local_auth_windows

70c341d

stuartmorgan-g suggested changes Feb 24, 2022

View reviewed changes

stuartmorgan-g requested a review from cbracken February 24, 2022 16:38

azchohfi added 2 commits February 24, 2022 14:38

Merge branch 'main' into local_auth_windows

c492f89

Handling biometricOnly parameter.

c997ad8

azchohfi added 2 commits April 7, 2022 16:00

Merge branch 'main' into local_auth_windows

9cf0a1a

Fixed readme

975ca8d

[local_auth] Fix version and readme.

3fcbca7

azchohfi added 4 commits April 15, 2022 11:44

Merge branch 'main' into local_auth_windows

34afbf5

[local_auth] Small fixes.

835f5cc

Small fix.

9eaba3c

Fixed tests.

22ec14c

azchohfi force-pushed the local_auth_windows branch from 0e6b141 to 22ec14c Compare April 15, 2022 21:25

Added local_auth_windows tests.

9349aab

azchohfi added 5 commits May 10, 2022 14:27

[local_auth_windows] Fixed changelog.

30f0958

[local_auth_windows] PR feedback.

97e1ee9

Merge branch 'main' into local_auth_windows

40aa9a2

Fixed build

6813b7b

Fixed format.

47823c6

stuartmorgan-g removed the needs tests label May 13, 2022

stuartmorgan-g suggested changes May 13, 2022

View reviewed changes

azchohfi added 3 commits May 13, 2022 14:03

Merge branch 'main' into local_auth_windows

45ceb6e

Applied more PR feedback.

271a49e

Added C++ comments.

e1e24a6

cbracken approved these changes May 13, 2022

View reviewed changes

Small doc fixes.

cabc43e

Merge branch 'main' into local_auth_windows

34a58de

stuartmorgan-g approved these changes May 17, 2022

View reviewed changes

stuartmorgan-g added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label May 17, 2022

fluttergithubbot merged commit 550ba3c into flutter:main May 17, 2022

azchohfi deleted the local_auth_windows branch May 17, 2022 17:34

engine-flutter-autoroll mentioned this pull request May 18, 2022

Roll Plugins from a692f840ce72 to bd3f490353b0 (9 revisions) flutter/flutter#104085

Merged

[local_auth] Windows support. #4806

[local_auth] Windows support. #4806

Uh oh!

Conversation

azchohfi commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 11, 2022

Uh oh!

azchohfi commented Feb 15, 2022

Uh oh!

azchohfi commented Feb 18, 2022

Uh oh!

stuartmorgan-g commented Feb 24, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Mar 25, 2022

Uh oh!

stuartmorgan-g commented Apr 8, 2022

Uh oh!

stuartmorgan-g commented Apr 15, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

azchohfi commented May 13, 2022

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

azchohfi commented May 13, 2022

Uh oh!

cbracken commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

azchohfi commented Feb 11, 2022 •

edited

Loading

stuartmorgan-g Feb 25, 2022 •

edited

Loading

stuartmorgan-g Feb 25, 2022 •

edited

Loading

cbracken commented May 13, 2022 •

edited

Loading