[Admin] Add an auth configuration that can be attached to the existing one #5214

elia · 2023-07-03T11:23:56Z

Summary

This PR is not intended for merging (yet)

Having the long term goal of separating the storefront customers form the admin users, would be nice to create the layer of configuration that will then allow us to replace with a different setup when the time comes.

The configuration options were inspired from the extensive experience gathered by ActiveAdmin over the years.

A default setup that taps in the existing solidus_backend authentication is provided and will be helpful during the transition from backend to admin. Eventually solidus_auth_devise can support solidus_admin directly or an alternative system can be provided.

A tentative plan for the great separation ✂️

Make the SolidusAdmin authentication (internal) endpoints configurable, see this PR
While Spree::Backend and SolidusAdmin are coexisting let SolidusAdmin rely on Spree::Backend for authenticating users
Prepare core for a separate user class (e.g. spree_orders.created_by should accept an arbitrary admin class when created by an admin, etc.)
Provide a new default admin-user with independent authentication and eventually drop core support for sharing it in a major
Start building on it adding features like impersonation and cleaning up any ambiguity that's left

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed:

📖 I have updated the README to account for my changes.
📑 I have documented new code with YARD.
🛣️ I have opened a PR to update the guides.
✅ I have added automated tests to cover my changes.
📸 I have attached screenshots to demo visual changes.

codecov · 2023-07-03T11:31:03Z

Codecov Report

Merging #5214 (ced3dd7) into nebulab/admin (56a1ea3) will increase coverage by 0.04%.
Report is 15 commits behind head on nebulab/admin.
The diff coverage is 93.47%.

❗ Current head ced3dd7 differs from pull request most recent head 7cce8ce. Consider uploading reports for the commit 7cce8ce to get more accurate results

@@                Coverage Diff                @@
##           nebulab/admin    #5214      +/-   ##
=================================================
+ Coverage          88.51%   88.55%   +0.04%     
=================================================
  Files                596      598       +2     
  Lines              14511    14554      +43     
=================================================
+ Hits               12844    12888      +44     
+ Misses              1667     1666       -1

Files Changed	Coverage Δ
...controllers/solidus_admin/auth_adapters/backend.rb	`84.21% <84.21%> (ø)`
admin/app/controllers/solidus_admin/auth.rb	`100.00% <100.00%> (ø)`
...n/app/controllers/solidus_admin/base_controller.rb	`100.00% <100.00%> (ø)`
admin/lib/solidus_admin/configuration.rb	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

rainerdema

Thank you, @elia! Having that simple configuration for authentication seems like a good approach to me 👍

admin/lib/solidus_admin/configuration.rb

kennyadsl · 2023-07-04T07:48:38Z

This might make a lot of sense @elia!

Can you please share a high-level overview of how you envision the long-term plan to separate operators and users? I understand this is just the first piece but I miss all the rest of the strategy to leave some feedback that makes sense.

waiting-for-dev

Thanks for raising it now, @elia. We're at the right moment to make solidus admin authentication decoupled from the one for customers, as it'll guide us in making the right architectural decisions. I'd also like to hear about the rest of the plan when that's ready, but this is a significant first step! I left some questions.

waiting-for-dev · 2023-07-04T07:55:56Z

admin/app/controllers/solidus_admin/base_controller.rb

+    include ActiveStorage::SetCurrent
+    include ::SolidusAdmin::Auth
+    include Spree::Core::ControllerHelpers::Store


Why do we need SetCurrent & Store modules in this context? 🤔

Those includes are a trimmed down version of what's included in Spree::BaseController:

ActiveStorage::SetCurrent is needed whenever you use AS, which is the default for images and there's no doubt we'll need to support uploading

Spree::Core::ControllerHelpers::Store is the one providing current_store, I initially removed it, but then it would have required more than a simple helper and so I cut it short and kept it; eventually I'd be more than happy to remove it and be don with any Core::ControllerHelpers 🙌

I understand they'll be used, but are they needed in the context of the PR configuring authentication? 🤔

Been a long time, but I'll add an answer anyway 😊.

Those are not unused, current_store is necessary for the sidebar, while ActiveStorage::SetCurrent is needed in order to generate blob urls, removing it would cause this error (I erroneously thought it was only needed for uploading):

See https://github.com/rails/rails/blob/7-0-stable/activestorage/app/controllers/concerns/active_storage/set_current.rb#L3-L6

admin/lib/solidus_admin/configuration.rb

waiting-for-dev · 2023-07-04T08:00:21Z

admin/app/controllers/solidus_admin/backend_auth_support.rb

+    @current_ability ||= Spree::Ability.new(spree_current_user)
+  end
+
+  def store_location


Where is this method used? 🤔

This is a sad story, it's used by the solidus_auth_devise version of Spree::Admin::BaseController.unauthorized_redirect, usually is provided by the core Auth controller helper along with a bunch of other stuff.

I see. Do you think it's worth it to drop a comment so it's clear for those reading the code?

waiting-for-dev · 2023-07-04T08:01:41Z

admin/app/controllers/solidus_admin/backend_auth_support.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module SolidusAdmin::BackendAuthSupport


Shouldn't we also define the spree_current_user method? Or is it already available from somewhere else?

It's coming from spree_auth_devise

As above, do you think a comment will make things clearer?

admin/app/controllers/solidus_admin/auth.rb

waiting-for-dev · 2023-07-05T04:02:08Z

admin/app/controllers/solidus_admin/auth.rb

+  def authenticate_solidus_admin_user!
+    send SolidusAdmin::Config.authentication_method if SolidusAdmin::Config.authentication_method
+  end


Why not define those methods in the included module adapter?

This file contains the adapter mechanics, providing the hooks that different adapter can use. The backend one is of course the one we need while migrating out of it, but eventually I think other will pop up (devise, maybe 3rd party single sign on, etc.) according to "the plan" in the description.

waiting-for-dev · 2023-07-05T04:03:53Z

admin/lib/solidus_admin/configuration.rb

+    preference :authentication_method, :string, default: :authenticate_solidus_backend_user!
+
+    # The method used to retrieve the current user in the admin interface.
+    preference :current_user_method, :string, default: :spree_current_user
+
+    # The path used to logout the user in the admin interface.
+    preference :logout_link_path, :string, default: :admin_logout_path
+
+    # The HTTP method used to logout the user in the admin interface.
+    preference :logout_link_method, :string, default: :delete


In the same direction as the above comment, if we expect the adapter to define those methods, there's no need to define them in the initializer.

waiting-for-dev · 2023-07-05T04:06:10Z

admin/app/controllers/solidus_admin/base_controller.rb

+    include ActiveStorage::SetCurrent
+    include ::SolidusAdmin::Auth
+    include Spree::Core::ControllerHelpers::Store


I understand they'll be used, but are they needed in the context of the PR configuring authentication? 🤔

kennyadsl · 2023-07-05T07:43:45Z

@elia reading the plan, I have a question. I know it's hard to predict at this stage but if you already wrapped your mind around this, what's the DX for making the switch? I imagine at some point, we will release a rake task that converts all the users that have some admin/operator roles to the other model, with the right roles assigned. That rake task should be run right before the switch of the authentication_adapter preference (otherwise, for example, people could change the roles on the backend side and that not be reflected on the admin side), and after changing that preference, we have to be sure that the functions related to roles are disabled on the backend, to avoid errors (could be that people use the new admin auth for admins with the old backend? 🤯 ).

elia · 2023-07-05T10:54:22Z

could be that people use the new admin auth for admins with the old backend?

I think in this regard we have an opportunity to leverage the new admin for the switch, and the old backend will still run on the spree_users table

So once the apps are fully switched we'll offere a rake task for the migration and preferences, but I think for some time the core will need to still allow using the old table. That shouldn't be a big problem if everything related to admins has stopped assuming it's the same model as customers (that's probably the biggest lift).

… new admin

elia

✅ Thanks for taking care of this @the-krg!

elia · 2023-07-28T15:48:40Z

admin/spec/features/accounts_spec.rb

+  let(:user) { create(:admin_user, email: '[email protected]') }
+
+  before do
+    allow_any_instance_of(SolidusAdmin::BaseController).to receive(:spree_current_user).and_return(user)


I think eventually we'll need a helper method for doing the sign in programmatically, but for now this is ok 👍

rainerdema

Great job! 🥇

rainerdema · 2023-07-31T14:07:43Z

admin/app/views/solidus_admin/accounts/show.html.erb

  class="text-4xl"
 ><a href="#" data-action="hello#greet">Account</a></h1>

-<h2 class="text-3xl underline">Your account info</h1>
+<p>Logged in as <%= current_solidus_admin_user.email %></p>
+<p><%= button_to "Log out", solidus_admin_logout_path, method: solidus_admin_logout_method, class: 'underline' %></p>


This view could be part of a new "account/show" component to be rendered (like "products/index").
Non-blocking for now, I've added a dedicated point in the list of the final things to do before the release:

[Admin] Release SolidusAdmin v0.0.1.alpha.1 #5280

[Admin] Refactor "accounts/show" view into a separate component #5292

waiting-for-dev · 2023-08-07T08:09:37Z

Thanks, @elia & @the-krg, for working on that! This is a very nice step towards having a decoupled authentication system ❤️

I'm only left wondering if there's a benefit to the extra indirection we're adding through the option to configure the adapter method names. If the adapter is configurable, and we expect users to create those interfaces, why not fix the method names?

In short, instead of:

# admin/lib/solidus_admin/configuration.rb
preference :authentication_method, :string, default: :authenticate_solidus_backend_user!
preference :current_user_method, :string, default: :spree_current_user
preference :logout_link_path, :string, default: :admin_logout_path
preference :logout_link_method, :string, default: :delete
preference :authentication_adapter, :string, default: 'SolidusAdmin::AuthAdapters::Backend'

We could only have the following:

preference :authentication_adapter, :string, default: 'SolidusAdmin::AuthAdapters::Backend'

With:

# admin/app/controllers/solidus_admin/auth_adapters/backend.rb
def authentication_method
  authenticate_solidus_backend_user!
end

def current_user_method
  spree_current_user
end

def logout_link_path
  admin_logout_path
end

def logout_link_method
  :delete
end

That's to say: simply require the configured module to implement the expected interface. SolidusAdmin::Auth would also be simplified by not needing to define those methods.

Am I missing some context? Thoughts, @elia, @the-krg, @rainerdema?

elia · 2023-08-07T08:16:47Z

@waiting-for-dev that's a completely valid alternative, and that's why it made sense to pick either the configurations or the adapter (and we picked the configuration).

The reason the configuration was preferred is that generally any existing authentication system won't require any adapter and will have those methods in place (one way or another). The only reason that made auth-devise need an adapter is that weird entanglement of authentication and authorization that we have.

In other words we're building on the experience accrued by active-admin with the following benefits:

auth systems won't need any glue code
auth systems will add (or let you add) those authentication methods to ApplicationControlller or some other base controller
generally with this solution we increase the number of configuration points but we don't need to write any code or patch anything
in the worst case (or in case of a completely custom auth system) it's still easy to plug into it

Hope this makes sense, but happy to answer to more questions 🙌

waiting-for-dev · 2023-08-07T08:30:24Z

Thanks for your answer, @elia! That makes sense. To better understand real-world usage, do you have an example you can share about how other authentication systems different than devise are used in active admin?

elia · 2023-08-07T09:29:49Z

Yes, looking at https://www.ruby-toolbox.com/categories/rails_authentication and picking the most popular ones:

https://doorkeeper.gitbook.io/guides/ruby-on-rails/protecting-your-resources => before_action :doorkeeper_authorize!
authlogic seems to require some manual redirecting by design: https://stackoverflow.com/a/19991617
https://github.com/thoughtbot/clearance#access-control => before_action :require_login
https://github.com/sorcery/sorcery#core => before_action :require_login

waiting-for-dev · 2023-08-07T09:33:25Z

Thanks, @elia. I saw that the option to configure the adapter was removed altogether in another PR. I'm a bit concerned that it will require users to patch SolidusAdmin::BaseController for authentication systems that don't natively integrate with Rails and that probably need some glue code (thinking of https://github.com/jeremyevans/rodauth, but probably there are others). Probably that's something we can change in the future.

elia · 2023-08-07T10:38:27Z

@waiting-for-dev I don't think we need to optimize for every possible authentication system, should be easy to use the popular ones and possible to use anything else.

That said I think even for rodauth the tune is the same:

and you can delegate :require_account, :current_accout, to: :rodauth, prefix: true and configure solidus_admin to use :rodauth_require_account as the authentication method and :rodauth_current_account as the current user method.

elia changed the base branch from main to nebulab/admin July 3, 2023 11:24

github-actions bot added changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus Changes to the solidus meta-gem changelog:repository Changes to the repository not within any gem labels Jul 3, 2023

elia self-assigned this Jul 3, 2023

rainerdema reviewed Jul 3, 2023

View reviewed changes

admin/lib/solidus_admin/configuration.rb Outdated Show resolved Hide resolved

elia requested review from waiting-for-dev and kennyadsl July 3, 2023 17:37

waiting-for-dev reviewed Jul 4, 2023

View reviewed changes

github-actions bot removed changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus Changes to the solidus meta-gem labels Jul 4, 2023

elia force-pushed the elia/admin/auth-config branch 2 times, most recently from 1cde4c0 to 604af8d Compare July 4, 2023 11:25

waiting-for-dev reviewed Jul 5, 2023

View reviewed changes

elia force-pushed the nebulab/admin branch from 2aef522 to 5bc90fc Compare July 11, 2023 08:01

elia force-pushed the elia/admin/auth-config branch from 604af8d to af6b6db Compare July 13, 2023 13:56

elia mentioned this pull request Jul 25, 2023

[Admin] Release SolidusAdmin v0.0.1.alpha.1 #5280

Closed

10 tasks

elia changed the title ~~[Admin] Authentication configuration spike/demo~~ [Admin] Add an auth configuration that can be attached to the existing one Jul 25, 2023

github-actions bot added changelog:solidus_admin and removed changelog:repository Changes to the repository not within any gem labels Jul 27, 2023

elia assigned the-krg Jul 27, 2023

the-krg force-pushed the elia/admin/auth-config branch 3 times, most recently from b532cd9 to ced3dd7 Compare July 28, 2023 06:11

elia added 2 commits July 28, 2023 11:57

Add configuration endpoints for the admin authentication

509efb4

Start out by reusing the solidus_backend authentication method in the…

e084ae7

… new admin

the-krg force-pushed the elia/admin/auth-config branch from ced3dd7 to a53cccf Compare July 28, 2023 15:03

Add authorization stub before running specs

7cce8ce

the-krg force-pushed the elia/admin/auth-config branch from a53cccf to 7cce8ce Compare July 28, 2023 15:08

the-krg marked this pull request as ready for review July 28, 2023 15:25

the-krg requested a review from a team as a code owner July 28, 2023 15:25

elia commented Jul 28, 2023

View reviewed changes

the-krg approved these changes Jul 28, 2023

View reviewed changes

elia requested a review from rainerdema July 31, 2023 08:11

rainerdema approved these changes Jul 31, 2023

View reviewed changes

elia merged commit 4e8c91a into nebulab/admin Jul 31, 2023
4 checks passed

elia deleted the elia/admin/auth-config branch July 31, 2023 14:13

rainerdema mentioned this pull request Jul 31, 2023

[Admin] Refactor "accounts/show" view into a separate component #5292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Admin] Add an auth configuration that can be attached to the existing one #5214

[Admin] Add an auth configuration that can be attached to the existing one #5214

elia commented Jul 3, 2023 •

edited

Loading

codecov bot commented Jul 3, 2023 •

edited

Loading

rainerdema left a comment

kennyadsl commented Jul 4, 2023

waiting-for-dev left a comment

waiting-for-dev Jul 4, 2023

elia Jul 4, 2023

waiting-for-dev Jul 5, 2023

elia Jul 28, 2023

waiting-for-dev Jul 4, 2023

elia Jul 4, 2023

waiting-for-dev Jul 5, 2023

waiting-for-dev Jul 4, 2023

elia Jul 4, 2023

waiting-for-dev Jul 5, 2023

waiting-for-dev Jul 5, 2023

elia Jul 28, 2023

waiting-for-dev Jul 5, 2023

waiting-for-dev Jul 5, 2023

kennyadsl commented Jul 5, 2023

elia commented Jul 5, 2023

elia left a comment

elia Jul 28, 2023

rainerdema left a comment

rainerdema Jul 31, 2023

waiting-for-dev commented Aug 7, 2023

elia commented Aug 7, 2023

waiting-for-dev commented Aug 7, 2023

elia commented Aug 7, 2023

waiting-for-dev commented Aug 7, 2023

elia commented Aug 7, 2023

		@@ -0,0 +1,28 @@
		# frozen_string_literal: true

		module SolidusAdmin::BackendAuthSupport

[Admin] Add an auth configuration that can be attached to the existing one #5214

[Admin] Add an auth configuration that can be attached to the existing one #5214

Conversation

elia commented Jul 3, 2023 • edited Loading

Summary

A tentative plan for the great separation ✂️

Checklist

codecov bot commented Jul 3, 2023 • edited Loading

Codecov Report

rainerdema left a comment

Choose a reason for hiding this comment

kennyadsl commented Jul 4, 2023

waiting-for-dev left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented Jul 5, 2023

elia commented Jul 5, 2023

elia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainerdema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev commented Aug 7, 2023

elia commented Aug 7, 2023

waiting-for-dev commented Aug 7, 2023

elia commented Aug 7, 2023

waiting-for-dev commented Aug 7, 2023

elia commented Aug 7, 2023

elia commented Jul 3, 2023 •

edited

Loading

codecov bot commented Jul 3, 2023 •

edited

Loading