Skip to content

Conversation

@mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Oct 14, 2025

Solidus ships an admin users controller within the core backend distribution, while Alchemy does this within the auth extension alchemy-devise. So when using alchemy-devise with solidus-backend, Solidus` admin users controller takes over, and we need to make sure our non-users has the right abilites to create a first admin user, and that that admin user then has admin rights, too.

In a previous comment we were discussing whether to use Devise.authentication_keys - however, that method also has :subdomain in its example, and I'm unsure exactly how it works. I'd rather explicitly set :login, which does the right thing in 99.99% of all cases, and is clearer.

@mamhoff mamhoff mentioned this pull request Oct 14, 2025
@mamhoff mamhoff force-pushed the interop-with-solidus branch from 5e24cd9 to 0bbbb64 Compare October 14, 2025 11:36
@tvdeyen
Copy link
Member

tvdeyen commented Oct 15, 2025

In a previous comment we were discussing whether to use Devise.authentication_keys - however, that method also has :subdomain in its example, and I'm unsure exactly how it works. I'd rather explicitly set :login, which does the right thing in 99.99% of all cases, and is clearer.

:login is not part of devises defaults. Alchemy::User adds this attribute and alchemy-devise generates a devise.rb initializer which has Devise.authentication_keys configured to include the attribute for bootstrapped projects. Shops are able to configure it in their app. If we do not use those configured keys, we end up with the signup not working for Shops changing that to ie. username (email should already in the list auf permitted attributes). Not saying that this is an actual issue, but still we need a way tackle this.

@tvdeyen
Copy link
Member

tvdeyen commented Oct 15, 2025

however, that method also has :subdomain in its example

I do not understand this comment. Can you elaborate?

@mamhoff
Copy link
Contributor Author

mamhoff commented Oct 15, 2025

however, that method also has :subdomain in its example

I do not understand this comment. Can you elaborate?

In the generated initializer, I found this comment:

You can configure it to use [:username, :subdomain], so for authenticating a user, both parameters are required.

Would both of these be ActiveRecord attributes? Do we also want to safelist :subdomain?

@mamhoff mamhoff force-pushed the interop-with-solidus branch from 0bbbb64 to b7cbbaf Compare October 15, 2025 12:12
@tvdeyen
Copy link
Member

tvdeyen commented Oct 15, 2025

You can configure it to use [:username, :subdomain], so for authenticating a user, both parameters are required.

Would both of these be ActiveRecord attributes? Do we also want to safelist :subdomain?

I am pretty sure that both are ActiveRecord attributes (the subdomain probably being an example of a multi tenant website) and yes, we would need to allow both attributes to be updated in the admin controller then.

What concerns me more is the next paragraph:

You can also supply a hash where the value is a boolean determining whether or not authentication should be aborted when the value is not present.

🙈 I wasn't aware of this, but we can neglect this I guess. I never saw this in a real app and we can tackle this if this causes any actual issues.

@@ -0,0 +1,24 @@
# /home/anselm/code/alchemy-solidus/app/patches/controllers/alchemy/solidus/spree_admin_users_controller_patch.rb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove this

end
end

::Spree::Admin::UsersController.prepend(Alchemy::Solidus::SpreeAdminUsersControllerPatch)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably want to safe guard?

Solidus ships an admin users controller within the core backend
distribution, while Alchemy does this within the auth extension
`alchemy-devise`. So when using `alchemy-devise` with `solidus-backend`,
Solidus` admin users controller takes over, and we need to make sure our
non-users has the right abilites to create a first admin user, and that
that admin user then has admin rights, too.
@tvdeyen tvdeyen force-pushed the interop-with-solidus branch from b7cbbaf to 7904616 Compare October 16, 2025 10:48
@tvdeyen
Copy link
Member

tvdeyen commented Oct 16, 2025

@mamhoff rebased with latest main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants