Skip to content

Add autoGenerateState flag to EndSessionRequest to control whether to auto-generate state parameter#647

Closed
pcba-dev wants to merge 1 commit into
MaikuB:masterfrom
SniffersDev:allow-explicitly-null-state-in-endsessionrequest
Closed

Add autoGenerateState flag to EndSessionRequest to control whether to auto-generate state parameter#647
pcba-dev wants to merge 1 commit into
MaikuB:masterfrom
SniffersDev:allow-explicitly-null-state-in-endsessionrequest

Conversation

@pcba-dev
Copy link
Copy Markdown

Problem

Some Identity Providers reject the end-session request because AppAuth (Android, iOS, macOS) automatically generates a random state value by default. When those IdPs do not echo the state back, response validation fails and the logout flow breaks.

`state` `autoGenerateState` Behaviour
null true (default) AppAuth auto-generates stateunchanged
"value" true Explicit non-empty "value" used as stateunchanged
"value" false Explicit non-empty "value" used as stateunchanged
null false State suppressed — no state sent to IdP → state suppressed

Fixes #646

Copy link
Copy Markdown
Owner

@MaikuB MaikuB left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Presumably, the addition of the autoGenerateState was done to minimise breaking changes. However, it looks it creates confusion in the semantics and requires a table like the one you provided to understand. For example, when autoGenerateState is true, this will not actually mean an automatically generated state value is used if an explicit state value was given. IMO, the root cause here is because there's now two parameters that dictate the behaviour. The idea that comes to mind for me to solve this is to use inheritance like so

abstract class State {
  String? get value;
}

class NullState implements State {
  @override
  String? get value => null;
}

class RandomState implements State {

  @override
  String? get value => ...
}

class CustomState implements State {
  final String _value;

  CustomState(this._value);

  @override
  String? get value => _value;
}

There may be better names for these classes but this would be how these are used

  • NullState: follows a null object pattern approach. The plugin interprets to nullify the default random state so the IdP gets a null state value
  • RandomState: plugin ensures a random state value is used to then go to the IdP. The calculation of the random state value could end up being lifted up to be done by this plugin in Dart instead of the underlying SDKs as well
  • CustomState: allows users of the plugin to explicitly provide a state value that is passed to the SDKs and then go to the IdP

This would require breaking changes but this would result in code that is easier to interpret/understand and could also be a pattern applied to other parts of the plugin (e.g. could be applied to the auth requests for state and nonce). Let me know your thoughts and if this something you could tackle in this PR

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow explicitly null state in EndSessionRequest

2 participants