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 #1131 (closed defect: fixed)

Opened 13 years ago

Last modified 12 years ago

[Patch] identity.SecureResource not catching IdentityExceptions properly

Reported by: PeterRussell Owned by: anonymous
Priority: high Milestone: 1.0b2
Component: Identity Version: 1.0b1
Severity: normal Keywords:
Cc:

Description

Throwing an identity.IdentityException? inside a controller method is not caught by identity.SecureResource?, as described in the Documentation ( http://docs.turbogears.org/1.0/IdentityManagment ).

Attached are two patches, one to the tests, which adds a few new tests, one of which fails, illustrating the problem, the other is to secure resource, which fixes the problem.

The second of these I'm not quite sure about. It seems the problem is that the original author expected IdentityExceptions? in the wrapped methods to propogate to the SecureResource? _getattribute_ method, but it won't since the wrapped method is returned into a different scope. There are therefore probably unnecessary try, except blocks, which this patch does not remove. Someone who understands Identity should have a careful look before applying this patch.

Attachments

identity_tests.patch Download (4.6 KB) - added by PeterRussell 13 years ago.
identity_conditions.patch Download (687 bytes) - added by PeterRussell 13 years ago.

Change History

Changed 13 years ago by PeterRussell

Changed 13 years ago by PeterRussell

comment:1 Changed 13 years ago by jorge.vargas

  • Priority changed from normal to high

thanks for the tests!

need a deeper look to the other patch

comment:2 Changed 13 years ago by alberto

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

I believe this patch should not be applied. The docs state that:

Again, derive your controller from identity.SecureResource. Perform your identity check at the method level. If the user doesn't have the required permissions, throw a *suitable* IdentityException

(Emphais by me)

SecureResource? should not trap any IdentityException? and redirect to login page because maybe that's not what the user wants (maybe a user-defined IdentityException? should trigger a "403 Forbidden" instead of a redirection to the login page and 401...)

I've comitted your tests at [2250] but tweaked in_admin_group_explicit_check to explicitly raise an IdentityFailure? which is what will trigger the intended behaviour (redirecting to login page).

What I've fixed is making sure IdentityFailure? inherits from IdentityException? too so the docs don't lie ;) (well, and because an IdentityFailure? should be an IdentityException?, shouldn't it?)

Thanks for the tests :)

Alberto

Note: See TracTickets for help on using tickets.