scream78 / oauth

Automatically exported from code.google.com/p/oauth
0 stars 0 forks source link

Separate 400 and 401 exceptions? #48

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
[this bug is in reference to the Python library]

The specification describes a set of cases where a 400 response is
appropriate, and another set of cases where a 401 response should be used
instead.

All these cases seem to generate an OAuthError exception (or in some cases
by returning None).  This makes it difficult to produce a relevant error
response.

Original issue reported on code.google.com by james.he...@gmail.com on 9 Oct 2008 at 7:44

GoogleCodeExporter commented 9 years ago

Original comment by leah.culver on 14 Jan 2009 at 9:31

GoogleCodeExporter commented 9 years ago
Right now there's only one kind of OAuth exception for the Python and PHP 
libraries.
The spec mentions 400 and 401 errors separately in section 10. Should we 
distinguish
between the two?

http://oauth.net/core/1.0/#http_codes

Original comment by leah.culver on 14 Jan 2009 at 11:23

GoogleCodeExporter commented 9 years ago
I think it would be very useful differentiate the different types of problems a
service provider can report to the consumer.

As it is at the moment it is difficult to write a server that can correctly 
tell a
consumer that it is unauthorised (indicating that it needs to authenticate) vs. 
that
it sent an broken request (indicating that the code needs fixing).

On the spec side, it'd be nice if the standardised error reporting options were 
a bit
more expressive than choice between two HTTP status codes ...

Original comment by james.he...@gmail.com on 16 Jan 2009 at 7:56

GoogleCodeExporter commented 9 years ago
Here's a proposed patch (following) my post to the list (pending moderation, 
maybe).

Original comment by olber...@gmail.com on 28 Mar 2011 at 8:45

Attachments:

GoogleCodeExporter commented 9 years ago
One of the reasons why I never got around to changing all the exceptions to 
include a 400/401 flag was that I never got around to deciding which way was 
better:
1) Having a error-code like you do (makes testing harder, as PHPUnit can only 
test on exception names, not content)
2) Having two different exception classes

I might think that option 2 is preferable and it shouldn't break BC if we just 
introduce two new exceptions that both inherit from the current.. 
Anyways, for future reference - it would be nice if any patch included updates 
to the test-case so they are kept up to date :)

Regards
Morten

Original comment by morten.f...@gmail.com on 29 Mar 2011 at 5:28

GoogleCodeExporter commented 9 years ago
Well... the solution I've used makes it quite easy by calling code to test 
wether there's an exception return code or not, and address this in a proper 
way...

So I don't know about what phpunit supports, but definitely, for the client 
code to have only different strings (5 or 6) to be checked by string comparison 
is definitely not handy.

Do you have an any such alternative implementation ?

And yes, unit tests would be nice... but I think my time is limited, and we 
could wait until such tests are necessary because of some regression I've 
introduced along with my patch ? ;)

Original comment by olber...@gmail.com on 6 Apr 2011 at 7:23