Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1628 (closed enhancement: fixed)

Opened 9 years ago

Last modified 9 years ago

Identity fails without message when cookies are disabled

Reported by: chrisz Owned by: anonymous
Priority: normal Milestone: 1.1
Component: TurboGears Version: 1.0.4b3
Severity: normal Keywords: cookies identity login
Cc:

Description

When cookies are disabled in your browser, then Identity lets you log in, but you immediately loose your credentials again. That's confusing for users.

I suggest the following patch that does not let you log in unless cookies are enabled and gives the user a message about this problem in the login dialog.

Alternatively, one could issue flash() message when cookies are disabled. However, this can easily be overwritten by other flash() messages. For instance, the default project sets its own "The application is now running" flash() message so you would never see this error.

Attachments

no_cookie_patch_branch_1_0.patch Download (5.2 KB) - added by chrisz 9 years ago.
Patch against 1.0 branch
no_cookie_patch_2_branch_1_0.patch Download (12.5 KB) - added by chrisz 9 years ago.
Patch with template unit tests against 1.0 branch

Change History

comment:1 Changed 9 years ago by Chris Arndt

We should set the milestone for this to 1.1, IMHO. We really need to get 1.0.4 out the door soon and this is too big a change for this late time in the release process.

We can always decide afterwards, whether we want to do another 1.0.x bugfix release.

comment:2 Changed 9 years ago by chrisz

It looks a bit bigger than it really is, and I have it already in production. But if you don't feel confident about this, we can move it to 1.0.5/1.1.

By the way, just noticed that another reason why the error has to be appended to the login message instead of flash is that flash also depends on cookies being enabled.

comment:3 Changed 9 years ago by chrisz

I have now simplified the patch and added unit tests to the quickstart template.

comment:4 Changed 9 years ago by chrisz

My unit tests covered only the error case so far. I thought when I'm already at it I should add another unit test to the quickstart template verifying that you can log in and log out with the right credentials.

Changed 9 years ago by chrisz

Patch against 1.0 branch

Changed 9 years ago by chrisz

Patch with template unit tests against 1.0 branch

comment:5 Changed 9 years ago by Chris Arndt

  • Milestone changed from 1.0.4 to 1.1

comment:6 Changed 9 years ago by faide

  • Milestone changed from 1.5 to 1.1

comment:7 Changed 9 years ago by chrisz

Implemented this in a cleaner way for 1.1 and 1.5 in r5459. Instead of checking the cookie, I check the visit object, so this should not give false alarms when visit.source="form". If you agree with this simple fix, I suggest backporting it to the 1.0 branch as well.

comment:8 Changed 9 years ago by Chris Arndt

Looks good to me. What about the tests from your patch?

comment:9 Changed 9 years ago by chrisz

Added the tests in r5460.

comment:10 Changed 9 years ago by faide

Not too keen on the backport. Even if you fix a use case you still add functionnalites. And this does not seem to me like a gaping bug since no one yell about this in 2 years.

I feel we should only have this in 1.1 and push people to use 1.1 anyway.

comment:11 Changed 9 years ago by chrisz

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

Ok, I won't backport it then and close the ticket. Since this affects only the project templates, this feature can easily be added to existing projects, and as you say new project should start with 1.1 anyway.

Note: See TracTickets for help on using tickets.