MaikuB / flutter_appauth

A Flutter wrapper for AppAuth iOS and Android SDKs
270 stars 243 forks source link

[flutter_appauth] nonce parameter fix #345

Closed nvx closed 2 years ago

nvx commented 2 years ago

This is a rebase of PR #329 against current master and moves the nonce reading out of additional parameters to use the parameters added in PR #331

biscottis commented 2 years ago

While you're at it, you might as well also update flutter_appauth_platform_interface/lib/src/authorization_request.dart to have nonce as AuthorizationTokenRequest has it

import 'authorization_parameters.dart';
import 'authorization_service_configuration.dart';
import 'common_request_details.dart';

/// The details of an authorization request to get an authorization code.
class AuthorizationRequest extends CommonRequestDetails with AuthorizationParameters {
  AuthorizationRequest(
    String clientId,
    String redirectUrl, {
    String? issuer,
    String? discoveryUrl,
    AuthorizationServiceConfiguration? serviceConfiguration,
    String? loginHint,
    List<String>? scopes,
    Map<String, String>? additionalParameters,
    List<String>? promptValues,
    bool allowInsecureConnections = false,
    bool preferEphemeralSession = false,
    String? responseMode,
    String? nonce,
  }) {
    this.clientId = clientId;
    this.redirectUrl = redirectUrl;
    this.scopes = scopes;
    this.serviceConfiguration = serviceConfiguration;
    this.additionalParameters = additionalParameters;
    this.issuer = issuer;
    this.discoveryUrl = discoveryUrl;
    this.loginHint = loginHint;
    this.promptValues = promptValues;
    this.allowInsecureConnections = allowInsecureConnections;
    this.preferEphemeralSession = preferEphemeralSession;
    this.responseMode = responseMode;
    this.nonce = nonce;
    assertConfigurationInfo();
  }
}
nvx commented 2 years ago

Good catches. I've made those changes you suggested.

MaikuB commented 2 years ago

FYI that after the latest commit that there still seems to be a compilation error. You should be able to use the checks shown above to drill further to help find the problem

nvx commented 2 years ago

macOS should be good now. I can squash the last few fix commits down if you prefer prior to merging.

MaikuB commented 2 years ago

Don't worry about squashing as that will happen prior to merging

MaikuB commented 2 years ago

Sorry just had another thought, do you think it would be possible to update the example app to demonstrate how the nonce could be used? These could be separate buttons in the example so it doesn't affect the existing scenarios already covered

nvx commented 2 years ago

I tried to update the example app to pass in a nonce and it looks like the nonce value isn't being parsed on iOS/macOS as the method as the processArguments hasn't been updated. Are you able to fix this up?

Good catch, although strangely on iOS when I was testing it seemed to be parsing the nonce fine (Firebase auth requires a specific nonce value to be used or it fails the auth which is what lead me to this fix in the first place, so I know it was definitely working) - but in hindsight I'm super confused how it is working. Will get that fixed up now though.

MaikuB commented 2 years ago

The code in the PR does fallback to generating a nonce so that's probably what was happening there. I noticed the issue when the nonce in the response didn't match what was specified in the request

nvx commented 2 years ago

The code in the PR does fallback to generating a nonce so that's probably what was happening there. I noticed the issue when the nonce in the response didn't match what was specified in the request

nah as in for firebase auth you need the nonce to be sha256(somevalue), then provide somevalue to firebase along with the id token, if the nonce in the id token doesn't match the sha256 hash of the other parameter you pass it doesn't let you login. Either way it's added to the processArguments now while I continue to scratch my head how it was working for me.