Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

#9658 closed defect (fixed)

Internal error when switching the session ID to an existing ID

Reported by: Remy Blank Owned by: Remy Blank
Priority: normal Milestone: 0.12.2
Component: web frontend Version: 0.12-stable
Severity: normal Keywords:
Cc: doki_pen@…
Release Notes:

Fixed various corner cases in session handling.

API Changes:

Description

Trying to change the session ID of an anonymous session to an existing ID, for example an existing authenticated user, results in the following traceback:

Traceback (most recent call last):
  File "/home/joe/src/trac/0.12-stable/trac/web/main.py", line 511, in _dispatch_request
    dispatcher.dispatch(req)
  File "/home/joe/src/trac/0.12-stable/trac/web/main.py", line 237, in dispatch
    resp = chosen_handler.process_request(req)
  File "/home/joe/src/trac/0.12-stable/trac/prefs/web_ui.py", line 77, in process_request
    template, data = chosen_provider.render_preference_panel(req, panel_id)
  File "/home/joe/src/trac/0.12-stable/trac/prefs/web_ui.py", line 99, in render_preference_panel
    self._do_save(req)
  File "/home/joe/src/trac/0.12-stable/trac/prefs/web_ui.py", line 135, in _do_save
    req.session.change_sid(val)
  File "/home/joe/src/trac/0.12-stable/trac/web/session.py", line 197, in change_sid
    @self.env.with_transaction()
  File "/home/joe/src/trac/0.12-stable/trac/db/api.py", line 77, in transaction_wrapper
    fn(ldb)
  File "/home/joe/src/trac/0.12-stable/trac/web/session.py", line 205, in update_session_id
    id=new_sid), _("Error renaming session")))
TypeError: decoding Unicode is not supported

Attachments (2)

9658-pref-forms-r10251.patch (3.1 KB ) - added by Remy Blank 8 years ago.
Optionally disable automatic form + buttons on preference panels.
9658-load-empty-r10259.patch (1.8 KB ) - added by Remy Blank 8 years ago.
Avoid carrying over attributes when loading a session.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by Remy Blank

There's a misplaced parenthesis at line 205:

  • trac/web/session.py

    diff --git a/trac/web/session.py b/trac/web/session.py
    a b class Session(DetachedSession):  
    202202                raise TracError(Markup(
    203203                    _("Session '%(id)s' already exists.<br />"
    204204                      "Please choose a different session ID.",
    205                       id=new_sid), _("Error renaming session")))
     205                      id=new_sid)), _("Error renaming session"))
    206206            self.env.log.debug('Changing session ID %s to %s', self.sid,
    207207                               new_sid)
    208208            cursor.execute("""

This patch fixes the issue, and I now get a normal error. However, the markup contained in the error message is escaped. This seems to be the only location where Markup is used in an exception, other locations use tag(). So this should probably be converted as well.

comment:2 Changed 8 years ago by Remy Blank

There seems to be another issue. When trying to set the session ID to an existing session last logged in more than UPDATE_INTERVAL (1 day), we get an assertion error:

Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/trac/web/main.py", line 511, in _dispatch_request
    dispatcher.dispatch(req)
  File "build/bdist.linux-x86_64/egg/trac/web/main.py", line 237, in dispatch
    resp = chosen_handler.process_request(req)
  File "build/bdist.linux-x86_64/egg/trac/prefs/web_ui.py", line 77, in process_request
    template, data = chosen_provider.render_preference_panel(req, panel_id)
  File "build/bdist.linux-x86_64/egg/trac/prefs/web_ui.py", line 97, in render_preference_panel
    self._do_load(req)
  File "build/bdist.linux-x86_64/egg/trac/prefs/web_ui.py", line 148, in _do_load
    req.session.get_session(oldsid)
  File "build/bdist.linux-x86_64/egg/trac/web/session.py", line 189, in get_session
    self.bake_cookie()
  File "build/bdist.linux-x86_64/egg/trac/web/session.py", line 170, in bake_cookie
    assert self.sid, 'Session ID not set'
AssertionError: Session ID not set

comment:3 Changed 8 years ago by Remy Blank

Patch from comment:1 applied in [10161]. The rest will have to wait for 0.12.2.

comment:4 Changed 8 years ago by Remy Blank

doki_pen posted a procedure to reproduce the issue on trac-dev:

I have setup a virtualenv with Babel and Trac-0.12.

I create a trac environment with all default options.

I have created an htpasswd file with root:$encryptedpassword. I start trac like this:

tracd -p 7421 . -sr --basic-auth *,htpasswd,*

Login as root and set email. Logout. As anonymous, go to advanced prefs and set sid to "root". Set email. Login as root.. exception.

It may have something to do with basic auth and the browser caches basic auth. I'm on Chrome. I haven't tried logging into root from a different browser. If I loging as someone else, and then switch to root, things seem to be fine.

comment:5 Changed 8 years ago by Remy Blank

Cc: doki_pen@… added

I have tried the exact sequence of comment:4, but I always get an error "Session 'root' already exists. Please choose a different session ID." when trying to set the sid to "root". So from my testing, this is a "worksforme".

There is however an issue that I consider quite serious: instead of setting the sid to "root", if I try to restore the session "root" as anonymous, then set an e-mail address, all session attributes of the authenticated user "root" are removed!

The following patch fixes the issue:

  • trac/web/session.py

    diff --git a/trac/web/session.py b/trac/web/session.py
    a b class DetachedSession(dict):  
    104104                attrs = [(self.sid, authenticated, k, v)
    105105                         for k, v in self.items()]
    106106                cursor.execute("""
    107                     DELETE FROM session_attribute WHERE sid=%s
    108                     """, (self.sid,))
     107                    DELETE FROM session_attribute
     108                    WHERE sid=%s AND authenticated=%s
     109                    """, (self.sid, authenticated))
    109110                self._old = dict(self.items())
    110111                if attrs:
    111112                    cursor.executemany("""

I'm looking into the issue from comment:2 now.

comment:6 Changed 8 years ago by Remy Blank

Patch from comment:5 applied in [10234], together with a test case. It also fixes the error message mentioned in comment:1.

comment:7 Changed 8 years ago by Remy Blank

Refactored and improved the session admin commands in [10248]. In particular, it is now possible to consistently operate on both unauthenticated and authenticated sessions.

AFAICT, the whole session subsystem now correctly supports unauthenticated and authenticated sessions with the same name. The only remaining issue is comment:2.

Changed 8 years ago by Remy Blank

Optionally disable automatic form + buttons on preference panels.

comment:8 Changed 8 years ago by Remy Blank

The error in comment:2 is actually a "user error". It happens when entering a new session ID in the "Session key" field of the "Advanced" preference panel, and validating with ENTER. This triggers the "Load" button instead of the "Save Changes" button, as it is the first button in the form, and hence tries to load a session with an empty name.

The error can be fixed easily, but having the wrong button trigger is a bit of a usability issue. This is due to the "hardcoded" structure of preference panels, which are supposed to have a single form. The whole preference subsystem feels a bit hackish, so I don't mind adding some more hackery to allow individual panels not to have a single form with a "Save Changes" button, but to define their own forms and buttons.

9658-pref-forms-r10251.patch implements this only adding the form and buttons if the <body> of the panel doesn't have the class noform.

Thoughts? Too hackish?

comment:9 Changed 8 years ago by Christian Boos

Two id="userprefs" in prefs_advanced.html? Simple typo or needed for the styles? In the latter case, some changes in the CSS are needed as well.

Otherwise I'm fine with the change, it seems better to have all the information about the presence of multiple forms in the panel html rather than having to add a specific flag in the data. We could even use the select() directly for detecting if there are multiple forms, something like: len(list(select('form//input[@type="submit"]/@type'))) > 1.

comment:10 in reply to:  9 ; Changed 8 years ago by Remy Blank

Replying to cboos:

Two id="userprefs" in prefs_advanced.html? Simple typo or needed for the styles?

No, that's a copy/paste error.

We could even use the select() directly for detecting if there are multiple forms, something like: len(list(select('form//input[@type="submit"]/@type'))) > 1.

Interesting, the "marker class" is actually what I found hackish, so I'm all for removing it. That expression counts the submit buttons, though, and there could be several buttons for a form. How about counting the forms instead? The strategy could be to add the form and buttons if no form is defined in the panel, something like: len(list(select('form'))) > 0 or even simpler list(select('form')).

comment:11 in reply to:  10 Changed 8 years ago by Christian Boos

Replying to rblank:

… The strategy could be to add the form and buttons if no form is defined in the panel

Ah yes, that's better even. My select above was the best approximation I could find for counting the forms and I wrongly assumed more than one was needed, but as it's actually at least one, the test can be simplified further.

comment:12 Changed 8 years ago by Remy Blank

Applied an improved patch in [10259] (note to self: XPath is not CSS).

I have found one more issue with the session code, where session attributes are carried over from an anonymous session to a restored session:

  • Create an anonymous session and add attributes (e.g. full name).
  • In the "Advanced" preference panel, load a non-existing session.
  • The attributes set in the initial session are stored in the session_attribute table for the new sid. However, as the new session didn't exist, no entry is added to the session table.

This tends to fill the sesion_attribute table with unnecessary entries, and at some point I even got a "Primary key is not unique" exception. 9658-load-empty-r10259.patch fixes the issue (chunks 2 & 3, the others are cosmetic).

Changed 8 years ago by Remy Blank

Avoid carrying over attributes when loading a session.

comment:13 Changed 8 years ago by Remy Blank

Release Notes: modified (diff)
Resolution: fixed
Status: newclosed

Patch applied in [10263], together with a test case demonstrating the issue. This concludes my little excursion into the wonderful land of session management :)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.