auth0-flutter icon indicating copy to clipboard operation
auth0-flutter copied to clipboard

implement app state getter for web

Open navaronbracke opened this issue 10 months ago • 27 comments

Fixes https://github.com/auth0/auth0-flutter/issues/486 Replaces https://github.com/auth0/auth0-flutter/pull/485 (I had an issue with signing the commits during the interactive rebase, so I started over)

  • [x] All new/changed/fixed functionality is covered by tests (or N/A)
  • [x] I have added documentation for all new/changed functionality (or N/A)

📋 Changes

This PR adds support for the appState parameter to loginWithRedirect() for the web. See https://github.com/auth0/auth0-spa-js/blob/f2e566849efa398ca599daf9ebdfbbd62fcb1894/src/global.ts#L298

There is prior art in https://github.com/auth0/auth0-angular/pull/168

🎯 Testing

Added several unit tests. This can be tested end-to-end by setting up a login with redirect, that uses the app state argument as parameter, and then validating that the app state is returned when the user is redirected back. I did verify end-to-end using an internal application that uses the SDK integration.

To run the unit tests locally, use flutter test test/web/auth0_flutter_web_test.dart --platform=chrome

navaronbracke avatar Feb 11 '25 12:02 navaronbracke

@Widcket Apologies, I had an issue during the interactive rebase when trying to fix the commit signing, so I had to start over. I also applied your comments from the previous PR into this one.

navaronbracke avatar Feb 11 '25 12:02 navaronbracke

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.16%. Comparing base (253209a) to head (3326ae1). Report is 147 commits behind head on main.

Files with missing lines Patch % Lines
...PI/AuthAPIPhoneNumberPasswordlessLoginMethod.swift 0.00% 1 Missing :warning:
Additional details and impacted files
@@              Coverage Diff              @@
##               main     #509       +/-   ##
=============================================
- Coverage     96.08%   84.16%   -11.93%     
=============================================
  Files            97      108       +11     
  Lines          1611     1907      +296     
  Branches        331      424       +93     
=============================================
+ Hits           1548     1605       +57     
- Misses           49      289      +240     
+ Partials         14       13        -1     
Flag Coverage Δ
auth0_flutter 91.72% <ø> (-8.28%) :arrow_down:
auth0_flutter_android 80.65% <ø> (-15.91%) :arrow_down:
auth0_flutter_ios 83.78% <0.00%> (-16.07%) :arrow_down:
auth0_flutter_platform_interface 87.00% <100.00%> (-0.04%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Feb 11 '25 12:02 codecov[bot]

I don't understand why the Sentry report might still be reporting the getter as uncovered? The Auth0FlutterWebPlatform class in which it is defined is abstract, so cannot be instantiated. The StubAuth0FlutterWeb class, which implements Auth0FlutterWebPlatform tests the getter in https://github.com/auth0/auth0-flutter/pull/509/files#diff-b6191cf5b6dc7f251d6af466701eb8de71f4bb8fdebb1bfb2cfdbbf865bec83eR121

navaronbracke avatar Feb 11 '25 13:02 navaronbracke

It seems that the upstream iOS & MacOS tests are broken currently? This prevents new test reports from being made.

navaronbracke avatar Feb 12 '25 07:02 navaronbracke

It seems that the upstream iOS & MacOS tests are broken currently? This prevents new test reports from being made.

The smoke tests are, but the coverage is not taken from those but from the unit tests.

Widcket avatar Feb 21 '25 13:02 Widcket

@Widcket Code coverage should be fine now. PTAL.

navaronbracke avatar Feb 24 '25 09:02 navaronbracke

@Widcket I had a look at the failing tests:

The failing iOS tests seem to be due to CI using XCode 15.0.1 ?

When I open the relevant code that is outlined in the logs

/Users/runner/work/auth0-flutter/auth0-flutter/auth0_flutter/example/ios/Pods/Auth0/Auth0/UIWindow+TopViewController.swift:14:40: error: cannot call value of non-function type 'UIApplication'
        guard let root = UIApplication.shared()?.windows.last(where: \.isKeyWindow)?.rootViewController else {
                                       ^     ~~
                                             
/Users/runner/work/auth0-flutter/auth0-flutter/auth0_flutter/example/ios/Pods/Auth0/Auth0/UIWindow+TopViewController.swift:14:70: error: cannot infer key path type from context; consider explicitly specifying a root type
        guard let root = UIApplication.shared()?.windows.last(where: \.isKeyWindow)?.rootViewController else {
                                                                     ^

I don't see any compilation warnings / errors when running this with XCode version 16.2 (16C5032a)

Screenshot 2025-02-27 at 15 25 49

As for the failing macOS tests, there is a redundant let keyword, which can be removed to fix the warning:

Screenshot 2025-02-27 at 15 33 19

I'll add the fix for the let keyword in this PR, since that is something I can do manually.

navaronbracke avatar Feb 27 '25 14:02 navaronbracke

The failing iOS tests seem to be due to CI using XCode 15.0.1 ? When I open the relevant code that is outlined in the logs

Thank you for taking a look. It should compile on Xcode 15.0.1, as the underlying Swift SDK (Auth0.swift) does. Also, we need to test using Xcode 15, as it's still possible to use it to compile apps to submit to the App Store. That is set to change on April:

Screenshot 2025-02-27 at 14 56 53

Widcket avatar Feb 27 '25 15:02 Widcket

Maybe we should update to the latest hotfix version for XCode 15, which is XCode 15.4? Maybe the fix for that problem is in one of the hotfixes for XCode 15 ? We could try switching to XCode 15.1 -> 15.2 -> 15.3 -> 15.4 to see which version fixed the bug?

navaronbracke avatar Feb 27 '25 15:02 navaronbracke

That's a good idea, though I'll start right with 15.4, and if that fixes it, then try one version back at a time.

Widcket avatar Feb 27 '25 15:02 Widcket

Nope, that didn't fix it https://github.com/auth0/auth0-flutter/actions/runs/13570466141/job/37938452729?pr=520

Widcket avatar Feb 27 '25 20:02 Widcket

Well, now there is a different issue for iOS:

/Users/runner/work/auth0-flutter/auth0-flutter/auth0_flutter/example/ios/Pods/Auth0/Auth0/ASProvider.swift:70:47: error: cannot assign value of type 'ASUserAgent' to type '(any ASWebAuthenticationPresentationContextProviding)?'
        session.presentationContextProvider = self
                                              ^~~~
/Users/runner/work/auth0-flutter/auth0-flutter/auth0_flutter/example/ios/Pods/Auth0/Auth0/ASProvider.swift:70:47: note: add missing conformance to 'ASWebAuthenticationPresentationContextProviding' to class 'ASUserAgent'
        session.presentationContextProvider = self
                                              ^

navaronbracke avatar Feb 27 '25 22:02 navaronbracke

Well, now there is a different issue for iOS:

My bad. That's the issue I thought you meant could be fixed by updating Xcode. I see now that there is Ruby problem, and I just raised https://github.com/auth0/auth0-flutter/pull/523 to fix it.

Regarding error: cannot assign value of type 'ASUserAgent' to type '(any ASWebAuthenticationPresentationContextProviding)?', it doesn't happen for Auth0.swift (it compiles fine) and only happens here when the GitHub actions get cached. I have to look deeper into it.

Widcket avatar Feb 28 '25 18:02 Widcket

The native iOS unit tests seem to pass on your PR for the Ruby version? Interesting, maybe there is indeed a caching issue in Github Actions.

navaronbracke avatar Feb 28 '25 19:02 navaronbracke

Since we now have WASM support, I am in the process of updating the web specific mocking for the tests.

navaronbracke avatar Mar 04 '25 13:03 navaronbracke

@pmathew92 It turns out that the original guidance for mocking JS interop types is a bit outdated. I managed to fix a first test case, by taking the existing interop tests from the camera_web package as an example (the first-party camera plugin). I'll be updating the other tests today and then I'll push that to this PR.

navaronbracke avatar Mar 05 '25 11:03 navaronbracke

Hi @navaronbracke , i have already fixed the tests when merging the WASM PR . Could you take the latest pull and try again or are you observing any new error?

pmathew92 avatar Mar 05 '25 11:03 pmathew92

I am referring to the web tests when running with JS interop extension types, not any of the native tests (iOS or MacOS).

navaronbracke avatar Mar 05 '25 11:03 navaronbracke

Yes , i have fixed them too. Please check the latest master @navaronbracke

pmathew92 avatar Mar 05 '25 12:03 pmathew92

@pmathew92 I cannot seem to update the mocks locally? None of the existing mocks actually mock extension types for the web tests, so I think that this just doesn't work for the web yet?

navaronbracke@MacBook-Pro-van-Navaron auth0_flutter % flutter pub run build_runner build --delete-conflicting-outputs
Deprecated. Use `dart run` instead.
Building package executable... (4.4s)
Built build_runner:build_runner.
[INFO] Generating build script completed, took 426ms
[INFO] Precompiling build script... completed, took 4.1s
[INFO] Building new asset graph completed, took 1.4s
[INFO] Checking for unexpected pre-existing outputs. completed, took 2ms
[INFO] Generating SDK summary completed, took 6.6s
[SEVERE] mockito:mockBuilder on test/web/auth0_flutter_web_test.dart:

Bad state: Asset URI is missing for abstract class JSObject
package:mockito/src/builder.dart 2223:10                     _MockClassInfo._typeImport
package:mockito/src/builder.dart 1695:21                     _MockClassInfo._addFakeClass.<fn>.<fn>.<fn>
package:code_builder/src/specs/type_reference.g.dart 164:33  _$TypeReferenceBuilder.update
package:code_builder/src/specs/type_reference.g.dart 22:36   new _$TypeReference
package:mockito/src/builder.dart 1692:33                     _MockClassInfo._addFakeClass.<fn>.<fn>
package:mockito/src/builder.dart 2101:24                     _MockClassInfo._withTypeParameters
package:mockito/src/builder.dart 1689:7                      _MockClassInfo._addFakeClass.<fn>
package:code_builder/src/specs/class.g.dart 345:33           _$ClassBuilder.update
package:code_builder/src/specs/class.g.dart 40:28            new _$Class
package:mockito/src/builder.dart 1682:37                     _MockClassInfo._addFakeClass
package:mockito/src/builder.dart 1641:7                      _MockClassInfo._dummyFakedValue
package:mockito/src/builder.dart 1663:27                     _MockClassInfo._dummyValueImplementing
package:mockito/src/builder.dart 1568:12                     _MockClassInfo._dummyValue
package:mockito/src/builder.dart 1671:11                     _MockClassInfo._dummyValueImplementing
package:mockito/src/builder.dart 1568:12                     _MockClassInfo._dummyValue
package:mockito/src/builder.dart 1972:24                     _MockClassInfo._buildOverridingGetter
package:mockito/src/builder.dart 1236:36                     _MockClassInfo.fieldOverrides.<fn>
package:code_builder/src/specs/method.g.dart 323:33          _$MethodBuilder.update
package:code_builder/src/specs/method.g.dart 38:29           new _$Method
package:mockito/src/builder.dart 1236:15                     _MockClassInfo.fieldOverrides
dart:core                                                    List.addAll
package:built_collection/src/list/list_builder.dart 98:14    ListBuilder.addAll
package:mockito/src/builder.dart 1194:28                     _MockClassInfo._buildMockClass.<fn>.<fn>
package:mockito/src/builder.dart 2101:24                     _MockClassInfo._withTypeParameters
package:mockito/src/builder.dart 1148:7                      _MockClassInfo._buildMockClass.<fn>
package:code_builder/src/specs/class.g.dart 345:33           _$ClassBuilder.update
package:code_builder/src/specs/class.g.dart 40:28            new _$Class
package:mockito/src/builder.dart 1126:12                     _MockClassInfo._buildMockClass
package:mockito/src/builder.dart 1072:9                      new _MockLibraryInfo
package:mockito/src/builder.dart 94:29                       MockBuilder.build

[INFO] Running build completed, took 23.5s
[INFO] Caching finalized dependency graph completed, took 81ms
[SEVERE] Failed after 23.6s
Failed to update packages.

I am running on Dart 3.7.0 / Flutter 3.29.0 (regardless of the version in the pubspec, which is lower)

navaronbracke avatar Mar 05 '25 12:03 navaronbracke

@pmathew92 Did you run the web tests with flutter test test/web/auth0_flutter_web_test.dart --platform=chrome when implementing WASM ? Because that doesn't work without updated mocks.

navaronbracke avatar Mar 05 '25 12:03 navaronbracke

@navaronbracke I ran flutter test --tags browser --platform chrome within the auth0_flutter module and it passed all the tests

pmathew92 avatar Mar 05 '25 12:03 pmathew92

I did manage to fix the mocks locally, seems to be an issue with build_runner / mockito. Had to do it manually though. I still have a different issue now, but I'll get that sorted also. Once the tests pass on my end I will push an update here.

navaronbracke avatar Mar 05 '25 12:03 navaronbracke

Tests passed locally now. Screenshot 2025-03-05 at 14 08 11

navaronbracke avatar Mar 05 '25 13:03 navaronbracke

@pmathew92 PTAL. I updated the JS interop to call dartify/jsify directly and moved the polyfill for JSArray to the correct file.

As for the appState argument, I think that that is working as intended? Named arguments can be written anywhere in the argument list of a function call. (as opposed to positional arguments, which do have a fixed order). And since the appState is nullable users can just omit the argument if it is not needed.

For the switch to Dart 3.6.0, that is up to you, as this would mean that users that are on Flutter 3.24 (for WASM support) will need to migrate to Flutter 3.27 for this change.

navaronbracke avatar Mar 06 '25 13:03 navaronbracke

Thankyou @navaronbracke

pmathew92 avatar Mar 07 '25 10:03 pmathew92

Any ideas why the code coverage check is failing? The indirect diff seems to be unrelated to this PR?

navaronbracke avatar Mar 07 '25 13:03 navaronbracke

Thanks @navaronbracke! Appreciate the patience, and the thorough PR.

Widcket avatar Apr 15 '25 21:04 Widcket

Thanks for getting this landed!

navaronbracke avatar Apr 15 '25 22:04 navaronbracke