Always challenge a user with no/bad credentials #359

imtayadeway · 2018-04-05T18:23:15Z

According to RFC 2617:

The 401 (Unauthorized) response message is used by an origin server
to challenge the authorization of a user agent. This response MUST
include a WWW-Authenticate header field containing at least one
challenge applicable to the requested resource.

We currently only issue a challenge when no credentials are provided -
if a user fails authentication they are not challenged again. This
broken implementation can cause some frustration for instance when
using the web browser, which will cache a mis-typed user/password
combination and not challenge the user again.

This is somewhat problematic as we only use the Authorization header
for Basic Authentication. It seems like a safe compromise for the user
to be challenged to use Basic Authentication if authentication fails
for whatever reason.

According to RFC 2617: > The 401 (Unauthorized) response message is used by an origin server to challenge the authorization of a user agent. This response MUST include a WWW-Authenticate header field containing at least one challenge applicable to the requested resource. We currently only issue a challenge when no credentials are provided - if a user fails authentication they are not challenged again. This broken implementation can cause some frustration for instance when using the web browser, which will cache a mis-typed user/password combination and not challenge the user again. This is somewhat problematic as we only use the `Authorization` header for Basic Authentication. It seems like a safe compromise for the user to be challenged to use Basic Authentication if authentication fails for whatever reason.

Fryguy · 2018-04-13T17:41:34Z

app/controllers/api/base_controller/authentication.rb

+            rescue MiqException::MiqEVMLoginError => e
+              raise AuthenticationError, e.message
+            end
+          end or raise AuthenticationError


why the or? that's generally a no-no

That's just a Perl-ism - || raise isn't valid Ruby, so the alternative is to make this (perhaps) less readable by assigning to a variable that's only used to check that this is non-nil

honestly, I find the variable more readable...or use tap (I do find tap less readable).

See also https://github.com/bbatsov/ruby-style-guide#no-and-or-or

|| is valid Ruby, so not sure why you think it's not...maybe you are using it without parenthesis?

[1] pry(main)> false || raise("blah") RuntimeError: blah from (pry):20:in `<main>' [2] pry(main)> false || raise "blah" SyntaxError: unexpected tSTRING_BEG, expecting keyword_do or '{' or '(' false || raise "blah" ^

Ah yes, I must have misinterpreted that error, but I find having to know that that raise requires parentheses perhaps a bit arcane. I'll change to use the variable.

FWIW I prefer Avdi's take on the no or/and rule 😉

Fryguy · 2018-04-13T17:50:48Z

app/controllers/api/base_controller/authentication.rb

+            rescue MiqException::MiqEVMLoginError => e
+              raise AuthenticationError, e.message
+            end
+          end or raise AuthenticationError


since the "guts" of the other legs of the if clause are methods, can we make this a method as well? Maybe authenticate_with_basic_auth?

Yeah I considered this, but then it's not clear to me at a glance how that method (or any similarly-named method following the scheme above) differs crucially from the one from Rails' Authentication library (authenticate_with_http_basic) so I felt that might be better dealt with in a follow up?

Fryguy · 2018-04-13T17:52:17Z

app/controllers/api/base_controller/authentication.rb

+            rescue MiqException::MiqEVMLoginError => e
+              raise AuthenticationError, e.message
+            end
+          end or raise AuthenticationError
        end
        log_api_auth


Strangely this only logged successful logins, but do we log unsuccessful logins anywhere?

We have never logged an unsuccessful basic authentication login....it looks like we might lose some logging around the other cases. I'll try to rectify that

This also restores the existing logging that was present for failures in the other strategies.

abellotti · 2018-04-16T22:38:54Z

app/controllers/api/base_controller/authentication.rb

+      rescue AuthenticationError => e
+        api_log_error("AuthenticationError: #{e.message}")
+        response.headers["Content-Type"] = "application/json"
+        request_http_basic_authentication("Application", ErrorSerializer.new(:unauthorized, e).serialize.to_json)


We're missing the logging of the 401,

before this PR, we get the following:

[----] I, [2018-04-16T17:48:16.502716 #26595:3fdeb52eb958] INFO -- : MIQ(Api::ApiController.log_request_initiated) [----] I, [2018-04-16T17:48:16.502900 #26595:3fdeb52eb958] INFO -- : MIQ(Api::ApiController.log_request) API Request: {:requested_at=>"2018-04-16 21:48:16 UTC", :method=>"GET", :url=>"http://localhost:3000/api"} [----] E, [2018-04-16T17:48:16.571327 #26595:3fdeb52eb958] ERROR -- : MIQ(Api::ApiController.api_error) API Error [----] E, [2018-04-16T17:48:16.571381 #26595:3fdeb52eb958] ERROR -- : MIQ(Api::ApiController.api_error) MiqException::MiqEVMLoginError: Authentication failed [----] I, [2018-04-16T17:48:16.571986 #26595:3fdeb52eb958] INFO -- : MIQ(Api::ApiController.log_request) Response: {:completed_at=>"2018-04-16 21:48:16 UTC", :size=>"0.108 KBytes", :time_taken=>"0.069 Seconds", :status=>401}

The last Response: entry is missing. This stuff is handled by api_error, so instead of adding more of that logic here, maybe the above can be as follows:

rescue AuthenticationError => e api_error(:unauthorized, e) request_http_basic_authentication("Application", ErrorSerializer.new(:unauthorized, e).serialize.to_json) end

Or possibly get api_error to return to the response generated

rescue AuthenticationError => e request_http_basic_authentication("Application", api_error(:unauthorized, e).to_json) end

Interesting. It looks like request_http_basic_authentication is short-circuiting the after_action that invokes this - it's not specific to api_error. Will look into it.

Addressed in 9fe4247

Thanks @imtayadeway for the update.

abellotti · 2018-04-16T22:47:41Z

app/controllers/api/base_controller.rb

@@ -37,8 +37,6 @@ class BaseController < ActionController::API
    rescue_from(ActiveRecord::StatementInvalid) { |e| api_error(:bad_request, e) }
    rescue_from(JSON::ParserError)              { |e| api_error(:bad_request, e) }
    rescue_from(MultiJson::LoadError)           { |e| api_error(:bad_request, e) }
-    rescue_from(MiqException::MiqEVMLoginError) { |e| api_error(:unauthorized, e) }
-    rescue_from(AuthenticationError)            { |e| api_error(:unauthorized, e) }


I see AuthenticationError raised from within this file so that's ok to remove. I'm not sure about MiqEVMLoginError in the other 2 token based auth. Should be verified. If that error can be raised, we'd need to rescue MiqException::MiqEVMLoginError for those cases too.

I think this is OK because:

I see tests that cover the happy/unhappy paths around authenticating with tokens

System token authentication does a catch-all rescue, and that specific error inherits from StandardError

User token authentication doesn't do any actual logging-in as such, it just talks to the token manager

miq-bot · 2018-04-17T15:35:11Z

Checked commits imtayadeway/manageiq-api@4ebe167~...9fe4247 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

abellotti · 2018-04-23T13:26:10Z

LGTM!! Thanks @imtayadeway for fixing this. 👍

himdel · 2018-04-24T18:26:04Z

(summarizing gitter ☝️ April 24, 2018 3:24 PM)

Turns out the change in app/controllers/api/base_controller/authentication.rb changes what happens when validate_user_identity fails.

Before, it would not prevent the user from logging in, but now that it is being run inside a authenticate_with_http_basic do ... block, it does.

Turns out logging in as a newly created user now fails with User's Group is missing (#366) because their current_group is not set yet.

himdel · 2018-08-21T12:16:49Z

Looks like this is a problem...

DELETE /api/auth with x-auth-token will request HTTP Basic Authentication if that token already expired.
So, trying to log out with an expired token causes the popup.

There seems to be no way of disabling the popup on the browser side, so this may need to be undone.
(Or the UI needs to keep track of the token validity, which seems too error-prone.)

himdel · 2018-10-10T12:21:10Z

Login dialog popup problem addressed in #488

imtayadeway added the bug label Apr 5, 2018

imtayadeway assigned abellotti Apr 5, 2018

imtayadeway requested a review from Fryguy April 5, 2018 18:23

Fryguy reviewed Apr 13, 2018

View reviewed changes

imtayadeway added 3 commits April 16, 2018 11:31

Prefer temp to thing or raise

de6feb6

Add logging for basic authentication failure

d85141c

This also restores the existing logging that was present for failures in the other strategies.

Extract timeout temp to appease codeclimate

1aa0675

abellotti reviewed Apr 16, 2018

View reviewed changes

Log response after halting with auth challenge

9fe4247

abellotti approved these changes Apr 23, 2018

View reviewed changes

abellotti added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 23, 2018

abellotti merged commit 97eea82 into ManageIQ:master Apr 23, 2018

imtayadeway mentioned this pull request May 3, 2018

Unable to login with a newly created user #366

Closed

tadeboro mentioned this pull request Sep 28, 2018

Sign-in pop-up ManageIQ/manageiq-ui-classic#4717

Closed

himdel mentioned this pull request Oct 10, 2018

Don't request HTTP Basic authenticaion when using a token #488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always challenge a user with no/bad credentials #359

Always challenge a user with no/bad credentials #359

imtayadeway commented Apr 5, 2018

Fryguy Apr 13, 2018

imtayadeway Apr 13, 2018

Fryguy Apr 13, 2018

imtayadeway Apr 13, 2018

Fryguy Apr 13, 2018

imtayadeway Apr 13, 2018

Fryguy Apr 13, 2018

imtayadeway Apr 13, 2018

abellotti Apr 16, 2018

imtayadeway Apr 17, 2018

imtayadeway Apr 17, 2018

abellotti Apr 17, 2018

abellotti Apr 16, 2018

imtayadeway Apr 17, 2018

miq-bot commented Apr 17, 2018

abellotti commented Apr 23, 2018

himdel commented Apr 24, 2018 •

edited

Loading

himdel commented Aug 21, 2018

himdel commented Oct 10, 2018

Always challenge a user with no/bad credentials #359

Always challenge a user with no/bad credentials #359

Conversation

imtayadeway commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Apr 17, 2018

abellotti commented Apr 23, 2018

himdel commented Apr 24, 2018 • edited Loading

himdel commented Aug 21, 2018

himdel commented Oct 10, 2018

himdel commented Apr 24, 2018 •

edited

Loading