Modify

Opened 3 years ago

Closed 2 years ago

#9252 closed defect (fixed)

All session attributes are deleted when user logs in first time

Reported by: stoecker Owned by: hasienda
Priority: high Component: AccountManagerPlugin
Severity: normal Keywords: user creation
Cc: rjollos, stuge Trac Release: 0.12

Description

When a new users is created and "verify_email" is false, the entry in "session" is defined as not authenticated:

select * from session where sid == 'zzzt';
zzzt|0|0

When user logs in, the data for username and email is no longer there. When data is entered again, then session structure gets a new entry, which seems dangerous to me:

select * from session where sid == 'zzzt';
zzzt|0|0
zzzt|1|1318262166

Afterwards it seems to work, but I expect trouble.

Issues:

  • After login username/mail data gets lots (but is in database)
  • After changes there are two entries in session structure, which may break dataset (sid should be unique).

I get broken database from time to time and I assume this may be the reason for this.

Installed version: TracAccountManager-0.4dev_r10747

Attachments (0)

Change History (19)

comment:1 follow-up: Changed 3 years ago by stoecker

NOTE: When I create a user as admin, authenticated is 0 as well.

comment:2 follow-up: Changed 3 years ago by stoecker

Also the reset-password function sends me an email with new password, but valid is still the old password. Don't know if this is related or another issue.

comment:3 Changed 3 years ago by stoecker

I use HtPasswd password store BTW.

comment:4 in reply to: ↑ 2 Changed 3 years ago by hasienda

  • Keywords user creation added

Replying to stoecker:

Also the reset-password function sends me an email with new password, but valid is still the old password. Don't know if this is related or another issue.

Not exactly right. This is a new feature added in [10264], to take care of complains about possible dangerous behavior according to #816. You'll see that both passwords are valid before the next successful login. No matter what are your primary password stores, the (alternative) new password is stored in a separate db-based store ResetPwStore, that is derived from SessionStore.

comment:5 in reply to: ↑ 1 ; follow-up: Changed 3 years ago by hasienda

Replying to stoecker:

NOTE: When I create a user as admin, authenticated is 0 as well.

Do you suggest, that we should lie about that again? In fact authenticated has been set before [10520], but starting with [10585] it's not authenticated, until the user has been really authenticated at least once.

I'll have to find a way to preserve the initial attributes. There's obviously something wrong in the way the unauthenticated session is propagated into an authenticated one.

comment:6 in reply to: ↑ description Changed 3 years ago by hasienda

  • Priority changed from highest to high

Replying to stoecker:

When user logs in, the data for username and email is no longer there. When data is entered again, then session structure gets a new entry, which seems dangerous to me:

Afterwards it seems to work, but I expect trouble.

Why? Don't think so. At least the db schema suggests, that only sid and authenticate together are required to be unique. I can reproduce the second session, that results in two entries in session table. Really annoying issue, but highest priority?

I get broken database from time to time and I assume this may be the reason for this.

I'd really appreciate to give more help, if possible. Could you provide such a broken db, at least with the session and session_attribute table, other tables and sensitive content stripped?

comment:7 follow-up: Changed 3 years ago by anonymous

Hello,

I'd really appreciate to give more help, if possible. Could you provide such a broken db, at least with the session and session_attribute table, other tables and sensitive content stripped?

I don't think so. But according to your text I assume there is another reason.

separate db-based store ResetPwStore, that is derived from SessionStore

Shouldn't there be another table in the trac DB then?

comment:8 in reply to: ↑ 7 Changed 3 years ago by hasienda

Replying to anonymous:

Hello,

I'd really appreciate to give more help, if possible. Could you provide such a broken db, at least with the session and session_attribute table, other tables and sensitive content stripped?

I don't think so. But according to your text I assume there is another reason.

Just an offer.

separate db-based store ResetPwStore, that is derived from SessionStore

Shouldn't there be another table in the trac DB then?

No, why? SessionStore is yet another content welded into session_attribute where the attribute name is the key. Same technique as ticket_custom. And you can easily have multiple SessionStore instances in that table (I changed the class that way), just use another name than 'password' for self.key: ResetPwStore's key is 'password_reset' - as can be read at the top of changeset [10264].

Actually I can't believe that I explain this to you. Sorry, but did you care to read my comments and corresponding links? I'm certainly far less experienced than you, but OTOH I don't do random programming for pure chance. Bear with me.

comment:9 follow-up: Changed 3 years ago by stoecker

Actually I can't believe that I explain this to you.

I'm in no way a Trac expert.

key is 'password_reset'

I now checked it and I get a password change e-mail. A new entry is added to DB:

zzz|1|password_reset|:e7ba839ea782bdf77dae2a64a79f89ca

But I can't use the password for login. Only the old password works. So either password retrieval from the DB or password check fail.

You can try yourself if you want: Username is zzz at josm.openstreetmap.de. Old password is "zzz". New one is "ffku4oxw".

JOSM has open account policy, so you can change the account like you wish :-)

Regarding the authenticated issue. For a new entered user the entry is like this:

session:
zzz2|0|0
session_attribute:
zzz2|1|email|zzz2@dstoecker.de
zzz2|1|name|zzz2

So while authenticated is "0", the session entry is "1". This does not match. Maybe this is the reason why the values are lost?

comment:10 in reply to: ↑ 9 Changed 3 years ago by hasienda

  • Status changed from new to assigned

Replying to stoecker:

Actually I can't believe that I explain this to you.

I'm in no way a Trac expert.

Well, only I thought so. :-)

key is 'password_reset'

I now checked it and I get a password change e-mail. A new entry is added to DB: ![...] But I can't use the password for login. Only the old password works. So either password retrieval from the DB or password check fail.

I've tested the procedure several times during it's development. But I can't deny the possibility of later modifications, that broke it again. (More and more I see the big value if test suites here, once you have them.)

You can try yourself if you want: Username is zzz ![...]

JOSM has open account policy, so you can change the account like you wish :-)

Oh, that's an unexpected offer. I'll check.

Regarding the authenticated issue. For a new entered user the entry is like this: ![...] So while authenticated is "0", the session entry is "1". This does not match. Maybe this is the reason why the values are lost?

Yes, this might be unexpected and existing values could get deleted on pivoting the session from unauthenticated to authenticated status. Still need to verify and find a better way.

comment:11 Changed 3 years ago by hasienda

The issue has been referred to in a comment to #6318 too.

comment:12 Changed 3 years ago by rjollos

  • Cc rjollos added; anonymous removed

comment:13 Changed 3 years ago by rjollos

#9755 closed as a duplicate.

comment:14 in reply to: ↑ 5 Changed 3 years ago by anonymous

Replying to hasienda:

Replying to stoecker:

NOTE: When I create a user as admin, authenticated is 0 as well.

Do you suggest, that we should lie about that again? In fact authenticated has been set before [10520], but starting with [10585] it's not authenticated, until the user has been really authenticated at least once.

Like in #9843, I am suggesting a clear "yes" (i.e. lie), and the most compelling reason is this: I want env.get_known_users() to list a user that just has been created by the admin, using this plugin's GUI. I can't see any reason why the user should be treated as "unknown" just after the admin created his or her account.

comment:15 Changed 2 years ago by peter@…

  • Cc peter@… added
  • Summary changed from New user creation broken without verification turned on to All session attributes are deleted when user logs in first time

comment:16 Changed 2 years ago by stuge

  • Cc stuge added; peter@… removed

comment:17 Changed 2 years ago by hasienda

I think, this is, what needs to be done then:

  • accountmanagerplugin/trunk/acct_mgr/web_ui.py

     
    165165        cursor.execute("""
    166166            INSERT INTO session
    167167                    (sid,authenticated,last_visit)
    168             VALUES  (%s,0,0)
     168            VALUES  (%s,1,0)
    169169            """, (username,))
    170170
    171171    for attribute in ('name', 'email'):

comment:18 Changed 2 years ago by hasienda

(In [11826]) AccountManagerPlugin: Hotfix for pending user-registration issue, refs #9079, #9252, #9843 and #9090.

Now the reversion of [10520] done by [10585] is completed, although there are many related changes and improvements in-between, so this is not quite the same code as before.

Taking the chance to update now obsolete configuration examples in README too.

This has already open for much too long, so let's ditch the rather esoteric considerations of authenticated user entries in Trac db table session for now and move on.

Big sorry, that this took that much time, and thanks to Peter Stuge for finally pushing me to it tonight.

comment:19 Changed 2 years ago by hasienda

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

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.

Add Comment

Modify Ticket

Action
as closed The owner will remain hasienda.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.