Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[local_auth] Convert Windows to Pigeon #7012

Merged
merged 8 commits into from
Jan 24, 2023

Conversation

stuartmorgan-g
Copy link
Contributor

Updates local_auth_windows to use Pigeon, and moves to a more platform-tailored and Dart-centric implementation (rather than keeping the previous cross-platform method channel interface that the current implementation was duplicated from):

  • Eliminates deviceSupportsBiometrics from the platform interface, since it's always the same as isDeviceSupported, in favor of doing that mapping in Dart.
  • Eliminates getEnrolledBiometrics from the platform interface, since it was the same implementation as isDeviceSupported just with a different return value, in favor of doing that logic in Dart.
  • Moves throwing for the biometricOnly option to the Dart side, simplifying the native logic.

Related changes:

  • Adds a significant amount of previously-missing Dart unit test coverage.
  • Removes the biometricOnly UI from the example app, since it will always fail.

Part of flutter/flutter#117912

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@stuartmorgan-g
Copy link
Contributor Author

@azchohfi If you could test this on supported hardware to make sure I didn't accidentally break anything there, that would be great; I don't have any so the only end-to-end manual test I could do was of no support.

@azchohfi
Copy link
Contributor

@stuartmorgan absolutely! Any Surface device should support Windows Hello, so I can test this quite easily, as I have one. Also, I've checked the source changes and LGTM. I'll just test it and approve.

@azchohfi
Copy link
Contributor

Working perfectly fine on my Windows 11 Lenovo with Windows Hello (PIN only). Call paths are exactly the same for biometric/non-biometric, so LGTM.

@azchohfi
Copy link
Contributor

Also, I'm assuming this is not a breaking change, since API surface and behavior is the same, right?

@stuartmorgan-g
Copy link
Contributor Author

Working perfectly fine on my Windows 11 Lenovo with Windows Hello (PIN only).

Great, thanks for testing!

Also, I'm assuming this is not a breaking change, since API surface and behavior is the same, right?

It's a grey area, because I changed biometricOnly from being a PlatformException to the more accurate UnsupportedError. That could break someone who was relying in that specific failure behavior, but in this case that seemed very unlikely since it never works so I opted not to consider it breaking. (For cases where we expect real-world code to be dealing with specific exceptions at runtime, we generally would.)

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2023
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

lgtm!

@stuartmorgan-g
Copy link
Contributor Author

Landing manually to re-open the tree, which is stuck red due to flutter/flutter#116226

@stuartmorgan-g stuartmorgan-g merged commit d649e18 into flutter:main Jan 24, 2023
@stuartmorgan-g stuartmorgan-g deleted the local-auth-windows-pigeon branch January 24, 2023 18:33
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
Updates `local_auth_windows` to use Pigeon, and moves to a more platform-tailored and Dart-centric implementation (rather than keeping the previous cross-platform method channel interface that the current implementation was duplicated from):
- Eliminates `deviceSupportsBiometrics` from the platform interface, since it's always the same as `isDeviceSupported`, in favor of doing that mapping in Dart.
- Eliminates `getEnrolledBiometrics` from the platform interface, since it was the same implementation as `isDeviceSupported` just with a different return value, in favor of doing that logic in Dart.
- Moves throwing for the `biometricOnly` option to the Dart side, simplifying the native logic.

Related changes:
- Adds a significant amount of previously-missing Dart unit test coverage.
- Removes the `biometricOnly` UI from the example app, since it will always fail.

Part of flutter/flutter#117912
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: local_auth platform-windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants