janko / rodauth-rails

Rails integration for Rodauth authentication framework
https://github.com/jeremyevans/rodauth
MIT License
565 stars 40 forks source link

[WIP] Overhaul generators #226

Closed thedumbtechguy closed 10 months ago

thedumbtechguy commented 10 months ago

It is quite early here and I've been hacking away at this continuously over the weekend, so my description is going to be sparse until I get some rest.

First of all, thank you for an such an amazing project. Taking an awesome framework like rodauth and making it easy to use on rails is to be applauded.

I have been working on a set of generators to quickly bootstrap apps and I arrived at rodauth-rails because I want to use rodauth (obviously why not?). I found quite a number of issues with the generators. They were not quite right. They worked for a basic setup, but for someone new to rodauth, it was quite difficult to get things kicked off. There were some unused options, some hard coded values in some places which would break if users passed in an unexpected value. Some confusion wrt how jwt, json and only_json interact. Most especially, I needed this for a multi-user setup and unfortunately, that was not to be offered.

So rather than write the generators in my own project, I felt the best thing to do would be to improve upon the work you've already done and then reuse the updated generators in mine.

So basically, I overhauled it

There is still a lot more work to be done to make this superb. For e.g. I am yet to work with views and figure out if they work as expected in a multi-user setup. Tests and documentation are yet to be updated. Split up mailers per account?

I also plan on adding options for some specific setups, e.g. email auth only

I am only submitting this PR this early so I can gather some feedback as I proceed with it. Is this useful to you? Do you see it being merged upon completion?

If you'd like to take a look, my generators project is https://github.com/plutonium-rails/plutonium-generators

thedumbtechguy commented 10 months ago

So mailers have been split up too. Views worked so I slightly modified that and incorporated it into the account generation process. I've added a centralized generator plugin config to make management better.

My auto formatter mangled some files, so I'll have to fix that. I recommend you run rubocop in the entire project to prevent future occurrences.

I'll be moving on to tests and documentation now.

janko commented 10 months ago

I appreciate the pull request, though I have to admit it's bit overwhelming. Could you describe on more details the specific problems you're aiming to solve? There seem to be a lot of different changes and refactoring here, and I didn't understand many of the points such as "streamlined options and arguments" and "ensured each plugin is configured appropriately". What is "standalone account generation", what's the use case for skipping base model creation?

When you talked about multi-user setup, what exactly were you missing? The migration generator supports different table names (in case secondary Rodauth configurations use different tables), while the view generator accepts a Rodauth configuration name. The mailer is configured to automatically work with multiple configurations, though I agree in the long run you probably want to have different mailers.

Automatically update database overrides after migration. No need for user action.

How are you able to do that? How do you know which Rodauth configuration the migration applies to, and where it's located? The developer has the freedom to define Rodauth configuration however they want, even use inheritance to share behavior. What if you have RodauthMain < RodauthBase and RodauthAdmin < RodauthBase, and the migration overrides need to go in RodauthBase < Rodauth::Rails::Auth which isn't registered in the Rodauth app?

Whatever functionality generators provide, it always needs to work regardless of where the files are placed. That's the rule of thumb that was guiding me in the current generator implementation.

I also plan on adding options for some specific setups, e.g. email auth only

I'm not sure I would be OK with the complexity needed to offer such an option, because a lot would need to change, right? It seems better that these setups are covered by tutorials.

thedumbtechguy commented 10 months ago

@janko I am happy to address your questions, but doing so via writing might be difficult. Do you mind if I made a screencast instead?

janko commented 10 months ago

@thedumbtechguy Not at all, I think that would be great 😉

thedumbtechguy commented 10 months ago

I have uploaded the video here. Tried to make it short but it ended up about 19 minutes.

https://youtu.be/NFaz4-eC8G4

janko commented 10 months ago

One of the features of this PR I would like to have is the ability to select Rodauth features in the rodauth:install generator. I could never fully visualize how that would be implemented, so I didn't attempt it yet. I'm also open to the --jwt option automatically adding the jwt gem to the Gemfile, as well as --argon2 adding the argon2 gem.

I don't like the idea of having a CLI option for each Rodauth feature. It bloats the option list, and the metaprogramming required seems to add a lot of complexity. What was the reason for deciding for this instead of a --features array option?

Note that Rodauth configurations are not "plugins". Rodauth already has plugins, which it calls "features". A Rodauth configuration is class with fully defined authentication logic, the only thing it "plugs into" is the Roda app, but technically it can be used standalone as well (typically through internal requests).

Having the migration generator automatically insert table/column overrides into the Rodauth configuration only when it's defined at the expected *_plugin.rb location is a no-go to me. This means the behavior will depend on where your configuration is defined, and it will suggest that you should follow certain naming conventions that I don't want to impose.

Regarding the --passwordless option, it's currently opinionated that it's referring to email authentication, but Rodauth also supports passwordless authentication via passkeys (WebAuthn). So, I don't think a --passwordless option would make sense for rodauth-rails, because it would have to impose an opinion what "passwordless" means exactly. I think that would open a slippery slope, where another person might suggest adding a --passkeys option or a --mfa option, which would be hard to define and later maintain. I want generator options to have exact meaning.


I'm open to incremental changes, as opposed to overhauling everything. As I mentioned, I would accept PRs where --jwt and --argon2 options add jwt and argon2 gems to the Gemfile, and an option for specifying a list of Rodauth features on installation.

janko commented 10 months ago

I also wouldn't like for rodauth:install generator to automatically generate view templates, as that number of files generated can be overwhelming for a newcomer. I was even contemplating moving mailer configuration back into a separate generator, because I already think the current number of files are on the edge.

A person can easily generate views for their currently enabled features by running rails generate rodauth:views without any arguments. Since it's just one extra simple command, I don't see a benefit in running it automatically on rodauth:install.

janko commented 10 months ago

Regarding the rodauth:account generator, I don't think we can really make assumptions how a new Rodauth configuration will look like. The developer might want to use dedicated tables, or just a type column (see wiki page). They might want to use the same session prefix, or have the session data separate to potentially allow both account types to be logged in at the same time. Also, they should have some configuration portion shared through a superclass, but we cannot determine which one.

Once rodauth:install sets up one configuration, I think it's much better that the developer sets up other one manually by copy-pasting or moving what they need. I don't see the value in a dedicated generator for this.

Since I don't want to accept the changes as they're currently proposed, I'll close this pull request. As I said, I'm open to the mentioned subset of these changes, added incrementally as individual PRs.

thedumbtechguy commented 10 months ago

Thanks for your time.