Internal error when switching the session ID to an existing ID

[Remy Blank]

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

[Remy Blank]

There's a misplaced parenthesis at line 205:

trac/web/session.py

@@ -202 +202 @@
                 raise TracError(Markup(
                     _("Session '%(id)s' already exists.<br />"
                       "Please choose a different session ID.",
-                      id=new_sid), _("Error renaming session")))
+                      id=new_sid)), _("Error renaming session"))
             self.env.log.debug('Changing session ID %s to %s', self.sid,
                                new_sid)
             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.

[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

[Remy Blank]

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

[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.

[Remy Blank]

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

@@ -104 +104 @@
                 attrs = [(self.sid, authenticated, k, v) 
                          for k, v in self.items()]
                 cursor.execute("""
-                    DELETE FROM session_attribute WHERE sid=%s
-                    """, (self.sid,))
+                    DELETE FROM session_attribute
+                    WHERE sid=%s AND authenticated=%s
+                    """, (self.sid, authenticated))
                 self._old = dict(self.items())
                 if attrs:
                     cursor.executemany("""

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

[Remy Blank]

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

[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.

[Remy Blank]

Optionally disable automatic form + buttons on preference panels.

[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?

[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.

[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')).

[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.

[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).

[Remy Blank]

Avoid carrying over attributes when loading a session.

[Remy Blank]

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