751419 - AITC spec should explicitly define behavior when PUTing to an invalid app id

Status: RESOLVED FIXED

Description

[Ryan Kelly [:rfkelly]]

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

[Toby Elliott [:telliott]]

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

[Gregory Szorc [:gps]]

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.

Comment 3

[Ryan Kelly [:rfkelly]]

Created attachment 621570 [details] [diff] [review]
docs patch to add 403 forbidden response code

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.

Comment 4

[Ryan Kelly [:rfkelly]]

Created attachment 621821 [details] [diff] [review]
code patch to add 403 forbidden response code

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

Comment 5

[Ryan Kelly [:rfkelly]]

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

[Ben Bangert [:benbangert]]

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.

Comment 7

[Ryan Kelly [:rfkelly]]

(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.

Comment 8

[Ryan Kelly [:rfkelly]]

Created attachment 622176 [details] [diff] [review]
code patch to add 403 forbidden response code

Updated code patch, sucking it up and just returning the error response.

Comment 9

[Ryan Kelly [:rfkelly]]

Docs:
https://github.com/mozilla-services/docs/commit/69f6ce44d48651e3c8403257252394b50bb6c423

Code:
https://github.com/mozilla-services/server-aitc/commit/a8a063e51ce379bf26195101e559e9865161c75e