jywarren / plots2

The Public Lab website!
http://publiclab.org
GNU General Public License v3.0
17 stars 2 forks source link

Password reset by key is not responding #194

Closed btbonval closed 11 years ago

btbonval commented 11 years ago

Use Naljorpa has a reset key, but when submitting the proper username and a password, the browser is forwarded to publiclab.org with an error that "You must be logged in."

The password hash in the database remains unchanged, meaning this auto forward failed to modify the database.

jywarren commented 11 years ago

Is this independent from the capitalization bug in reset?

On Thu, Oct 10, 2013 at 12:33 PM, Bryan Bonvallet notifications@github.comwrote:

Use Naljorpa has a reset key, but when submitting the proper username and a password, the browser is forwarded to publiclab.org with an error that "You must be logged in."

The password hash in the database remains unchanged, meaning this auto forward failed to modify the database.

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194 .

btbonval commented 11 years ago

It is the same user, but otherwise independent of #193. When the case is properly used, the problem in this ticket is the result.

On Thu, Oct 10, 2013 at 1:46 PM, Jeffrey Warren notifications@github.comwrote:

Is this independent from the capitalization bug in reset?

On Thu, Oct 10, 2013 at 12:33 PM, Bryan Bonvallet notifications@github.comwrote:

Use Naljorpa has a reset key, but when submitting the proper username and a password, the browser is forwarded to publiclab.org with an error that "You must be logged in."

The password hash in the database remains unchanged, meaning this auto forward failed to modify the database.

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194> .

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26075108 .

jywarren commented 11 years ago

when submitting the proper username and a password

being when the password has actually been reset, or at the moment of resetting it? Sorry, just trying to figure out what line of code is resulting in the redirect to login. What step does that happen at?

On Thu, Oct 10, 2013 at 1:48 PM, Bryan Bonvallet notifications@github.comwrote:

It is the same user, but otherwise independent of #193. When the case is properly used, the problem in this ticket is the result.

On Thu, Oct 10, 2013 at 1:46 PM, Jeffrey Warren notifications@github.comwrote:

Is this independent from the capitalization bug in reset?

On Thu, Oct 10, 2013 at 12:33 PM, Bryan Bonvallet notifications@github.comwrote:

Use Naljorpa has a reset key, but when submitting the proper username and a password, the browser is forwarded to publiclab.org with an error that "You must be logged in."

The password hash in the database remains unchanged, meaning this auto forward failed to modify the database.

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194> .

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194#issuecomment-26075108> .

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26075250 .

btbonval commented 11 years ago

Steps to reproduce: visit /reset/key/somelongthing type in username in proper case type in desired password press Save

get forwarded to publiclab.org with error about not being logged in.

On Thu, Oct 10, 2013 at 2:01 PM, Jeffrey Warren notifications@github.comwrote:

when submitting the proper username and a password

being when the password has actually been reset, or at the moment of resetting it? Sorry, just trying to figure out what line of code is resulting in the redirect to login. What step does that happen at?

On Thu, Oct 10, 2013 at 1:48 PM, Bryan Bonvallet notifications@github.comwrote:

It is the same user, but otherwise independent of #193. When the case is properly used, the problem in this ticket is the result.

On Thu, Oct 10, 2013 at 1:46 PM, Jeffrey Warren < notifications@github.com>wrote:

Is this independent from the capitalization bug in reset?

On Thu, Oct 10, 2013 at 12:33 PM, Bryan Bonvallet notifications@github.comwrote:

Use Naljorpa has a reset key, but when submitting the proper username and a password, the browser is forwarded to publiclab.org with an error that "You must be logged in."

The password hash in the database remains unchanged, meaning this auto forward failed to modify the database.

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194> .

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194#issuecomment-26075108> .

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194#issuecomment-26075250> .

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26076427 .

btbonval commented 11 years ago

Found the log snippet (things in all caps were scrubbed for public posting):

Started GET "/reset/key/LONGTHING?key=LONGTHING&user%5Busername%5D=USERNAME&user%5Bpassword%5D=[FILTERED]" for IPADDRESS at 2013-10-10 16:22:03 +0000
Processing by UsersController#reset as HTML
  Parameters: {"key"=>"LONGTHING", "user"=>{"username"=>"USERNAME", "password"=>"[FILTERED]"}}
Redirected to http://www.publiclab.org/dashboard
Completed 302 Found in 121ms (ActiveRecord: 108.3ms)
  Rendered users/profile.html.erb within layouts/application (625.4ms)
Read fragment views/tool-place-menu-4604740 (0.3ms)
  Rendered layouts/_alerts.html.erb (0.1ms)
Completed 200 OK in 643ms (Views: 368.8ms | ActiveRecord: 265.5ms)
Started GET "/dashboard" for IPADDRESS at 2013-10-10 16:22:03 +0000
jywarren commented 11 years ago

So do you think it's happening within this method? I can't see how... but my brain is filled with crap right now...

https://github.com/jywarren/plots2/blob/master/app/controllers/users_controller.rb#L128

On Thu, Oct 10, 2013 at 2:07 PM, Bryan Bonvallet notifications@github.comwrote:

Found the log snippet (things in all caps were scrubbed for public posting):

Started GET "/reset/key/LONGTHING?key=LONGTHING&user%5Busername%5D=USERNAME&user%5Bpassword%5D=[FILTERED]" for IPADDRESS at 2013-10-10 16:22:03 +0000 Processing by UsersController#reset as HTML Parameters: {"key"=>"LONGTHING", "user"=>{"username"=>"USERNAME", "password"=>"[FILTERED]"}} Redirected to http://www.publiclab.org/dashboard Completed 302 Found in 121ms (ActiveRecord: 108.3ms) Rendered users/profile.html.erb within layouts/application (625.4ms) Read fragment views/tool-place-menu-4604740 (0.3ms) Rendered layouts/_alerts.html.erb (0.1ms) Completed 200 OK in 643ms (Views: 368.8ms | ActiveRecord: 265.5ms) Started GET "/dashboard" for 96.237.171.246 at 2013-10-10 16:22:03 +0000

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26076914 .

btbonval commented 11 years ago

Because the error is "You must be logged in to view this page.", and also based on the log paste above, I'm guessing the browser is being forwarded to the dashboard. Since the user is not signed in, the dashboard forwards to publiclab.org with the error.

The only dashboard redirect is in the section of code that should be called. https://github.com/jywarren/plots2/blob/master/app/controllers/users_controller.rb#L138

The lines of code above it must be failing quietly or something. save({}) can be wrapped up in a thingy to catch errors which otherwise seem to be quietly ignored. Since that is the only real action being taken in the block, it's the only point I can see a failure occurring.

jywarren commented 11 years ago

Wait, but is the desired password actually saved? because do we really know that something has failed? If the password is actually saved, we may have a poorly designed notification where we do want them to log in, but not by directing them to the dashboard -- instead by redirecting them to /login with a message like "Your password was changed. Please log in now."

But if the password is not saved, we do indeed have a "real" problem.

On Thu, Oct 10, 2013 at 2:26 PM, Bryan Bonvallet notifications@github.comwrote:

Because the error is "You must be logged in to view this page.", and also based on the log paste above, I'm guessing the browser is being forwarded to the dashboard. Since the user is not signed in, the dashboard forwards to publiclab.org with the error.

The only dashboard redirect is in the section of code that should be called.

https://github.com/jywarren/plots2/blob/master/app/controllers/users_controller.rb#L138

The lines of code above it must be failing quietly or something. save({})can be wrapped up in a thingy to catch errors which otherwise seem to be quietly ignored. Since that is the only real action being taken in the block, it's the only point I can see a failure occurring.

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26078460 .

btbonval commented 11 years ago

Password is not saved. The password hash in the database remains completely unchanged, in fact the entire database entry for the user remains completely unchanged after the Save button is clicked and the browser is forwarded.

On Thu, Oct 10, 2013 at 2:50 PM, Jeffrey Warren notifications@github.comwrote:

Wait, but is the desired password actually saved? because do we really know that something has failed? If the password is actually saved, we may have a poorly designed notification where we do want them to log in, but not by directing them to the dashboard -- instead by redirecting them to /login with a message like "Your password was changed. Please log in now."

But if the password is not saved, we do indeed have a "real" problem.

On Thu, Oct 10, 2013 at 2:26 PM, Bryan Bonvallet notifications@github.comwrote:

Because the error is "You must be logged in to view this page.", and also based on the log paste above, I'm guessing the browser is being forwarded to the dashboard. Since the user is not signed in, the dashboard forwards to publiclab.org with the error.

The only dashboard redirect is in the section of code that should be called.

https://github.com/jywarren/plots2/blob/master/app/controllers/users_controller.rb#L138

The lines of code above it must be failing quietly or something. save({})can be wrapped up in a thingy to catch errors which otherwise seem to be quietly ignored. Since that is the only real action being taken in the block, it's the only point I can see a failure occurring.

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194#issuecomment-26078460> .

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26080416 .

jywarren commented 11 years ago

This is weird... for any user or just this user? We've long been able to reset passwords with this system. Just trying to wrap my head around it.

On Thu, Oct 10, 2013 at 2:51 PM, Bryan Bonvallet notifications@github.comwrote:

Password is not saved. The password hash in the database remains completely unchanged, in fact the entire database entry for the user remains completely unchanged after the Save button is clicked and the browser is forwarded.

On Thu, Oct 10, 2013 at 2:50 PM, Jeffrey Warren notifications@github.comwrote:

Wait, but is the desired password actually saved? because do we really know that something has failed? If the password is actually saved, we may have a poorly designed notification where we do want them to log in, but not by directing them to the dashboard -- instead by redirecting them to /login with a message like "Your password was changed. Please log in now."

But if the password is not saved, we do indeed have a "real" problem.

On Thu, Oct 10, 2013 at 2:26 PM, Bryan Bonvallet notifications@github.comwrote:

Because the error is "You must be logged in to view this page.", and also based on the log paste above, I'm guessing the browser is being forwarded to the dashboard. Since the user is not signed in, the dashboard forwards to publiclab.org with the error.

The only dashboard redirect is in the section of code that should be called.

https://github.com/jywarren/plots2/blob/master/app/controllers/users_controller.rb#L138

The lines of code above it must be failing quietly or something. save({})can be wrapped up in a thingy to catch errors which otherwise seem to be quietly ignored. Since that is the only real action being taken in the block, it's the only point I can see a failure occurring.

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194#issuecomment-26078460> .

— Reply to this email directly or view it on GitHub< https://github.com/jywarren/plots2/issues/194#issuecomment-26080416> .

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26080527 .

btbonval commented 11 years ago

I volunteer for guinea pig! I got the reset email. I reset my password, it forwarded me to dashboard, there were no problems.

It appears to be just this user. how fun and exciting! to preserve anonymity, the user's ruser.id = 306917. This user account was created like yesterday, whereas mine was created in the long, long ago.

I wonder if maybe there is something missing from the users table or something, for newly created users. we should create a brand new test user and reset its password.

btbonval commented 11 years ago

Managed to reproduce the problem using a new user called "btbonvaltesting". Reset key was emailed: http://publiclab.org/reset/key/abiapgzzlrqoegvpopwa

Putting username "btbonvaltesting" into the user field, and any password, then clicking save causes the same problem: "Not logged in" and no real change.

Hypothesis re: new users vs old users confirmed.

So what is different?

btbonval commented 11 years ago

DATA is null for new users in users table, but is all gobbledy gook(the good kind) for old users.

openid_identifier is null for new users in rusers table, but has old.publiclaboratory.org link for old users (at least it has it for my old user account which I successfully reset my password with).

No other differences pop out.

btbonval commented 11 years ago

Filled in users.data from my old user into my new user. Problem did not resolve.

Reset users.data back to NULL.

Filled in rusers.openid_identifier to be http://old.publiclab.org/user/#/identity, with # matching the user id. Reset worked as expected.

Problem found. I'll update the user who issued the problem with an openid_identifier so that user can move forward. This problem needs to be fixed for user creation in general.

We also need to backfill anyone without openid_identifiers

btbonval commented 11 years ago

temporary btbonvaltesting user deleted.

jywarren commented 11 years ago

Ah, is it a validation fail that users require an openid_identifier? But if they have only created a new account and haven't migrated from the old site, they don't really need one, and actually perhaps do not really have a legacy openid identity on old.publiclab.org... right? I don't see a validation for that in https://github.com/jywarren/plots2/blob/master/app/models/user.rb, so I wonder why it's saying one is required? we should grep around in the code to find if we're assuming its presence anywhere else. What if we just make it not null? just an empty string... hmm

On Thu, Oct 10, 2013 at 4:30 PM, Bryan Bonvallet notifications@github.comwrote:

temporary btbonvaltesting user deleted.

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26088729 .

btbonval commented 11 years ago

I don't think it is the user model validation. The user is created with an empty openid_identifier from the get-go, and that should go through all the same validation when saved at creation, right?

jywarren commented 11 years ago

yeah... good point

On Thu, Oct 10, 2013 at 5:18 PM, Bryan Bonvallet notifications@github.comwrote:

I don't think it is the user model validation. The user is created with an empty openid_identifier from the get-go, and that should go through all the same validation when saved at creation, right?

— Reply to this email directly or view it on GitHubhttps://github.com/jywarren/plots2/issues/194#issuecomment-26092675 .

btbonval commented 11 years ago

Yet it still feels like the problem could only be caused on line 136, as that is the only real action taken prior to the redirect. Is it possible that there is different validation for save depending upon circumstances?

https://github.com/jywarren/plots2/blob/master/app/controllers/users_controller.rb#L136