Closed
Bug 751419
Opened 13 years ago
Closed 13 years ago
AITC spec should explicitly define behavior when PUTing to an invalid app id
Categories
(Cloud Services Graveyard :: Server: Sync, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: rfkelly, Assigned: rfkelly)
Details
(Whiteboard: [qa+])
Attachments
(2 files, 1 obsolete file)
638 bytes,
patch
|
Details | Diff | Splinter Review | |
2.43 KB,
patch
|
telliott
:
review+
|
Details | Diff | Splinter Review |
The AITC spec is not clear on expected behavior when I PUT /apps/<id> with an invalid <id>, i.e. thwn the <id> is not base64urlencode(sha1(origin)) of the incoming record.
I am deferring to Greg on this one, as I can see arguments for any of 400, 403 or 404 being the correct response code.
Comment 1•13 years ago
|
||
I think 400 is closest. It implies that altering the request may make it succeed.
404 just seems right out, though I could see 403. Pretty weird, though
Comment 2•13 years ago
|
||
I don't think 400 is proper because it implies that altering the request may make it succeed. And, I don't consider the URI part of the request. So, there is nothing you can do to the request to make it work.
I'd be fine with 403 or 404. 403 feels better to me. "You tried to do that, but I'm not allowing you to use that URI." 404 feels weird on a PUT.
Updated•13 years ago
|
Whiteboard: [qa+]
Assignee | ||
Comment 3•13 years ago
|
||
I agree that there's something fundamentally different about this particular error - you're not posting "bad data", you're posting good data to the wrong location. 404 feels weird, so 403 seems like the best option.
Assignee | ||
Comment 4•13 years ago
|
||
The code for this was not nearly as simple as I expected.
It appears that Pyramid uses the HTTPForbidden exception to trigger parts of its access-control mechanism. So if I just do
Assignee: nobody → rfkelly
Attachment #621821 -
Flags: feedback?(telliott)
Attachment #621821 -
Flags: feedback?(bbangert)
Assignee | ||
Comment 5•13 years ago
|
||
Err, something ate my comment. Here it is in full:
The code for this was not nearly as simple as I expected.
It appears that Pyramid uses the HTTPForbidden exception to trigger parts of its access-control mechanism. So if I just do a "raise HTTPForbidden" then it gets intercepted and interpreted as "permission denied", triggering the view that prompts for login credentials (aka the "forbidden view").
In the attached patch I have hacked around this by introducing a special subclass of HTTPForbidden, then customizing the forbidden view to pass it straight through to the client. This is pretty yuck - Ben, do you have any suggestions on how I can do this better?
(I could just say "return HTTPForbidden()" instead of raising it, but that doesn't sit right with me. This is an error condition, it should raise something)
Comment 6•13 years ago
|
||
The forbidden view can be overridden, and probably should be. Because we need custom logic in the forbidden view to determine whether it really should be a forbidden warning (user is already logged in, don't retry request) vs. whether it should actually be altered to a 401 (user wasn't logged in, and should retry after logging in). Pyramid's permission checking as you noted can't tell the difference apparently, so it raises a Forbidden for an ACL mismatch as its close to impossible for Pyramid to actually figure out if retrying would work.
More details on why its this way here:
https://github.com/Pylons/pyramid/issues/436
Upon seeing that, I quickly decided not to enter that bike-shed and ran away.
I'm surprised this patch works, given that the forbidden view is technically already an exception view, which means its not allowed to raise exceptions of their own as they'll be raised all the way up.... oh... repoze.who. It's worth noting that r.who's own creator doesn't use it, and its been ripped out of most everything that people have used because its interactions with existing security policies leads to hard to debug situations (multiple sources of authn truth in a single app at different layers gets very yucky, precisely when using pyramid as it has its own security that is now interacting with a layer above it as well).
tl;dr; I'd suggest returning HTTPForbidden instead of raising it even though it doesn't sit right with you, because you actually want to return that to the user, and not have it go through the various security layers (please rip out r.who if possible, multiple sources of authn truth and middleware auth layers interacting with pyramid security layers is gonna end in tears someday). Since it seems unlikely that would be a quick removal, I'll have to pull all the code down and study it a bit to get a clearer picture of what's happening and get back to you within the next day or so after seeing the big picture.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Ben Bangert from comment #6)
> More details on why its this way here:
> https://github.com/Pylons/pyramid/issues/436
>
> Upon seeing that, I quickly decided not to enter that bike-shed and ran away.
A wise decision!
> I'm surprised this patch works, given that the forbidden view is technically
> already an exception view, which means its not allowed to raise exceptions
> of their own as they'll be raised all the way up....
Now that you mention it, it doesn't work - at least, not when I actually *raise* the exception instead of accidentally returning it like I do in the current patch :-/
> tl;dr; I'd suggest returning HTTPForbidden instead of raising it even though
> it doesn't sit right with you, because you actually want to return that to
> the user, and not have it go through the various security layers
Yeah, I had a feeling it might come down to that. Also recommended by mcdonc in the epic bug thread about this: https://github.com/Pylons/pyramid/issues/436#issuecomment-3961079
> (please rip
> out r.who if possible, multiple sources of authn truth and middleware auth
> layers interacting with pyramid security layers is gonna end in tears
> someday).
Twice now I've come close to doing this, and twice someone has piped up with a potential complication that made it sound worth keeping. Its continued presence is me hedging my bets that we might need to continue something from the old codebase with a new auth mechanism.
I think things have evolved far enough that this is an unlikely outcome, and the responsibility for pluggable auth schemes has been shifted to the tokenserver. So I am on board with ripping out r.who and using a plain pyramid auth plugin.
> Since it seems unlikely that would be a quick removal, I'll have
> to pull all the code down and study it a bit to get a clearer picture of
> what's happening and get back to you within the next day or so after seeing
> the big picture.
I'm not too worried about major upheaval here, since I've been trying very hard to have consumer code only use the native pyramid auth stuff. It would basically involve recasting this as a pyramid auth plugin:
https://github.com/mozilla-services/repoze.who.plugins.macauth
And changing the mozsvc custom subclass of it to match:
https://github.com/mozilla-services/mozservices/blob/master/mozsvc/user/whoauth.py#L59
Anyway, I'll file another bug for that.
Assignee | ||
Comment 8•13 years ago
|
||
Updated code patch, sucking it up and just returning the error response.
Attachment #621821 -
Attachment is obsolete: true
Attachment #621821 -
Flags: feedback?(telliott)
Attachment #621821 -
Flags: feedback?(bbangert)
Attachment #622176 -
Flags: review?(telliott)
Updated•13 years ago
|
Attachment #622176 -
Flags: review?(telliott) → review+
Assignee | ||
Comment 9•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•2 years ago
|
Product: Cloud Services → Cloud Services Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•