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

[local_auth] Implemented plugin to support local auth in macOS #5766

Closed
wants to merge 6 commits into from
Closed

[local_auth] Implemented plugin to support local auth in macOS #5766

wants to merge 6 commits into from

Conversation

OutdatedGuy
Copy link
Contributor

Created plugin local_auth_macos to support macOS platform for biometric and password authentication for the local_auth plugin.

The Objective-C code for macOS is built upon the implementation of iOS with necessary changes to work on macOS.

Working Demo:

Screen.Recording.2023-12-28.at.8.24.51.AM.mov

resolves flutter/flutter#140685

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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package 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.

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

@stuartmorgan
Copy link
Contributor

Per the comment in flutter/flutter#140685, the first step here would need to be a package rename; we wouldn't accept this as-is since it duplicates a significant amount of code.

Marking this as a draft pending a prequel PR to rename the package.

@stuartmorgan stuartmorgan marked this pull request as draft December 28, 2023 12:07
@jmagman
Copy link
Member

jmagman commented Jan 2, 2024

@OutdatedGuy thanks for contributing to this! An example of what @stuartmorgan is asking for (sharing between the platforms instead of copying) was done for path_provider and shared_preferences, as he mentioned in flutter/flutter#140685 (comment)

Examples:
flutter/plugins#6988
flutter/plugins#6920

@OutdatedGuy
Copy link
Contributor Author

@OutdatedGuy thanks for contributing to this! An example of what @stuartmorgan is asking for (sharing between the platforms instead of copying) was done for path_provider and shared_preferences, as he mentioned in flutter/flutter#140685 (comment)

Examples: flutter/plugins#6988 flutter/plugins#6920

Aahhh, now I get it.

@jmagman so what should I rename it to? local_auth_foundation as in examples you gave or local_auth_darwin?

@stuartmorgan
Copy link
Contributor

local_auth_foundation as in examples you gave or local_auth_darwin?

LocalAuthentication is not from the Foundation framework, so we would not call it local_auth_foundation.

@stuartmorgan
Copy link
Contributor

Examples: flutter/plugins#6988 flutter/plugins#6920

Aahhh, now I get it.

Note that those are a single PR for each plugin because we were merging existing implementations for two platforms, which is not the case for local_auth. So there won't be a single PR like in those examples, but instead a series of distinct PRs as a described in the issue.

@jmagman
Copy link
Member

jmagman commented Jan 5, 2024

@jmagman so what should I rename it to? local_auth_foundation as in examples you gave or local_auth_darwin?

I guess local_auth_darwin, naming is hard 🙂

@OutdatedGuy
Copy link
Contributor Author

@jmagman Questions while renaming:

  1. Should the IOSAuthMessages be renamed as DarwinAuthMessages to be used commonly for authMessages param in LocalAuthentication.authenticate.
  • If yes, then should the developers use Platform checking to show different messages for apple platforms if they want.
  • If no, then should be there 2 implementations when adding support for macOS later with 2 files for each platform:
    • lib/types/auth_messages_ios.dart & lib/types/auth_messages_macos.dart
    • pigeons/messages_ios.dart & pigeons/messages_macos.dart
    • Same in the darwin directory
  1. What should be the contents of the author in the local_auth_darwin.podspec file? As it would a new package(local_auth_darwin) should the name and email be mine or what?

@stuartmorgan
Copy link
Contributor

  1. Should the IOSAuthMessages be renamed as DarwinAuthMessages to be used commonly for authMessages param in LocalAuthentication.authenticate.

No; there is no reason to expect that strings would be the same. The default message "Biometric authentication is not set up on your device. Please either enable Touch ID or Face ID on your phone.", for instance, would make no sense on macOS.

  • If no, then should be there 2 implementations when adding support for macOS later with 2 files for each platform:

    • lib/types/auth_messages_ios.dart & lib/types/auth_messages_macos.dart

Yes.

  • pigeons/messages_ios.dart & pigeons/messages_macos.dart

No. Pigeon communication in internal to the plugin, and should be largely, if not entirely, identical.

  • Same in the darwin directory

It's not clear to me what this is referring to. I would not expect there to need to be two of anything in the darwin directory.

  1. What should be the contents of the author in the local_auth_darwin.podspec file? As it would a new package(local_auth_darwin) should the name and email be mine or what?

It's not a new package, it's a renaming of an existing package. The authorship of all the existing code you would be moving has not changed. You're welcome to add yourself, as with any PR to a package.

@OutdatedGuy
Copy link
Contributor Author

@stuartmorgan can you tell me what is exactly failing and how to solve it.

Also can you add the override: no versioning needed label.

@stuartmorgan
Copy link
Contributor

@stuartmorgan can you tell me what is exactly failing and how to solve it.

The project's unit tests appear to be configured incorrectly; I'll take a look in Xcode.

Also can you add the override: no versioning needed label.

Which of the linked exemptions do you believe that this falls under? And why would you not want people to be able to use the macOS implementation?

@stuartmorgan
Copy link
Contributor

The unit test file had not been added to the macOS project, so it couldn't build anything. I've pushed a fix.

@@ -156,6 +156,28 @@ - (void)showAlertWithMessage:(NSString *)message
dismissButtonTitle:(NSString *)dismissButtonTitle
openSettingsButtonTitle:(NSString *)openSettingsButtonTitle
completion:(FLAAuthCompletion)completion {
#if TARGET_OS_OSX
NSAlert *alert = [[NSAlert alloc] init];
[alert setMessageText:@""];
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this line actually do anything? What's the default value?

if (openSettingsButtonTitle != nil) {
[alert addButtonWithTitle:openSettingsButtonTitle];
[alert setShowsHelp:YES];
[alert setHelpAnchor:@"OpenSettings"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this magic string come from; is there a default system help anchor with this name? This needs a code comment explaining it.

}
[alert addButtonWithTitle:dismissButtonTitle];

NSModalResponse response = [alert runModal];
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use beginSheetModalForWindow:completionHandler: with the window obtained from the registrar, and handleSucceeded: should be called from the completion.

You'll nee do adjust the plugin's init methods to take the registrar from registerWithRegistrar: and keep a reference to it.

@@ -260,7 +283,12 @@ - (void)handleError:(NSError *)authError

#pragma mark - AppDelegate

- (void)applicationDidBecomeActive:(UIApplication *)application {
#if TARGET_OS_OSX
- (void)applicationDidBecomeActive:(NSNotification *)notification
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does macOS need this flow at all?

s.ios.dependency 'Flutter'
s.osx.dependency 'FlutterMacOS'
s.ios.deployment_target = '12.0'
s.osx.deployment_target = '10.15'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 10.15 rather than 10.14?

/// Class wrapping all authentication messages needed on macOS.
/// Provides default values for all messages.
@immutable
class MacOSAuthMessages extends AuthMessages {
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be identical to iOS currently, which means there should be a base class (I guess DarwinAuthMessages) that has all of this, and the two platform versions should just inherit from it with (currently) no additions.

Copy link
Contributor

Choose a reason for hiding this comment

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

(At which point the code for populating the Pigeon AuthStrings will become much simpler.)

@stuartmorgan
Copy link
Contributor

FYI, you'll need to resolve a raft of conflicts with #6108 once that lands. The easiest way to do it will most likely be to make the same changes in your code and then just take your copy of everything.

@jmagman
Copy link
Member

jmagman commented Feb 12, 2024

Heads up some of these classes are being renamed, though I don't see any obvious spots you'll merge conflict #6108
Edit well Stuart just said that, missed his comment 🙂

@stuartmorgan stuartmorgan mentioned this pull request Mar 5, 2024
11 tasks
@alexrabin-sentracam
Copy link
Contributor

The implementation of this pr seems to be outdated with the current local_auth_darwin approach. I made a new pr #6267

@stuartmorgan
Copy link
Contributor

Since this is marked as a draft and hasn't been updated in several months I'm going to close it to clean out our review queue. Please don't hesitate to submit a new PR if you decide to revisit this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[local_auth] Support for macOS platform
4 participants