packages icon indicating copy to clipboard operation
packages copied to clipboard

[local_auth] Implemented plugin to support local auth in macOS

Open OutdatedGuy opened this issue 1 year ago • 14 comments

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:

https://github.com/flutter/packages/assets/74326345/71f31ec1-8d67-4208-9d06-ef2dd8f7ad89


resolves https://github.com/flutter/flutter/issues/140685

Pre-launch Checklist

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

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

OutdatedGuy avatar Dec 28 '23 03:12 OutdatedGuy

Per the comment in https://github.com/flutter/flutter/issues/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-g avatar Dec 28 '23 12:12 stuartmorgan-g

@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 https://github.com/flutter/flutter/issues/140685#issuecomment-1871098814

Examples: https://github.com/flutter/plugins/pull/6988 https://github.com/flutter/plugins/pull/6920

jmagman avatar Jan 02 '24 21:01 jmagman

@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?

OutdatedGuy avatar Jan 03 '24 09:01 OutdatedGuy

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-g avatar Jan 03 '24 11:01 stuartmorgan-g

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.

stuartmorgan-g avatar Jan 03 '24 11:01 stuartmorgan-g

@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 🙂

jmagman avatar Jan 05 '24 00:01 jmagman

@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?

OutdatedGuy avatar Jan 05 '24 12:01 OutdatedGuy

  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.

stuartmorgan-g avatar Jan 05 '24 13:01 stuartmorgan-g

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

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

OutdatedGuy avatar Feb 12 '24 12:02 OutdatedGuy

@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-g avatar Feb 12 '24 15:02 stuartmorgan-g

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

stuartmorgan-g avatar Feb 12 '24 15:02 stuartmorgan-g

FYI, you'll need to resolve a raft of conflicts with https://github.com/flutter/packages/pull/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.

stuartmorgan-g avatar Feb 12 '24 20:02 stuartmorgan-g

~~Heads up some of these classes are being renamed, though I don't see any obvious spots you'll merge conflict https://github.com/flutter/packages/pull/6108~~ Edit well Stuart just said that, missed his comment 🙂

jmagman avatar Feb 12 '24 21:02 jmagman

The implementation of this pr seems to be outdated with the current local_auth_darwin approach. I made a new pr https://github.com/flutter/packages/pull/6267

alexrabin-sentracam avatar Mar 05 '24 19:03 alexrabin-sentracam

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!

stuartmorgan-g avatar Apr 09 '24 20:04 stuartmorgan-g