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

Avoid session invalidation when changing session lifetime. #546

Merged
merged 7 commits into from
Jun 10, 2022

Conversation

colinmollenhour
Copy link
Member

This PR fixes a few session related issues:

  • Failed session validation when the lifetime is updated (store renew timestamp and lifetime in separate keys)
  • Optimizes checking the password timestamp (load just the timestamp value instead of the entire customer model)
  • Fix call to protected method in Varien_Date::toTimestamp
  • Adds session_before_renew_cookie event to allow session lifetime to be more dynamic (e.g. I use this event to make sessions for customers with items in their cart much longer than those without).

Flyingmana
Flyingmana previously approved these changes Oct 4, 2018
Copy link
Member

@Flyingmana Flyingmana left a comment

Choose a reason for hiding this comment

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

Iam a bit sceptic about removing the constant, and that it could invalidate all sessions when this change gets deployed on a running system.
But besides the migration path Iam ok with it.

@colinmollenhour
Copy link
Member Author

The validation code uses isset($sessionData[...]) so validation will be skipped on sessions that were created before the update.

if (strpos($currentCookieDomain, $host) > 0) {
$cookie->delete($this->getSessionName(), null, $host);
}
$secureCookieName = session_name() . '_cid';
Copy link
Collaborator

@tbaden tbaden Jun 1, 2019

Choose a reason for hiding this comment

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

$sessionName is passed anyway, why switching to method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because $sessionName can be null. I suppose it could be $this->getSessionName() but it would be functionally equivalent.

@tbaden tbaden added the review needed Problem should be verified label Jun 1, 2019
tmotyl
tmotyl previously requested changes Jun 6, 2019
Copy link
Contributor

@tmotyl tmotyl left a comment

Choose a reason for hiding this comment

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

please leave the constant to avoid breaking change

@colinmollenhour
Copy link
Member Author

please leave the constant to avoid breaking change

I added the constant back in.

@colinmollenhour colinmollenhour dismissed tmotyl’s stale review September 24, 2019 14:53

Addressed as requested.

@colinmollenhour
Copy link
Member Author

I merged the latest and cleaned up the logic around the secure cookie check to make it easier to read and understand without changing the functionality. These changes are good improvements IMO and no BC breakage. Please review! Thanks!

@sreichel sreichel added enhancement Component: Core Relates to Mage_Core Component: Customer Relates to Mage_Customer labels Jun 5, 2020
@fballiano
Copy link
Contributor

@colinmollenhour could you address the conflicts here? I was checkin them but I don't trust my knowledge of this specific subject.

Adel-Magebinary
Adel-Magebinary previously approved these changes Jun 9, 2022
Copy link
Collaborator

@Adel-Magebinary Adel-Magebinary left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@colinmollenhour
Copy link
Member Author

I resolved the conflicts.

@fballiano
Copy link
Contributor

phpstan doesn't let anything slip :-D

@fballiano fballiano merged commit 447e27e into OpenMage:1.9.4.x Jun 10, 2022
@github-actions
Copy link

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 447e27e. ± Comparison against base commit c83207b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Relates to Mage_Core Component: Customer Relates to Mage_Customer enhancement review needed Problem should be verified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants