Skip to content

fix(auth, iOS): add detailed error handling for Apple authorization failures#18415

Open
SelaseKay wants to merge 2 commits into
mainfrom
auth_18413
Open

fix(auth, iOS): add detailed error handling for Apple authorization failures#18415
SelaseKay wants to merge 2 commits into
mainfrom
auth_18413

Conversation

@SelaseKay

Copy link
Copy Markdown
Contributor

Description

Replace this paragraph with a description of what this PR is doing. If you're modifying existing behavior, describe the existing behavior, how this PR is changing it, and what motivated the change.

Related Issues

Fixes #18413

Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR. Note that you'll have to prefix the issue numbers with flutter/flutter#.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@gemini-code-assist

Copy link
Copy Markdown
Contributor
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

@bjrochem72

Copy link
Copy Markdown

Thanks for picking this up so quickly — the native side is exactly what I was hoping for.

Two things I noticed when tracing what happens to the new error on the Dart side, one of which I think is worth fixing before merge:

1. The Underlying: label gets the message truncated in Dart.

In firebase_auth_platform_interface/lib/src/method_channel/utils/exception.dart, the Pigeon path does:

var parsedMessage = platformException.message?.split(': ').last;

The new format includes the literal ", Underlying: Domain=%@ Code=%ld", so whenever an underlying error is present, everything before that ": " is discarded. For example:

Built natively:
The operation couldn't be completed. (com.apple.AuthenticationServices.AuthorizationError error 1000.) (Domain=com.apple.AuthenticationServices.AuthorizationError Code=1000, Underlying: Domain=AKAuthenticationError Code=-7026)

Received in Dart:
Domain=AKAuthenticationError Code=-7026)

— the localizedDescription and the primary Domain/Code are lost, which is the exact case (AKAuthenticationError −7026) that motivated the issue. The no-underlying case is fine, since Apple's synthesized descriptions don't contain ": ".

Minimal fix: drop the colon from the label, e.g.

[NSString stringWithFormat:@", Underlying Domain=%@ Code=%ld", ...]

2. The structured details dictionary doesn't currently reach app code.

platformExceptionToFirebaseAuthException only reads authCredential and email from details, and FirebaseAuthException has no field to carry the rest, so nativeErrorDomain / nativeErrorCode / underlyingNativeErrorDomain / underlyingNativeErrorCode are dropped during conversion. Totally fine by me if the message stays the single source (that's parseable once point 1 is fixed) — just flagging it so the keys aren't assumed to be reachable from Dart.

Happy to test the branch against the device states from the original report once it's updated.

@SelaseKay

Copy link
Copy Markdown
Contributor Author

Good catch @bjrochem72. I've updated the PR. Will wait for your feedback on the test.

@bjrochem72

Copy link
Copy Markdown

Tested the updated branch on device (iOS 26.4 simulator, no Apple ID signed in — the AuthorizationError 1000 case from the report). Confirming the truncation fix in 5a0c012d works: Dart now receives

[firebase_auth/unknown] The operation couldn't be completed. (com.apple.AuthenticationServices.AuthorizationError error 1000.) (Domain=com.apple.AuthenticationServices.AuthorizationError Code=1000)

— the primary Domain/Code survive the platform-interface message.split(': ').last path intact, where before this PR it was just An unknown error has occurred. The colon-free , Underlying Domain=… Code=… label is the right fix for the underlying-error case; I wasn't able to force AKAuthenticationError -7026 deterministically on device, but by inspection nothing before that label can be truncated now, so 👍.

Note on my harness: I couldn't pull the auth_18413 branch directly because its Dart layer references FirebasePlugin/pluginConstants from unreleased firebase_core master. I tested by applying just this PR's FLTFirebaseAuthPlugin.m on top of the released firebase_auth 6.5.4 — so this exercises exactly the native change.

On "the test": I only see FLTFirebaseAuthPlugin.m in the diff — if you pushed a test it may not have synced. Since the details dictionary isn't surfaced to Dart, an ObjC-level unit test over convertAppleAuthorizationErrorToFlutterError: (assert the formatted message and the nativeErrorDomain/nativeErrorCode/underlying keys, with and without an NSUnderlyingError) would cover it best. Happy to review once it's up.

Unrelated: the web (tests) red looks like infra/flake — this is an iOS-only ObjC change.

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

Labels

None yet

Projects

None yet

2 participants