Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrating Rails 8 authentication with Avo #3361

Open
4 of 11 tasks
matiaskorhonen opened this issue Oct 24, 2024 · 10 comments · May be fixed by #3376
Open
4 of 11 tasks

Integrating Rails 8 authentication with Avo #3361

matiaskorhonen opened this issue Oct 24, 2024 · 10 comments · May be fixed by #3376

Comments

@matiaskorhonen
Copy link

Describe the bug

I'm trying to figure out how to integration Rails 8's new built-in/generated authentication with Avo and it's a bit of a struggle.

Steps to Reproduce

Steps to reproduce the behavior:

  1. Create a new Rails 8 app (rails new avo-rails-8-auth)
  2. Install Avo as usual (bin/rails app:template LOCATION='https://avohq.io/app-template')
  3. Set up authentication (bin/rails generate authentication && rails db:migrate)
  4. Somehow configure Avo to authenticate using the methods in the Authentication concern (app/controllers/concerns/authentication.rb)

Expected behavior & Actual behavior

Configuring the authentication in Avo is unintuitive. Maybe the Avo application controller needs to be monkey patched to include the Authentication concern? I tried doing that but couldn't get it to actually work…

Models and resource files

Reproduction repo at: https://github.com/matiaskorhonen/avo-rails-8-auth

System configuration

Avo version: 3.13.7

Rails version: 8.0.0.rc1

Ruby version: 3.3.5

License type:

  • Community
  • Pro
  • Advanced

Are you using Avo monkey patches, overriding views or view components?

  • Yes. If so, please post code samples.
  • No

Screenshots or screen recordings

Additional context

Impact

  • High impact (It makes my app un-usable.)
  • Medium impact (I'm annoyed, but I'll live.)
  • Low impact (It's really a tiny thing that I could live with.)

(High impact but on a project that doesn't matter)

Urgency

  • High urgency (I can't continue development without it.)
  • Medium urgency (I found a workaround, but I'd love to have it fixed.)
  • Low urgency (It can wait. I just wanted you to know about it.)
@adrianthedev
Copy link
Collaborator

I'm having a look into this one.
The quick quick fix is to add this somewhere (I adde it at the end of avo.rb intitializer), but I'm working to support this use case easier.

Rails.configuration.to_prepare do
  Avo::ApplicationController.include Authentication
end

@davidlormor
Copy link
Contributor

@matiaskorhonen @adrianthedev I was just hacking on a new Rails 8 app tonight and decided to give it a shot...MAN, what a pain. The initial solution from Adrian worked great, but everything blew up as soon as I added Pundit for authorization. I spent hours trying to figure out why my policy scopes were receiving nil for the user argument.

Turns out that the before_action :require_authentication wasn't firing before Avo tried to look up the current user. I swapped it to prepend_before_action :require_authentication and it's all working now 😅

That's probably a safe change for the new Rails 8 auth template anyways, as you probably want to authenticate a user before attempting to do anything else in most cases.

@adrianthedev
Copy link
Collaborator

Yeah, we need to do more work on the default Rails authorization generator.

I started that and it's not straightforward once you see that there is no real indicator that the user is using the Rails generator.
I mean, with devise, it was pretty straighforward to detect if the gem was installed or not.

I'll hack more on this so we have it properly supported by the next Avo release.

... And hopefully not broken by the next Rails release 😅

@davidlormor
Copy link
Contributor

@adrianthedev - yeah, since it's essentially a template to roll your own auth, it makes sense that there's no way to detect the config from the Avo side. Seems like the simplest thing to do for now, might be to have some sort of flag for "use rails auth" that would include the Authentication module before any other modules in the base Avo controller. From there, an Avo user should be able to use the existing current_user_method configs in Avo to make it work. Just a thought 🤷

Happy to hack on it with you at some point 😄

@matiaskorhonen
Copy link
Author

I haven't tested this out but perhaps one choice would be to have some sort of more generic way of including modules in the Avo base controller. Since it feels like there are difficulties when using any authentication besides just Devise…

Maybe something like:

Avo.configure do |config|
  config.controller_includes = [Authentication]
  # config.current_user_method = …
  # and so forth
end

@adrianthedev
Copy link
Collaborator

We thought about that solution @matiaskorhonen, but the fact of the matter is that we'll build a mechanism to take that config, store it, and then dynamically include it, when, in reality, if you just use the ApplicationController.include solution you only add two more lines of code.
Plus, with the more verbose version, you can control the narrative much better. You can include or prepend modules

config.controller_includes = [Authentication]

# versus
Rails.configuration.to_prepare do
  Avo::ApplicationController.include Authentication
end

And in the end it doesn't solve the problem with the order of the actions.

@adrianthedev
Copy link
Collaborator

Hey @davidlormor.

I can't replicate that behavior.
What I did was:

  1. rails new
  2. generate authentication
  3. add Avo
  4. add avo-pro
  5. add resource and policy
  6. ...

What should be broken? I can navigate Avo and all the policy methods are applied.

@adrianthedev
Copy link
Collaborator

I did some work to support this better in #3376 and #3377.

@davidlormor
Copy link
Contributor

@adrianthedev hmmm...sounds like the same general order of operations I used to set things up. I've had to yank out our Avo authorization for now (realized I had access to Avo pro/advanced through my other company's key, but I had to remove that... 😉).

From what I remember, Avo was trying to run the authorization check before Current.user was set from the Authentication module. Maybe it had something to do with using the self.includes on the resource and those authorization calls were happening before the Current.user was loaded?

Anyways, happy to continue looking into this, but I'd need some help with a license for local testing...

@davidlormor
Copy link
Contributor

@adrianthedev just took a look at the PRs you tagged, from what I remember the issue seemed to be coming from the set_authorization callback...because the before_action :require_authentication hadn't run, the Current.user hadn't been set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

3 participants