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

[SDK-3984] Support login on web platform #203

Merged
merged 17 commits into from
Feb 23, 2023
Merged

[SDK-3984] Support login on web platform #203

merged 17 commits into from
Feb 23, 2023

Conversation

stevehobbsdev
Copy link
Contributor

@stevehobbsdev stevehobbsdev commented Feb 23, 2023

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

📋 Changes

This PR presents an initial implementation of the Flutter Web with JS interop and Auth0 SPA SDK. It supports:

  • login using loginWithRedirect
  • checkSession
  • Automatic use of handleRedirectCallback on load

Things that are pending in forthcoming PRs:

  • all options for client set up and login (currently only a subset of things like AuthorizeParams are supported until the API matures)
  • test suite (do once the API is stable)
  • support for getting tokens
  • logout

Usage

// inside some initializer func like 'initState':
@override
void initState() {
  super.initState();

  auth0Web =
      Auth0Web(dotenv.env['AUTH0_DOMAIN']!, dotenv.env['AUTH0_CLIENT_ID']!);

  // call to initialize the client and auto-handle callback or check session
  auth0Web.onLoad();
}

// In a login button click handler..
Future<void> webAuthLogin() async {
  String output = '';

  // Redirect the user agent to the login page
  auth0Web.loginWithRedirect(redirectUri: 'http://localhost:3000');
}

This PR also includes a refactoring of WebAuthLoginOptions to create a base type LoginOptions that can also be used by the web platform. The former extends the latter and adds the two mobile platform properties useEphemeralSession and scheme that are not applicable for the web.

🎯 Testing

The example app has been updated to support login when using the Web platform. To run:

flutter run -d web-server --web-port 3000

The example app will use the same domain & client ID as defined in .env.

@stevehobbsdev stevehobbsdev requested a review from a team as a code owner February 23, 2023 16:58
Auth0Web(final String domain, final String clientId)
: _account = Account(domain, clientId);

Future<Credentials?> onLoad() async {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might need a new name - I'm not precious about it - but the point was to introduce some kind of initializer that could perform some async work on startup. This would allow us to create the client, but also do the check for calling handleRedirectCallback or checkSession depending on URL contents on startup.

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Base: 98.33% // Head: 98.07% // Decreases project coverage by -0.27% ⚠️

Coverage data is based on head (f73323c) compared to base (feaf216).
Patch coverage: 76.47% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               beta     #203      +/-   ##
============================================
- Coverage     98.33%   98.07%   -0.27%     
  Complexity       82       82              
============================================
  Files            81       82       +1     
  Lines          1445     1453       +8     
  Branches        320      320              
============================================
+ Hits           1421     1425       +4     
- Misses           12       16       +4     
  Partials         12       12              
Flag Coverage Δ
auth0_flutter 100.00% <ø> (ø)
auth0_flutter_android 96.60% <ø> (ø)
auth0_flutter_ios 100.00% <ø> (ø)
auth0_flutter_platform_interface 95.95% <76.47%> (-1.28%) ⬇️

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

Impacted Files Coverage Δ
auth0_flutter/lib/auth0_flutter.dart 100.00% <ø> (ø)
..._interface/lib/src/auth0_flutter_web_platform.dart 0.00% <0.00%> (ø)
..._interface/lib/src/id_token_validation_config.dart 100.00% <ø> (ø)
...tter_platform_interface/lib/src/login_options.dart 100.00% <100.00%> (ø)
...rface/lib/src/web-auth/web_auth_login_options.dart 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 35 to 50
T stripNulls<T extends Object>(final T obj) {
final keys = objectKeys(obj);
final output = newObject<Object>();

for (var i = 0; i < keys.length; i++) {
final key = keys[i] as String;
final value = getProperty(obj, key) as dynamic;

if (value != null) {
setProperty(output, key, value);
}
}

return output as T;
}
Copy link
Contributor Author

@stevehobbsdev stevehobbsdev Feb 23, 2023

Choose a reason for hiding this comment

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

Some mapping between the keys to strip out values that were null is necessary, otherwise the nulls get sent through the URL to the IdP, which is undesirable. The SPA SDK doesn't strip these out.

This is needed as Dart JS interop treats null and undefined as completely identical. Consider this example:

final String? redirectUri = null;

client.loginWithRedirect(RedirectLoginOptions(
  authorizationParams: AuthorizationParams(
    redirect_uri: redirectUri)));

// redirect_uri is `null` in the URL

Without some mapping function that executes in JS land, an alternative would be to do two different calls depending on whether redirectUri is null or not, which doesn't scale as this applies to all optional parameters.

Or, use Map<string, string> in the API, in which case we lose strong typing but also in JS land this equates to using a Dart Map type, which is different from a plain JS object, so the JS interop API doesn't line up with reality (must use classes annotated with @anonymous and external factory constructors).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, not precious on the name of the function, happy to hear suggestions.

Future<Credentials?> onLoad() async {
await Auth0FlutterWebPlatform.instance.initialize(_account);

// Get credentials here and return them on load?
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the consumer use the result to check if the user is logged in?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, I'd say go for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I suppose the idea would be if the returned Credentials is not null, the user could be considered logged in. The use case I imagined would be if you were to initialize it in initState, the developer could then set up their initial app state accordingly:

@override
void initState() {
  super.initState();

  auth0Web =
      Auth0Web(dotenv.env['AUTH0_DOMAIN']!, dotenv.env['AUTH0_CLIENT_ID']!);

  // call to initialize the client and auto-handle callback or check session
  auth0Web.onLoad().then((creds) {
    if (creds != null) {
      // set "logged in" state here
    }
  });
}

String output = '';

if (kIsWeb) {
auth0Web.loginWithRedirect(redirectUri: 'http://localhost:3000');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have an await, and be all wrapped in the same try/catch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what happens with the errors returned from JS land?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this have an await, and be all wrapped in the same try/catch?

Yes, probably should.

Also, what happens with the errors returned from JS land?

They should get surfaced in the same way. Something to test, though.

@stevehobbsdev stevehobbsdev added the review:medium Medium review label Feb 23, 2023
@Widcket Widcket merged commit 29409e4 into beta Feb 23, 2023
@Widcket Widcket deleted the sdk-3984/spa-js branch February 23, 2023 23:46
This was referenced Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants