janko / rodauth-omniauth

OmniAuth login and registration for Rodauth authentication framework
MIT License
48 stars 3 forks source link

Getting 'uncaught throw' from `before_login` #19

Open jesseduffield opened 1 month ago

jesseduffield commented 1 month ago

When I throw an error in before_login (or before_omniauth_create_account), I get an uncaught throw error (effectively a 500 error). I expect that instead I'd get a sanitised error surfaced to the user. I don't believe I need to be explicitly catching these errors.

class RodauthAdminPortal < RodauthBase
  ...
  before_login do
    throw_error_status(422, "Test", "Test")
  end
end

Test:

require "rails_helper"

RSpec.describe "Admin authentication", type: :feature do
  describe "login" do
    context "when integration test" do
      before do
        Account.create_admin_account(email: "foo@bar.com")
      end

      fit "logs in" do
        visit "/admin"
        expect(page).to have_current_path("/admin/auth/login")

        click_on "Developer Login"
        expect(page).to have_current_path("/admin/auth/auth/developer")
        fill_in "name", with: "Test User"
        fill_in "email", with: "unknown@bar.com"
        click_on "Sign In"
      end
    end
  end
end

Error:

  1) Admin authentication login when integration test logs in
     Failure/Error: throw_error_status(422, "Test", "Test")

     UncaughtThrowError:
       uncaught throw :rodauth_error
     # ./app/misc/rodauth_admin_portal.rb:27:in `block (2 levels) in <class:RodauthAdminPortal>'
     # ./app/misc/rodauth_app.rb:14:in `block in <class:RodauthApp>'
     # ./spec/integration/admin_authentication_spec.rb:29:in `block (4 levels) in <main>'
     # -e:1:in `<main>'

Rodauth App:

class RodauthApp < Rodauth::Rails::App
  configure RodauthCustomerPortal, :customer_portal
  configure RodauthAdminPortal, :admin_portal

  route do |r|
    # rodauth.load_memory # autologin remembered users (requires the :remember feature)

    # Check the DB for an active session. This allows us to delete active session
    # records in order to forcefully log out a user.
    rodauth(:customer_portal).check_active_session
    rodauth(:admin_portal).check_active_session

    r.rodauth(:customer_portal) # route customer rodauth requests
    r.rodauth(:admin_portal) # route admin rodauth requests
  end
end

Let me know if I'm missing something. Thanks

janko commented 1 month ago

Yeah, the OmniAuth callback route doesn't currently have any validation error handling, since it doesn't really have a form associated (the form is external).

Not sure what would be the correct reaction to the error throw. We could decide to render the login form, but the login form won't work automatically when the URL is different from the login route. We could also redirect with a generic error flash message, but I feel like that would hide the validation error from the developer.

What is your use case for having validation errors on OmniAuth login/registration?

jesseduffield commented 1 month ago

I see. I'm working on this in development at the moment so that's colouring the experience: seeing the big red rails screen explaining the error and the stack trace just strikes me as wrong: but I'm aware this won't show up on prod.

As for the use case, I'm using omniauth for my admin configuration, but I don't want to allow the creation of accounts via omniauth: only logins. And the logins that do happen, I've got some extra validation in there just to make extra sure that the user is the admin I think they are. Admin accounts are created directly in the console rather than through a UI.