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

Improve transient authorization data handling #397

Merged
merged 6 commits into from
Nov 20, 2019

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Nov 14, 2019

Changes

Potential breaking changes:

  • Remove the session_base_name config option for the Auth0 class. Instead, pass the StoreInterface implementing class with the modified name.
  • Remove state_handler config key, StateHandler interface, and SessionStateHandler and DummyStateHandler classes. This makes state handling mandatory for callback routes using Auth0::getUser() or Auth0::exchange(). Guidance for IpD-initiated flows will be provided in an upcoming migration guide.
  • Remove EmptyStore class and the ability to pass false on the store config key. Set persist_user to false in the config key to skip all persistence.
  • Default state handling is now cookies instead of sessions. This should provide more flexibility but may require whitelisting cookie names (auth0__nonce and auth0__state by default) on certain managed hosts.
  • Default state key changed from auth0__webauth_state to auth0__state.
  • Remove unused \Auth0\SDK\Auth0::$URL_MAP

Testing

  • This change adds test coverage
  • This change has been tested on PHP 7.1.29 and 7.2.18

Checklist

  • All existing and new tests complete without errors.
  • The correct base branch is being used - 7.0.0-dev

@joshcanhelp joshcanhelp added this to the 7.0.0 milestone Nov 14, 2019
@joshcanhelp joshcanhelp changed the title Remove session_base_name config and clean up store assignment [WIP] Improve transient authorization data handling Nov 14, 2019
src/Auth0.php Outdated
Comment on lines 307 to 311
$this->store = $config['store'] ?? null;
if ($this->store === false) {
$this->store = new EmptyStore();
} else if (! $this->store instanceof StoreInterface) {
$this->store = new SessionStore();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactor only, no expected functional changes.

Choose a reason for hiding this comment

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

If $config['store'] is null, is the intention for it to fall into the condition on line 300? I'm assuming that the check on 298 would return false in this case. If so, the code isn't very clear here. At first glance I thought that the check on 298 is wrong and it should be $this->store === null, but after looking deeper I just have more questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this. I'll take a step back and look at it again. It's more clear than it was before at least 😆

Comment on lines +314 to +316
$transientStore = $config['transient_store'] ?? null;
if (! $transientStore instanceof StoreInterface) {
$transientStore = new CookieStore();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Name changed from #395

Choose a reason for hiding this comment

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

Just curious - why are we falling back to an EmptyStore above, but not for this transient store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

store is for identity artifacts (user identity, tokens) and transientStore is for required auth transients like nonce and state. Things work fine but are not stored if store is empty but things break if transientStore is.

That said, I'll take another look here along with the above.

@joshcanhelp joshcanhelp changed the title [WIP] Improve transient authorization data handling Improve transient authorization data handling Nov 14, 2019
@joshcanhelp joshcanhelp marked this pull request as ready for review November 15, 2019 22:09
@joshcanhelp joshcanhelp requested a review from a team November 15, 2019 22:09
@joshcanhelp joshcanhelp merged commit 0f938be into 7.0.0-dev Nov 20, 2019
@joshcanhelp joshcanhelp deleted the improve-state-handling branch December 4, 2019 00:42
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants