Closed Bug 553131 Opened 14 years ago Closed 14 years ago

[Kitsune] Add Tiki session detection

Categories

(support.mozilla.org :: General, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: paulc, Assigned: paulc)

Details

Attachments

(2 files, 2 obsolete files)

For the sidebar menu, and probably display the advanced search link, we need to detect whether to user is logged in or not. This involves looking for the cookie set by Tiki on login.
Talked to Dave about how we could do it, and the AMO implementation. AMO has cake middleware that checks user auth state from the cake user table and, for first timers, copies over the data to a zamboni table.

We can do something similar, have a temporary tiki user model that creates a django user object as they do. This is useful, since we'll need to do that anyway in for the next planned milestone (forums).

FTR, the cookie used by SUMO is called 'SUMOloggedin'.
(In reply to comment #1)
> Talked to Dave about how we could do it, and the AMO implementation. AMO has
> cake middleware that checks user auth state from the cake user table and, for
> first timers, copies over the data to a zamboni table.
> 
> We can do something similar, have a temporary tiki user model that creates a
> django user object as they do. This is useful, since we'll need to do that
> anyway in for the next planned milestone (forums).

I definitely support following AMO's lead here. Their solution isn't perfect but it's very workable.
 
> FTR, the cookie used by SUMO is called 'SUMOloggedin'.

The cookie is actually called SUMOv1, iirc, and that name is configurable in the admin area. SUMOloggedin is not a secure cookie, and is only used to redirect (probably) logged-in users from HTTP back to HTTPS so the real session cookie (SUMOv1) is sent and people don't find themselves "suddenly logged out."
(In reply to comment #2)
> The cookie is actually called SUMOv1, iirc, and that name is configurable in
> the admin area. SUMOloggedin is not a secure cookie, and is only used to
> redirect (probably) logged-in users from HTTP back to HTTPS so the real session
> cookie (SUMOv1) is sent and people don't find themselves "suddenly logged out."
You're right. I should have been more specific -- SUMOlogged in is the cookie indicating whether the user is logged in or not, that's what I meant. SUMOv1 is the one to check for actual user data, iirc too.
(In reply to comment #3)
> You're right. I should have been more specific -- SUMOlogged in is the cookie
> indicating whether the user is logged in or not, that's what I meant. SUMOv1 is
> the one to check for actual user data, iirc too.

We should not rely on SUMOloggedin at all for checking that a user is logged in, because the behavior will be different and confusing: since it's not a secure cookie, a user could appear logged in on certain HTTP URLs, but not on others. (Ignoring the HTTP->HTTPS redirect for a moment.)
I'll pick up the TikiWiki half of this. (Logout hook to delete the Django session.)
Digging around I learned two things:

0) Almost all of the Tiki session cleanup was commented out in r11193 as part of an attempt to fix bug 415306. The fix didn't work, and Laura eventually got it in r17373. This should have been reverted at the time, but wasn't, so now I'm reverting it, and making Tiki clean up after itself.

1) PHP doesn't delete its session cookies when you call session_destroy(). (Not hugely relevant, but interesting!)

This patch thus does 2^M3^M4^Mseveral things:

0) Revert r11193. Make Tiki delete session data on log out. (If this is a performance regression, it's not critical and can be dropped--I didn't notice anything locally but obviously tiki_session and session are empty locally. I'll ask timellis/justdave and let them bikeshed over it.)

1) Check for the Django session cookie on logout and, if it exists:
1.1) Delete the corresponding row from the Django session table.
1.2) Delete the cookie.

2) Delete the PHP session cookie on logout.

3) Fixes the indent level of userslib->user_logout()
Attachment #437993 - Flags: review?(paulc)
After talking to tellis I un-reverted the commenting-out. Uh....

The DELETE statements are commented out again.
Attachment #437993 - Attachment is obsolete: true
Attachment #437993 - Flags: review?(paulc)
Attachment #438008 - Flags: review?(paulc)
Simpler: all it does is delete the cookies, and leaves session data for the garbage collectors.
Attachment #438008 - Attachment is obsolete: true
Attachment #438241 - Flags: review?(paulc)
Attachment #438008 - Flags: review?(paulc)
Comment on attachment 438241 [details] [diff] [review]
just deletes cookies.

WFM. The two cookies were cleared: SUMOv1 and sessionid.
Attachment #438241 - Flags: review?(paulc) → review+
Priority: -- → P1
Severity: normal → blocker
Fixed more things, sent another pull request, same url as above.
This makes sure a session is created in tiki_sessions before sending control back to Kitsune.

James: I've made no code changes to the Kitsune side. This patch should fix the problem you were experience where redirecting back to search wasn't showing you're logged in.
Attachment #439362 - Flags: review?(james)
The deleting-cookies patch is in. r66013 in trunk. Looking at the other one now.
Comment on attachment 439362 [details] [diff] [review]
update tiki_sessions on log in

OK, I saw some weirdness but I think it's actually a bug in the Web Developer Toolbar. This seems good.

Still looking at the branch in github, but it's working so that's a good sign.
Attachment #439362 - Flags: review?(james) → review+
http://github.com/pcraciunoiu/kitsune/compare/development...bug-553131
Another r? for the minor fixes you mentioned.

r66046 on trunk for the tiki_sessions fix in attachment 439362 [details] [diff] [review].
http://github.com/jsocol/kitsune/commit/eccf2e71205500d3d7ca62cf895b038f3f8fb9e6

Done!
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Is https://support-stage-new.mozilla.com/en-US/search separate?  If I log in via https://support-stage-new.mozilla.com/en-US/kb/, my logged-in state is correctly reflected there, but going to https://support-stage-new.mozilla.com/en-US/search while logged in, I can't get the advanced search UI to appear, as it still appears I'm logged out.
(In reply to comment #17)
> Is https://support-stage-new.mozilla.com/en-US/search separate?

Well it's running on Kitsune, yes. Nothing else is, yet.

> If I log in
> via https://support-stage-new.mozilla.com/en-US/kb/, my logged-in state is
> correctly reflected there, but going to
> https://support-stage-new.mozilla.com/en-US/search while logged in, I can't get
> the advanced search UI to appear, as it still appears I'm logged out.

Can you post more detailed STR? (It's possible support-stage-new might not be picking up updates to TikiWiki from SVN.)
(In reply to comment #18)

> Can you post more detailed STR? (It's possible support-stage-new might not be
> picking up updates to TikiWiki from SVN.)

It was that, pretty sure; comment 17 is fine, now, and I've done additional session/login/logout testing, and haven't found anything awry.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: