Ticket #1628 (closed enhancement: fixed)
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
(5.2 KB) -
added by chrisz 4 years ago.
- Patch against 1.0 branch
-
no_cookie_patch_2_branch_1_0.patch
(12.5 KB) -
added by chrisz 4 years ago.
- Patch with template unit tests against 1.0 branch
Change History
comment:2 Changed 4 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 4 years ago by chrisz
I have now simplified the patch and added unit tests to the quickstart template.
comment:4 Changed 4 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 4 years ago by chrisz
-
attachment
no_cookie_patch_branch_1_0.patch
added
Patch against 1.0 branch
Changed 4 years ago by chrisz
-
attachment
no_cookie_patch_2_branch_1_0.patch
added
Patch with template unit tests against 1.0 branch
comment:7 Changed 3 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 3 years ago by Chris Arndt
Looks good to me. What about the tests from your patch?
comment:10 Changed 3 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 3 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.
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.