Edgewall Software
Modify

Opened 15 years ago

Closed 10 years ago

Last modified 10 years ago

#1347 closed enhancement (fixed)

remove old authenticated user sessions

Reported by: dnauck Owned by: John Hampton
Priority: normal Milestone: 0.12
Component: ticket system Version: devel
Severity: normal Keywords: authenticated user sessions review
Cc: shishz@…, webkontakt@…, vyt@…, jeffh@…, pacopablo@… Branch:
Release Notes:
API Changes:

Description

when you're using the restrict_owner setting there is no possibility to delete old users from this dropdown box.

Attachments (5)

WebAdminSessions.patch (2.8 KB ) - added by markus 14 years ago.
WebAdminSessions.2.patch (5.4 KB ) - added by markus 14 years ago.
WebAdminSessions.3.patch (5.5 KB ) - added by markus 14 years ago.
t1347_session_management.patch (33.7 KB ) - added by John Hampton 10 years ago.
Patch for session management in trac-admin
1347-cleanup-r9261.patch (21.8 KB ) - added by Remy Blank 10 years ago.
Cleanup + minor fixes, applies on top of t1347_session_management.patch.

Download all attachments as: .zip

Change History (32)

comment:1 by dna, 14 years ago

Type: defectenhancement

comment:2 by sylvain@…, 14 years ago

Component: generalticket system

In fact, the drop down list in restrict_owner mode, should be better build from the permission list of something which list the user of the site.

My be a new permission priviledge (TICKET_ASSIGNABLE) could decide if a user listed here is selectable to be assigned as a ticket owner.

comment:3 by Christopher Lenz, 14 years ago

Milestone: 0.90.9.1

Postponing…

comment:4 by Christopher Lenz, 14 years ago

See also #2351.

comment:5 by anonymous, 14 years ago

Cc: shishz@… added

comment:6 by Christopher Lenz, 14 years ago

Milestone: 0.9.10.9.2

comment:7 by Christopher Lenz, 14 years ago

Milestone: 0.9.31.0

Later.

comment:8 by markus, 14 years ago

The attached patch adds a User Sessions tab in WebAdmin from where you can delete the project's user sessions.

by markus, 14 years ago

Attachment: WebAdminSessions.patch added

comment:9 by markus, 14 years ago

With the second patch you can also change a user session's name or email. I'm looking forward to your comments. (I'm not sure if the readonly text input is consistent with the rest of Trac's UI)

by markus, 14 years ago

Attachment: WebAdminSessions.2.patch added

by markus, 14 years ago

Attachment: WebAdminSessions.3.patch added

comment:10 by anonymous, 14 years ago

Cc: webkontakt@… added

comment:11 by anonymous, 13 years ago

Cc: vyt@… added

comment:12 by markus, 13 years ago

Is there any demand for an updated patch or should we close this ticket and wait for milestone:0.12 (announcing a Better user/session system)?

comment:13 by Lutz Frommberger <webkontakt@…>, 13 years ago

It will be quite a while until 0.12 will show up. I'd prefer to have the patch again, even if I actually don't need it.

comment:14 by sid, 13 years ago

A workaround for this problem is removing the TICKET_MODIFY permission for that user. If a user doesn't have that permission, they will not show up in the "Assign to" list. (according to TracTickets)

comment:15 by Noah Kantrowitz (coderanger) <coderanger@…>, 13 years ago

Why does this have to be a patch? Just make a plugin to handle it.

in reply to:  15 ; comment:16 by Emmanuel Blot, 13 years ago

Replying to Noah Kantrowitz (coderanger):

Why does this have to be a patch? Just make a plugin to handle it.

IMHO it would better be part of the WebAdmin plugin subsystem (vs. a plugin): this is an administrative task that is useful to any administrator.

in reply to:  16 comment:17 by Noah Kantrowitz (coderanger) <coderanger@…>, 13 years ago

Replying to eblot:

Replying to Noah Kantrowitz (coderanger):

Why does this have to be a patch? Just make a plugin to handle it.

IMHO it would better be part of the WebAdmin plugin subsystem (vs. a plugin): this is an administrative task that is useful to any administrator.

At the point that Trac has a better user directory system, yes. Until then I am just saying it may be easier for them to make a small plugin than to try to keep a patch in sync with WebAdmin.

comment:18 by anonymous, 13 years ago

Milestone: 1.0not applicable

Hi, Im a brazilian user of trac and I´ve made de modifications of the three patches and nothing has chance. There is some other information to apply this changes ?? Tks.

comment:19 by Christian Boos, 13 years ago

Milestone: not applicable1.0

Please don't change arbitrarily the milestone. not applicable is a special milestone (see TracTicketTriage#Milestone if you want more details).

in reply to:  14 comment:20 by Archon810, 12 years ago

Replying to sid:

A workaround for this problem is removing the TICKET_MODIFY permission for that user. If a user doesn't have that permission, they will not show up in the "Assign to" list. (according to TracTickets)

That is a faulty solution as there may be a wider permission group giving this permission (i.e. 'authenticated' group). There needs to be a utility or a plugin that does the sync. I'm quite shocked that Trac would have this bug.

comment:21 by Jeffrey Hulten <jeffh@…>, 11 years ago

Cc: jeffh@… added

comment:22 by John Hampton, 11 years ago

Cc: pacopablo@… added

by John Hampton, 10 years ago

Patch for session management in trac-admin

comment:23 by John Hampton, 10 years ago

Keywords: review added
Owner: changed from Christopher Lenz to John Hampton
Status: newassigned

OK, so, the attached patch adds session management to the trac-admin command. It comes with lots of tests, which pass, though I would like someone to review it for me.

Additionally, the account manager plugin removes sessions when removing users. AccountManagerPlugin should be merged for 0.13. The big question remains: Do we add the session management to the web admin, or just leave it in trac-admin until AccountManagerPlugin is merged. I think that session management in trac-admin is sufficient for 0.12. What say ye all?

in reply to:  23 comment:24 by Remy Blank, 10 years ago

Replying to jhampton:

though I would like someone to review it for me.

A few comments from reading the patch:

  • I would remove the periods at the end of the short command descriptions for consistency with the other commands.
  • There are a few lines >79 characters, in test_session_delete_multiple_sids(), _set_attr(), test_session_admin_purge().
  • There are a number of unnecessary pass statements.
  • Using the separate variables CHECK_AUTH_SQL, CHECK_SID_SQL and ALL_SQL is a bit unusual, all the rest of Trac places the statements in the execute() call.
  • CHECK_SID_SQL can be written more simply with a ",".join().
  • Instead of raise StopIteration, you can simply return from the generator function.
  • Some of your translation markers won't work. You must place the string literal within _() for it to be extracted correctly, and let _() make the string substitutions.
  • When logging exceptions, use exception_to_unicode() as the exception can contain non-ascii characters that are not handled correctly by log. OTOH, other commands don't catch exceptions during their execution, so I would remove the try: except: blocks in _add_session(), _set_attr(), _delete_session() and _purge_sessions() altogether.

I have collected all this into a patch, I'll add it later as an attachment, so you can pick what you like from it (if anything at all).

I think that session management in trac-admin is sufficient for 0.12. What say ye all?

I think so, too.

by Remy Blank, 10 years ago

Attachment: 1347-cleanup-r9261.patch added

Cleanup + minor fixes, applies on top of t1347_session_management.patch.

comment:25 by Remy Blank, 10 years ago

I may have gone a bit too far with the cleanup, so… pick what you want and drop the rest :)

comment:26 by John Hampton, 10 years ago

Resolution: fixed
Status: assignedclosed

I committed the updated patch in [9262]. Thanks for the thorough review. I took all of your changes except for using != in the SQL query. I believe that <> is the SQL operator. I could only find a draft of SQL92 but I don't think != is standard.

I am closing the ticket as fixed due to trac-admin support being sufficient for 0.12

comment:27 by Remy Blank, 10 years ago

Milestone: 1.00.12

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain John Hampton.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from John Hampton 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.