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

Fix EZP-21991: UserHash should be cached tied to user session id, not user cookies #720

Merged
merged 1 commit into from Feb 10, 2014

Conversation

lolautruche
Copy link
Contributor

https://jira.ez.no/browse/EZP-21991

This patch makes the UserHash cache key to be only bound to the user session id.

Important note

Session cookie path/domain/lifetime issue is voluntarily not addressed here and will be fixed in a different PR.

@lolautruche
Copy link
Contributor Author

}

/**
* Checks if passed string can be considered as a session name.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add something to the effect of "such as would be used in cookies" ?

@gggeek
Copy link
Contributor

gggeek commented Feb 7, 2014

A bit OT for this PR, but I do not like very much the "isAnonymous" method name, which in fact checks if user has session or not. What if anonymous users do have sessions?

@lolautruche
Copy link
Contributor Author

@gggeek This is indeed OT. Please focus on this PR review 😉

@joaoinacio
Copy link

Just to clarity, this does not attempt to handle the problem with multiple siteaccesses/sessions, correct? (part 2. of the JIRA issue)

I wonder if there is any limit to the cache key length, or if f.e. a checksum could be used instead?

@lolautruche
Copy link
Contributor Author

@joaoinacio Correct.
There is no need to use a checksum as Stash already does it. The advantage of using a clear cache key is that it makes it easier to retrieve the right Stash cache files (it adds the plain text cache key in a comment).

@@ -132,7 +132,7 @@ public function generateUserHash( Request $request )
// We must have a session at that point since we're supposed to be connected, so HTTP_COOKIE must contain session id.
// HTTP_COOKIE header will be used as cache key to store the user hash.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be updated I guess

@dpobel
Copy link
Contributor

dpobel commented Feb 10, 2014

besides the comment thing, +1

@andrerom
Copy link
Contributor

+1

@pspanja
Copy link
Contributor

pspanja commented Feb 10, 2014

Aside from @dpobel's remark, +1

@lolautruche
Copy link
Contributor Author

Updated and rebased.

lolautruche added a commit that referenced this pull request Feb 10, 2014
Fix EZP-21991: UserHash should be cached tied to user session id, not user cookies
@lolautruche lolautruche merged commit 898688f into master Feb 10, 2014
@lolautruche lolautruche deleted the fix_EZP-21991_userHashSessionId branch February 10, 2014 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants