Bug/ch132025/split configs for broker and central login #16150

amiedes · 2021-02-10T14:36:37Z

Related: https://app.clubhouse.io/cartoteam/story/132025/split-configs-for-broker-and-central-login-communication

shortcut-integration · 2021-02-10T14:36:39Z

This pull request has been linked to Clubhouse Story #132025: Split configs for broker and central login / communication.

amiedes · 2021-02-23T13:20:50Z

Staging acceptance ✅

CartoDB -> Central redirection enabled

ubuntu@ded-dynamic-1qa16uc:~$ grep cartodb_central_api www/production.cartodb.com/current/config/app_config.yml -A 2
-bash: fc: history specification out of range
  cartodb_central_api:
    host: "carto-staging.com"
    port: "443"

Regular user login (amiedes-testlogin-1)

Central login (https://carto-staging.com/login) logs user in and redirects to CartoDB dashboard (https://amiedes-testlogin-1.carto-staging.com/dashboard)
CartoDB login (https://amiedes-testlogin-1.carto-staging.com/login) redirects to Central login

Organization user login (amiedes-testlogin-1-org-admin)

Central login (https://carto-staging.com/login) logs user in and redirects to CartoDB dashboard (https://amiedes-testlogin-1-org.carto-staging.com/u/amiedes-testlogin-1-org-admin/dashboard)
CartoDB organization login (https://amiedes-testlogin-1-org.carto-staging.com/login) works OK and does not redirect to Central.

CartoDB -> Central redirection disabled

ubuntu@ded-dynamic-1qa16uc:~$ grep cartodb_central_api www/production.cartodb.com/current/config/app_config.yml -A 2
  #cartodb_central_api:
    #host: "carto-staging.com"
    #port: "443"

sudo systemctl restart unicorn-cartodb

Regular user login (amiedes-testlogin-1)

Central login (https://carto-staging.com/login) logs user in and redirects to CartoDB dashboard (https://amiedes-testlogin-1.carto-staging.com/dashboard)
CartoDB login (https://amiedes-testlogin-1.carto-staging.com/login) does not redirect to Central login. It logs the user in OK.

Organization user login (amiedes-testlogin-1-org-admin)

Central login (https://carto-staging.com/login) logs user in and redirects to CartoDB dashboard (https://amiedes-testlogin-1-org.carto-staging.com/u/amiedes-testlogin-1-org-admin/dashboard)
CartoDB organization login (https://amiedes-testlogin-1-org.carto-staging.com/login) works OK and does not redirect to Central. It redirects to the Dashboard (https://amiedes-testlogin-1-org.carto-staging.com/u/amiedes-testlogin-1-org-admin/dashboard/)

amiedes · 2021-02-23T13:29:25Z

app/models/carto/oauth_app.rb

@@ -12,7 +12,7 @@ class OauthApp < ActiveRecord::Base
    has_many :oauth_app_users, inverse_of: :oauth_app, dependent: :destroy
    has_many :oauth_app_organizations, inverse_of: :oauth_app, dependent: :destroy

-    validates :user, presence: true, if: -> { sync_with_central? || !central_enabled? }
+    validates :user, presence: true, if: -> { sync_with_central? || Cartodb::Central.api_sync_disabled? }


The process for migrating this was:

Replace central_enabled? per Cartodb::Central.api_sync_enabled?

Replace !central_enabled? per Cartodb::Central.api_sync_disabled?

The central_enabled? method becomes unused, so it can be removed

amiedes · 2021-02-23T13:30:07Z

app/models/carto/user_creation.rb

@@ -344,9 +344,4 @@ def clean_password
    self.crypted_password = ''
    self.save
  end
-
-  # INFO: state_machine needs guard methods to be instance methods


Maybe in a previous version of the gem, but not anymore. Removing the "alias" to avoid one additional level of indirection.

rafatower

LGTM 👍

About my silly comments, I'd just ignore them and focus on updating with master (to get CI back), test & deploy for the sake of pragmatism. In the mid term there won't be any "old http client" so not worth it.

rafatower · 2021-02-24T11:08:40Z

app/controllers/sessions_controller.rb

@@ -49,7 +49,7 @@ def new
    elsif saml_authentication? && !user
      # Automatically trigger SAML request on login view load -- could easily trigger this elsewhere
      redirect_to(saml_service.authentication_request)
-    elsif central_enabled? && !@organization.try(:auth_enabled?)
+    elsif Cartodb::Central.login_redirection_enabled? && !@organization.try(:auth_enabled?)


silly thing: can we take advantage to use the safe operator @organization&.auth_enabled? ?

Yes, I think it should be equivalent. Sometimes I'm more keen to refactorings that others 😆 🤷

rafatower · 2021-02-24T11:09:12Z

app/controllers/sessions_controller.rb

@@ -213,7 +213,11 @@ def password_expired

    respond_to do |format|
      format.html do
-        url = central_enabled? && !@organization.try(:auth_enabled?) ? Cartodb::Central.new.login_url : login_url
+        url = if Cartodb::Central.login_redirection_enabled? && !@organization.try(:auth_enabled?)


same here about safe operator

app/models/carto/helpers/user_commons.rb

rafatower · 2021-02-24T11:30:09Z

lib/cartodb/central.rb

+
+    class <<self
+
+      alias login_redirection_enabled? api_sync_enabled?


…central-login

amiedes force-pushed the bug/ch132025/split-configs-for-broker-and-central-login branch from 18a0ed0 to 1027160 Compare February 10, 2021 16:28

amiedes added 4 commits February 23, 2021 09:19

Split Message Broker sync and API sync configuration

2eff52f

Replace old method calls

b71c475

Fix specs

8c59b51

Remove useless stubs

0f658d5

amiedes force-pushed the bug/ch132025/split-configs-for-broker-and-central-login branch from 1027160 to 0f658d5 Compare February 23, 2021 08:19

Update NEWS.md

c1dee57

amiedes marked this pull request as ready for review February 23, 2021 13:24

amiedes commented Feb 23, 2021

View reviewed changes

amiedes requested a review from rafatower February 23, 2021 13:32

rafatower approved these changes Feb 24, 2021

View reviewed changes

Merge branch 'master' into bug/ch132025/split-configs-for-broker-and-…

9f7472e

…central-login

amiedes merged commit 88e5533 into master Feb 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/ch132025/split configs for broker and central login #16150

Bug/ch132025/split configs for broker and central login #16150

amiedes commented Feb 10, 2021 •

edited

Loading

shortcut-integration bot commented Feb 10, 2021

amiedes commented Feb 23, 2021

amiedes Feb 23, 2021

amiedes Feb 23, 2021

rafatower left a comment

rafatower Feb 24, 2021

amiedes Feb 24, 2021

rafatower Feb 24, 2021

rafatower Feb 24, 2021


		class <<self

		alias login_redirection_enabled? api_sync_enabled?

Bug/ch132025/split configs for broker and central login #16150

Bug/ch132025/split configs for broker and central login #16150

Conversation

amiedes commented Feb 10, 2021 • edited Loading

shortcut-integration bot commented Feb 10, 2021

amiedes commented Feb 23, 2021

Staging acceptance ✅

CartoDB -> Central redirection enabled

CartoDB -> Central redirection disabled

amiedes Feb 23, 2021

Choose a reason for hiding this comment

amiedes Feb 23, 2021

Choose a reason for hiding this comment

rafatower left a comment

Choose a reason for hiding this comment

rafatower Feb 24, 2021

Choose a reason for hiding this comment

amiedes Feb 24, 2021

Choose a reason for hiding this comment

rafatower Feb 24, 2021

Choose a reason for hiding this comment

rafatower Feb 24, 2021

Choose a reason for hiding this comment

amiedes commented Feb 10, 2021 •

edited

Loading