Edgewall Software
Modify

Opened 5 years ago

Closed 4 years ago

Last modified 2 years ago

#8486 closed enhancement (fixed)

make trac.web.auth.LoginModule cookie path configurable

Reported by: jhammel@… Owned by: jhammel@…
Priority: normal Milestone: 0.12
Component: web frontend Version: 0.11.5rc2
Severity: normal Keywords:
Cc:
Release Notes:
API Changes:

Description

Currently, the LoginModule sets (and deletes) a cookie on req.base_path:

        req.outcookie['trac_auth'] = cookie
        req.outcookie['trac_auth']['path'] = req.base_path or '/'
    def _expire_cookie(self, req):
        """Instruct the user agent to drop the auth cookie by setting the
        "expires" property to a date in the past.
        """
        req.outcookie['trac_auth'] = ''
        req.outcookie['trac_auth']['path'] = req.base_path or '/'
        req.outcookie['trac_auth']['expires'] = -10000
        if self.env.secure_cookies:
            req.outcookie['trac_auth']['secure'] = True

It would be nice for this to be configurable, so that, optionally the cookie path could be specified as an Option with the current behavior being the fallback if the Option is not set.

The reason I would like this is so that cookies can be parseable via multiple Trac environments on the same server, optionally. Currently, (in the soon to be released SharedCookieAuthPlugin?) I work around this with a monkey-patch:

class GenericObject(object):
    def __init__(self, **kw):
        for key, item in kw.items():
            setattr(self, key, item)

def _do_login(self, req):
    kw = [ 'incookie', 'remote_user', 'authname', 
           'remote_addr', 'outcookie' ]
    kw = dict([ (i, getattr(req, i)) for i in kw ])
    kw['base_path'] = '/'
    fake_req = GenericObject(**kw)
    auth_login_module_do_login(self, fake_req)

Attachments (2)

login_cookie_path.r8732.diff (1.5 KB) - added by Jeff Hammel <jhammel@…> 4 years ago.
patch against 0.11-stable
login_cookie_path.r8885.diff (1.5 KB) - added by jhammel@… 4 years ago.
revised patch vs 0.11-stable

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: Changed 5 years ago by rblank

I'm not sure I understand the use case. Isn't the cookie a randomly-generated identifier that only makes sense in the context of a specific Trac instance?

comment:2 in reply to: ↑ 1 Changed 5 years ago by anonymous

Replying to rblank:

I'm not sure I understand the use case. Isn't the cookie a randomly-generated identifier that only makes sense in the context of a specific Trac instance?

see http://trac-hacks.org/wiki/SharedCookieAuthPlugin

There are two basic cases of interest (probably more, but let's start with two):

  1. cookies are associated with a single environment; that is, you want different auth for different Trac instances
  1. cookies are readable across environments on the same server; that is, if you log into trac.example.com/foo you are also logged into trac.example.com/bar (single-sign on)

th:SharedCookieAuthPlugin is a very hacky way of doing this. On the other hand, it works, consumes existing code, and should be secure. I welcome better SSO solutions that are easily deployable, pluggable, and exist within the Trac framework and don't rely on other (usually less deployable) software.

comment:3 Changed 5 years ago by ebray

+1 to this. We use a single-sign on auth system for Trac, but still have to modify trac.web.auth.LoginModule? to set the cookie path to '/' so that it can work without fuss with other plugins that use it, such as AccountManager.

comment:4 follow-up: Changed 4 years ago by cboos

  • Component changed from general to web frontend
  • Milestone set to 1.0

Patch welcomed, then.

Changed 4 years ago by Jeff Hammel <jhammel@…>

patch against 0.11-stable

comment:5 in reply to: ↑ 4 Changed 4 years ago by Jeff Hammel <jhammel@…>

comment:6 Changed 4 years ago by cboos

  • Milestone changed from 1.0 to 0.12

Thanks!

Two minor nits:

  • instead of adding: from trac.config import Option , modify the previous import, add Option after BoolOption
  • more important, the docstring is a bit scarce; without being too long, it should nevertheless give a hint about why someone should ever want to change the default.

comment:7 follow-up: Changed 4 years ago by cboos

  • Milestone changed from 0.12 to next-minor-0.12.x

Well, feel free to update the patch as requested.

Changed 4 years ago by jhammel@…

revised patch vs 0.11-stable

comment:8 in reply to: ↑ 7 Changed 4 years ago by anonymous

Replying to cboos:

Well, feel free to update the patch as requested.

Done, http://trac.edgewall.org/attachment/ticket/8486/login_cookie_path.r8885.diff

If the docstring isn't adequate, suggestions welcome

comment:9 Changed 4 years ago by rblank

  • Milestone changed from next-minor-0.12.x to 0.12
  • Owner set to rblank

comment:10 Changed 4 years ago by rblank

  • Resolution set to fixed
  • Status changed from new to closed

Slightly adapted patch applied in [9226], and named the option auth_cookie_path to be consistent with auth_cookie_lifetime.

comment:11 Changed 4 years ago by rblank

  • Owner changed from rblank to jhammel@…

comment:12 Changed 2 years ago by shoffmann

I've been unsuccessful in seeking to resolve an issue rendering th:SharedCookieAuthPlugin useless in recent Trac applications (Trac 0.13dev-r10883 tested, but should apply at least down to Trac 0.12 as well).

After many hours of code studies and testing I've dropped that approach and created an alternative solution instead, on-top of th:AccountManagerPlugin. This effort is tracked in th:#9676 now. Any feedback welcome.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed The owner will remain jhammel@….
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from jhammel@… to the specified user.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.