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

Opened 10 years ago

Last modified 10 years ago

.allow_only doesn't distinguish 401s vs. 403s

Reported by: jorge.vargas Owned by: Gustavo
Priority: highest Milestone: 2.0rc1
Component: TurboGears Version: 2.0b7
Severity: blocker Keywords: repoze.what, repoze.what-pylons
Cc:

Description

After the changes needed for b7.

You will be logged out if you try to access a resource you can't access.

to reproduce.

quickstart a project login as editor click the /admin

you are redirected to login, after that your credentials are gone.

Attachments

no-check-security.diff Download (2.3 KB) - added by Gustavo 10 years ago.
Patch to revert _check_security() stuff

Change History

comment:1 Changed 10 years ago by Gustavo

  • Owner changed from percious to Gustavo
  • Priority changed from normal to highest
  • Status changed from new to assigned
  • Component changed from Catwalk2 to TurboGears
  • Severity changed from major to critical

That's the _check_security() stuff in tg.controller which doesn't distinguish 401s vs 403s.

I'd rather switch back to repoze.what-pylons instead of fixing it.

comment:2 Changed 10 years ago by Gustavo

  • Keywords repoze.what, repoze.what-pylons added
  • Severity changed from critical to blocker
  • Summary changed from Catwalk forgets your credentials when wrong predicate to .allow_only doesn't distinguish 401s vs. 403s

I'm waiting for a confirmation to undo [6481].

If repoze.what-pylons' approach stopped working is because __before__ is broken. So it needs to be fixed first.

comment:3 Changed 10 years ago by Gustavo

FTR, added four failing tests in [6491].

comment:4 follow-up: ↓ 5 Changed 10 years ago by mramm

Gustavo, can we sort this out tomorrow.

I have a couple failing auth tests with the current trunk, and I'd like to figure out the full problem and get this resolved sooner rather than later, so that we can do an RC1 release this week.

I'm willing to revert 6481, if and only if everything works out of the box without it.

Are there still problems with the before, and if so is there any way we can have tests that show only that problem so that we can fix it?

Changed 10 years ago by Gustavo

Patch to revert _check_security() stuff

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 10 years ago by Gustavo

Replying to mramm:

Gustavo, can we sort this out tomorrow.

I'm sorry, I didn't have enough time yesterday.

I have a couple failing auth tests with the current trunk, and I'd like to figure out the full problem and get this resolved sooner rather than later, so that we can do an RC1 release this week.

I'm willing to revert 6481, if and only if everything works out of the box without it.

Are there still problems with the before, and if so is there any way we can have tests that show only that problem so that we can fix it?

Yes, there are still problems with __before__.

I don't have tests I added some tests for controller-wide authorization using the .allow_only attribute and __before__, so that we can reproduce the problem. If you revert the _check_security stuff (e.g., using the patch I attached), you'll see four failing tests -- there's our problem.

I've spent more time on it, but I've not been able to fix it.

comment:6 in reply to: ↑ 5 Changed 10 years ago by Gustavo

Replying to Gustavo:

I don't have tests I added some tests for controller-wide authorization using the .allow_only attribute and __before__

Whops, I meant "I don't have tests for __before__ alone, but I added some tests for..."

comment:7 Changed 10 years ago by mramm

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

This is fixed in trunk as of this evening.

Note: See TracTickets for help on using tickets.