Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Xavier Shay November 11th, 2008 @ 12:24 AM

  Wow, I totally forgot I wrote this

• You flagged this item as spam.
  

  Gregg Kellogg November 11th, 2008 @ 12:54 AM

  Yeah, it's been pretty useful. I have some more updates to the patch, but I'm waiting for my project to complete before I put them there.

  Thanks for getting this started, hopefully it will pass muster for inclusion into the trunk at some point.

  Gregg

• 

  josh December 20th, 2008 @ 05:35 PM

  • Milestone cleared.
  • Assigned user set to “josh”
  • State changed from “new” to “open”
• 

  josh December 20th, 2008 @ 08:15 PM

  Cool, I want to apply this soon but it needs to be rebased against edge.

• You flagged this item as spam.
  

  Gregg Kellogg December 20th, 2008 @ 09:14 PM

  I'll work on an update to the patch now. I have a couple of other fixes I wanted to get in anyway.

• You flagged this item as spam.
  

  Gregg Kellogg December 20th, 2008 @ 11:32 PM

  New patch uploaded.

  • http_digest-2008-12-20.diff 23 KB
• 

  Repository December 28th, 2008 @ 09:14 PM

  • State changed from “open” to “resolved”

  (from [45dee3842d68359a189fe7c0729359bd5a905ea4]) HTTP Digest authentication [#1230 state:resolved] http://github.com/rails/rails/co...

• 

  Repository January 13th, 2009 @ 04:16 PM

  (from [c99ef814b0ce5d6b6a677ee6116edac03c8a35b3]) Revert "HTTP Digest authentication [#1230 state:resolved]"

  This reverts commit 45dee3842d68359a189fe7c0729359bd5a905ea4.

  Reasons :

  1. The code is not working in it's current state
  2. Should not be using exceptions for flow control http://github.com/rails/rails/co...
• 

  Pratik January 13th, 2009 @ 05:41 PM

  • Tests shouldn't use mocking. Just use normal integration tests.
  • ActionController::HttpAuthentication::Digest::ControllerMethods should get included somewhere.
  • Object#send! is long gone.
  • Shouldn't use exceptions for flow control
• 

  Pratik January 13th, 2009 @ 05:42 PM

  • State changed from “resolved” to “open”
  • Assigned user changed from “josh” to “Pratik”

  Btw, thanks for the nice work. I didn't mean to be discouraging by reverting the patch. We should definitely try to have it in for 2.3.

  Thanks!

• You flagged this item as spam.
  

  Gregg Kellogg January 13th, 2009 @ 07:09 PM

  Here is a new patch that addresses the issues raised:

  • Replace controller.send! with controller.send, mirroring the same case in Basic authentication
  • Refactor exception control flow in authenticate_or_request_with_http_digest.
  • Minimal changes to tests, simply to mirror the basic authentication test, which also uses a Mock controller.
  • Include HttpAuthentication::Digest::ControllerMethods in base.rb

  • http_digest-2009-01-13.diff 20.8 KB
• 

  Pratik January 13th, 2009 @ 07:14 PM

  I'm strongly in favor of removing mocking from these tests. It should be removed from basic authentication tests too, but that's not in the scope of this ticket.

  Also, "raise Error.new" should be gone.

  Thanks!

• You flagged this item as spam.
  

  Gregg Kellogg January 13th, 2009 @ 07:37 PM

  Exceptions are used within the body of Digest Authentication, and presented to the application level, as the nature of the encoded digest is dense, and it's useful for the library to provide the application with more detail as to the nature of the failure. The alternative would be to do a simple return of true/false and provide a separate analysis interface to help determine why an authentication attempt has failed. This seems like a reasonable use of exceptions, and is certainly used elsewhere within Rails. Is there some style-guide that relates to this that I'm unaware of?

  If you'd like to eliminate the unit tests, which require the use of a Mock controller, I can integrate the elements tested with the Integration tests in integration_test.rb.

• You flagged this item as spam.
  

  Gregg Kellogg January 14th, 2009 @ 05:44 AM

  This patch refactors HTTP Digest to remove exception usage. It adds http_digest_authentication_response_detail to be able to determine the cause of an authentication failure without changing the basic pattern used by Basic authentication.

  Unit tests still use Mock controller.

  • http_digest_2009-01-13.1.patch 33.1 KB

• You flagged this item as spam.
  

  Gregg Kellogg January 15th, 2009 @ 08:17 PM

  Fixed bug in http_digest_authentication_response_detail.

  • http_digest_2009-01-15.patch 35 KB

• You flagged this item as spam.
  

  Gregg Kellogg January 27th, 2009 @ 02:44 AM

  Here’s an updated patch. I ended up making some slight modifications to the digest code. Tests are now pretty much along the lines of Basic authentication.

  • http_digest_2009-01-19.patch 25.8 KB
• 

  Pratik January 29th, 2009 @ 01:37 AM

  Hey Gregg,

  
      def authenticate
        authenticate_or_request_with_http_digest("Super Secret") do |username, password|
          username == "hello" && 'world'
        end
      end
  

  The "hello && world" part is misleading. If the block is only concerned about the username, why is it passed the password too ?

• You flagged this item as spam.
  

  Gregg Kellogg January 29th, 2009 @ 02:16 AM

  Yes, this is a vestage from the basic use case. authenticate_or_request_with_http_digest and authenticate_with_http_digest only pass the username parameter. You can remove the password parameter from these tests.

  
  def authenticate
    authenticate_or_request_with_http_digest("Super Secret") do |username|
      username == "hello" && 'world'
    end
  end
  

• 

  Pratik January 29th, 2009 @ 02:42 AM

  Thanks. I'll make that modification.

• 

  Repository January 29th, 2009 @ 04:02 PM

  • State changed from “open” to “resolved”

  (from [306cc2b920203cfa51cee82d2fc452484efc72f8]) Implement HTTP Digest authentication. [#1230 state:resolved] [Gregg Kellogg, Pratik Naik]

  Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  Don Parish February 2nd, 2009 @ 09:24 PM

  Found a problem while trying to use http digest authentication at http://ryandaigle.com/articles/2.... The digest authentication failed using both IE and FireFox. The current implementation is using the actual URI from the request instead of the URI passed in the authentication header, which is stored in the credentials hash. It seems as if the client is responsible for the URI passed in. It could be an absolute URI, as suggested in http://tools.ietf.org/html/rfc2617, but every example I've seen, only the relative path is used.

  With the attached change, the tests pass, and the digest authentication work with my test controller using IE and FireFox on Windows.

  • http_digest_authentication_fix.diff 1.2 KB
• 

  Pratik February 2nd, 2009 @ 11:14 PM

  Hey Donald,

  A test would be very helpful to ensure the same issue doesn't happen again.

• You flagged this item as spam.
  

  Don Parish February 4th, 2009 @ 09:49 PM

  • Tag changed from “actionpack, authentication, controller, digest, http_authentication” to “actionpack, authentication, bug, controller, digest, http_authentication, patch”

  Pratik,

  Patch with test attached that will fail until fix is applied. Then, all tests pass. I had to modify the encode_credentials method so I could override the absolute URI it normally uses with a a relative path.

  • http_digest_authentication_fix.diff 2.8 KB
• 

  Pratik February 4th, 2009 @ 09:53 PM

  • State changed from “resolved” to “open”
• 

  Pratik February 16th, 2009 @ 07:23 PM

  • State changed from “open” to “resolved”

• You flagged this item as spam.
  

  Don Parish February 17th, 2009 @ 05:51 PM

  Pratik,

  Thanks for committing my patch last night, but I'm afraid I made a mistake in that patch that is fixed by this one. RFC 2617, Section 3.2.2.5, says that the authenticating server must ensure the document designated by the uri parameter is the same as the document served. To do this, the uri in the credentials should be compared to the uri in the request line.
  
  

  This patch does that. The prior implementation before my first patch, passed in the full URL instead of the URI, that's why the tests passed, but it failed with most browsers which send a uri without the full URL.

  I've added two tests. One that makes sure the uris in the request and credentials match, whether using a url or relative uri. The other test checks to see that the authorization will fail if the header uri is changed.

  Sorry about my mistake (and on my first commit :( ). Just wanted to make sure you had a chance to fix it before Rails 2.3 is released.

  Don

  P.S. It is easy to modify this implementation to handle passwords stored as a hash: MD5(username:realm:password). Is it too late to make a patch for this?

  Being able to store a hashed version on the server instead of a plaintext password is one reason Digest is so complicated.

  • http_digest_authentication_fix_uri.diff 5.3 KB
• 

  Pratik February 17th, 2009 @ 05:55 PM

  Don,

  Can you please open a new ticket and assign to me ?

  Thanks.

• You flagged this item as spam.
  

  Don Parish February 18th, 2009 @ 01:00 PM

  Pratik, Added ticket with inital patch. Later attached an updated patch to support use of storing a hash of user credentials instead of a plain text password, like htdigest in Apache. Let me know if that needs to be its own ticket. Thanks.

• You flagged this item as spam.
  

  Jacob Burkhart July 24th, 2009 @ 06:22 PM

  Any activity going on in fixing this? Just found out today that http auth is broken. What's the active ticket for fixing it? Need patches or +1's ?

  a little test that exposes the problem

  
      username_to_encode = "username_username_username"
      password_to_encode = "password_password_password"
      
      encoded_to = ActionController::HttpAuthentication::Basic.encode_credentials(username_to_encode, password_to_encode)    
      req_thing = OpenStruct.new(:env => {})
      req_thing.env['HTTP_AUTHORIZATION'] = encoded_to
      username_decoded, password_decoded = ActionController::HttpAuthentication::Basic.user_name_and_password(req_thing)
      
      message = "Expected #{username_to_encode}, #{password_to_encode} but got #{username_decoded} , #{password_decoded}"
      assert_equal(username_to_encode, username_decoded, message)
      assert_equal(password_to_encode, password_decoded, message)
  

  I see the fix was taken out in http://github.com/rails/rails/commit/c99ef814b0ce5d6b6a677ee6116eda... which references this ticket

  
         def decode_credentials(request)
  -        # Properly decode credentials spanning a new-line
  -        auth = authorization(request)
  -        auth.slice!('Basic ')
  -        ActiveSupport::Base64.decode64(auth || '')
  +        ActiveSupport::Base64.decode64(authorization(request).split.last || '')
         end
  

  How do we get this fix back in?

• You flagged this item as spam.
  

  Jacob Burkhart July 24th, 2009 @ 06:44 PM

  So it was fixed in a later commit... but it doesn't appear when I look at it in github... maybe there's just a bug in github?
  

  http://github.com/rails/rails/blob/master/actionpack/lib/action_con... shows the old code...

  Yet
  

  $ git branch -r --contains 306cc2b920203cfa51cee82d2fc452484efc72f8
    origin/2-3-stable
    origin/3-0-unstable
    origin/HEAD
    origin/master
  

  clearly shows the commit fixing it is in master... maybe there was a problem merging? How do I check which tags the commit made it into?