Fix locale inconsistence and remove redundant template

bitberry-dev commented 2 years ago

Hello guys,

I was removing a devise from one of my projects and came across a few of my notes.

When the language is changed on the backend, devise's backend controllers do not react to this, because set_user_language_locale_key is not set :admin_locale
The template spree/layouts/admin/login_nav is not used anywhere https://github.com/solidusio/solidus_auth_devise/commit/80bd9f993e83a619407985379878406d8222a1b1#r79287230

This PR fixes this

gsmendoza commented 2 years ago

Thank you @bitberry-dev! I'll investigate the issue with the backend language. For the meantime, can you split the commit into two? Please check out this article on the benefits of making commits atomic.

Please also check this article on ways to improve your git commit messages.

gsmendoza commented 2 years ago

Hi @bitberry-dev ! It seems I'm able to reproduce the issue you're having, but I'm not able to confirm if the solution you provided fixes it.

Please let me know if I'm correctly reproducing the issue.

Expected behavior

Given I have set up and am running the test Rails app like this:

git clone git@github.com:gsmendoza/solidus-auth-devise-224.git
cd solidus-auth-devise-224
bundle install
rake db:create
rake db:migrate
rake db:seed
rails s

When I visit http://localhost:3000/admin

Then I should see that I am redirected to http://localhost:3000/en/admin/login

And I should see that the Admin locale selector is set to English

When I change the locale in the Admin locale selector to Espanol

Then I should see that the login page's locale is switched to Espanol

Actual behavior

I remain in http://localhost:3000/en/admin/login. Furthermore, when I refresh the login page, the Admin locale selector is reset to English (US).

Result of fix

The Rails app already points to the proposed fix but it doesn't seem to have any effect on the issue.

Test app setup

Note that the test app is running on Rails 6. I updated the Solidus I18n gems to match this Rails version. The Solidus I18n readme installation steps haven't been updated yet for Rails 6.

bitberry-dev commented 2 years ago

Hi :)

I split commit into two, no problem 👍
Let's see how is the locale determined in solidus

def set_user_language
      available_locales = Spree.i18n_available_locales
      locale = [
        params[:locale],
        session[set_user_language_locale_key],
        (config_locale if respond_to?(:config_locale, true)),
        I18n.default_locale
      ].detect do |candidate|
        candidate &&
          available_locales.include?(candidate.to_sym)
      end
      session[set_user_language_locale_key] = locale
      I18n.locale = locale
      Carmen.i18n_backend.locale = locale
end

If the locale is not in the params then it extracts from the session. And set_user_language_locale_key affects part with session.

In your version, judging by the URL that includes en, the locale is most likely defined in the params and it obviously should work :) Because params have the highest priority.

Here video of the bug I'm talking about:

Before patch https://user-images.githubusercontent.com/3278464/180974474-7e20d206-e05e-4b1a-92e9-9546c2f40e15.mov

After patch https://user-images.githubusercontent.com/3278464/180974552-58deacea-96a4-4637-9b89-de32d8e273c4.mov

bitberry-dev commented 2 years ago

In addition to the previous answer*

To reproduce this in your test app, you need to set for example config.i18n.available_locales = [:en, :it]

And because of the implementation of Spree.i18n_available_locales you need to add translation of key spree.i18n.this_file_language on additional language, for :it it would be it.spree.i18n.this_file_language (or just add solidus_i18n gem)

After that you will have a choice of language at the admin bottom left

gsmendoza commented 2 years ago

@bitberry-dev I appreciate if you can watch this video testing this PR:

https://user-images.githubusercontent.com/61476/181145574-bb9dcf5c-2478-4b6c-b7f0-6402e649502b.mp4

As I mentioned in the video, I'm not able to confirm that this PR fixes the language selector in the admin login page.

There are two ways we can move forward with this PR. You can either

Update the Rails app I made to get this PR to work on it, or
Create your own Rails app demonstrating that the PR works.

Sorry for the inconvenience. I'm looking forward to getting your PR merged! :)

bitberry-dev commented 2 years ago

@gsmendoza Yes you are right. Indeed, there is such a bug - an unauthorized user cannot change the locale in the admin. And I also had this bug in my notes! And I missed that in my video this patch was applied, sorry for that :( Here is it :)

# frozen_string_literal: true

module Spree
  module Admin
    module LocaleControllerOverrideGlobalization
      extend Spree::Extension

      overrode do
        # todo send pull request to solidus - unauthorized user can't change language
        skip_before_action :authorize_admin, only: [:set]
      end

      def set
        locale = params[:switch_to_locale].to_s.presence

        if locale && I18n.available_locales.include?(locale.to_sym)
          I18n.locale = locale
          session[set_user_language_locale_key] = locale

          respond_to do |format|
            # overrode only for staying on same page when locale changed todo it is not durable code
            format.json { render json: {locale: locale, location: (request.referrer || admin_url)} }
          end
        else
          respond_to do |format|
            format.json { render json: {locale: I18n.locale}, status: 404 }
          end
        end
      end

      override!
    end
  end
end

It just skips authorize_admin before admin/locale#set

I was thinking how to check this particular bug (locale inconsistency) - in theory it would be possible to change the locale when user logged in and after log out and then check in what locale the login page is, but devise apparently clears the session and locale resets to default :)

As a result, to check this bug, we need to send a PR to solidus_backend 😅

bitberry-dev commented 2 years ago

So, in the end, I sent a PR to your repository with a patch for admin/locale#set so that you make sure that my PR fixes locale inconsistence :)

I also recorded two videos with the current solidus_auth_devise and with my fork.

With original solidus_auth_devise:

https://user-images.githubusercontent.com/3278464/181239003-36d2b272-52b8-48e8-8528-6a7f52013bde.mov

*as you can see - the locale cannot be set because it set its value to the session using the wrong key

With my solidus_auth_devise

https://user-images.githubusercontent.com/3278464/181239130-619ebd16-1198-4019-9233-98e59134dd38.mov

*and here we are already using the correct locale key for the admin panel, so everything works

I also found a bug with select2 locale and no fallback there, but that's another story)

gsmendoza commented 2 years ago

Thank you @bitberry-dev !

I've rebased https://github.com/gsmendoza/solidus-auth-devise-224/commits/main with your PR and have confirmed this fix there :+1:
I'll submit a PR to Solidus skipping authorize_admin before LocaleController#set.
I'm approving this PR, but can I make a minor request? Can you capitalize the subject line of each commit message? This will make your commit message consistent with the git log of this repo :)
I'll request someone else to do a second review of your PR.

bitberry-dev commented 2 years ago

@gsmendoza commit messages now capitalized 😉

waiting-for-dev commented 2 years ago

I'll submit a PR to Solidus skipping authorize_admin before LocaleController#set.

Not that dangerous, but it'd mean an unauthorized user can change the admin's locale. That's not ideal. Would it be possible to confirm the change in locale only if the login goes through and revert/not apply if not?

gsmendoza commented 2 years ago

@waiting-for-dev

Would it be possible to confirm the change in locale only if the login goes through and revert/not apply if not?

The change in locale automatically takes effect on the login page; it's not something that takes effect only after the actual login. Please see the with_fix.mov video in this PR.
From a user experience perspective, I think it can be annoying if the locale you have selected in the login page gets reverted immediately after you log in. I think most people would even think that it's a bug that the locale wasn't retained. From their point of view, they were the same person before and after the login.
To make the implementation more secure, I think that instead of skipping authorize_admin for LocaleController#set, we can probably tell LocaleController#set where the request came from. If the request came from a guest area of the admin site (e.g. the login page), then we can allow the guest user to call LocaleController#set.

Let me know what you think!

waiting-for-dev commented 2 years ago

From a user experience perspective, I think it can be annoying if the locale you have selected in the login page gets reverted immediately after you log in.

My idea was to preserve it if the login is successful, only rolling back when not. But that's probably not going to play well with the current implementation.

If the request came from a guest area of the admin site (e.g. the login page), then we can allow the guest user to call LocaleController#set.

And if it didn't come from a guest area, then the user was allowed to change it :slightly_smiling_face:

I'm ok with skipping authorization for now, as otherwise, I think it'll be very complex. In the end, physical access to the computer is out of this kind of security scope.

gsmendoza commented 2 years ago

My idea was to preserve it if the login is successful, only rolling back when not.

Sorry, I'm not following this. Let's discuss it offline :)

And if it didn't come from a guest area, then the user was allowed to change it slightly_smiling_face

Yeah, in hindsight, it's a convoluted implementation that does the same thing as skipping admin authorization :D

@waiting-for-dev Other than that, do you have any other concerns about this PR? Skipping authorize_admin before LocaleController#set is outside the scope of this PR: it has be done on the Solidus repo.

waiting-for-dev commented 2 years ago

I'm worried that if we change set_user_language_locale_key upstream in solidus_backend, that will break the behavior here.

What do you think about extracting the upstream method to a standalone module and including it from Spree::Admin::BaseController and the two descendants of the Devise controllers here?

About removing the template, I'd like to check with @kennyadsl that we're not missing anything, but I think it's ok.

kennyadsl commented 2 years ago

I have not been yet able to review everything, but I think it's ok to remove that partial now.

gsmendoza commented 2 years ago

@waiting-for-dev Something like this?

module Spree
  module Admin
    module SetsUserLanguageLocaleKey
      def set_user_language_locale_key
        :admin_locale
      end
    end
  end
end

module Spree
  module Admin
    class BaseController < Spree::BaseController
      # ...

      private

      # Overrides ControllerHelpers::Common
      # We want the admin's locale selection to be different than that on the frontend
      include SetsUserLanguageLocaleKey
    end
  end
end

class Spree::Admin::UserPasswordsController < Devise::PasswordsController
  # ...

  private

  include SetsUserLanguageLocaleKey
end

class Spree::Admin::UserSessionsController < Devise::SessionsController
  # ...

  private

  include SetsUserLanguageLocaleKey
end

waiting-for-dev commented 2 years ago

@waiting-for-dev Something like this?

Yes, @gsmendoza, I meant that :slightly_smiling_face: WDYT?

gsmendoza commented 2 years ago

It looks good @waiting-for-dev .

So, to recap, here are our TODOs:

Extract SetsUserLanguageLocaleKey module from Spree::Admin::BaseController.
Skip admin authorization for Spree::Admin::LocaleController#set.
Include SetsUserLanguageLocaleKey in SolidusAuthDevise controllers.

@bitberry-dev I'll add these to our TODO list, but if you want to work on them, let us know!

bitberry-dev commented 2 years ago

@bitberry-dev I'll add these to our TODO list, but if you want to work on them, let us know!

Okay, thanks!

gsmendoza commented 2 years ago

Hi @bitberry-dev ! We have just merged https://github.com/solidusio/solidus/pull/4493. We have to wait until Solidus 3.2 is released before we can include Spree::Admin::SetsUserLanguageLocaleKey in the Devise controllers of SolidusAuthDevise. For the meantime, I understand that you're able to work around this issue on your app by overriding the controllers. Is that right?

bitberry-dev commented 2 years ago

Hi @bitberry-dev ! We have just merged solidusio/solidus#4493. We have to wait until Solidus 3.2 is released before we can include Spree::Admin::SetsUserLanguageLocaleKey in the Devise controllers of SolidusAuthDevise.

Great news 👍 Thank you

For the meantime, I understand that you're able to work around this issue on your app by overriding the controllers. Is that right?

Yes, that's right, I got around this problem for several years with my overrides.

gsmendoza commented 2 years ago

Yes, that's right, I got around this problem for several years with my overrides.

Hehe.. Yeah, let's hope we can get this PR merged soon :)

gsmendoza commented 2 years ago

@bitberry-dev Just to let you know that Solidus 3.2 has been released :) Can you update this PR to include Spree::Admin::SetsUserLanguageLocaleKey in the Devise controllers? Thank you!

bitberry-dev commented 2 years ago

@gsmendoza PR updated, solidus_auth_devise supports solidus since version 2.6, so I check before include if this concern Spree::Admin::SetsUserLanguageLocaleKey is defined. When solidus_version in gemspec will bump to 3.2 then this code will need to be fixed.

gsmendoza commented 2 years ago

@bitberry-dev The code looks good. However, can you rebase your branch against master? That might be needed in order to get CI to pass. Thanks!

bitberry-dev commented 2 years ago

@gsmendoza Now all checks passed

bitberry-dev commented 2 years ago

@kennyadsl Sure, added comments in the code and in commit description

kennyadsl commented 2 years ago

Thanks @bitberry-dev!!!

bitberry-dev commented 2 years ago

@kennyadsl You are welcome :)

solidusio / solidus_auth_devise