Skip to content

Commit

Permalink
Fix issues with customized Omniauth callback handling
Browse files Browse the repository at this point in the history
If a 3rd party Decidim authentication module customizes the
Omniauth callback flow by extending the
OmniauthRegistrationsController, it would cause exceptions/errors
due to the assumptions made that the core controller is the only
one handling the callback flow.
  • Loading branch information
ahukkanen committed Sep 5, 2024
1 parent add3116 commit dbd4411
Show file tree
Hide file tree
Showing 8 changed files with 155 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def after_sign_in_path_for(user)
if user.present? && user.blocked?
check_user_block_status(user)
elsif user.needs_password_update?
change_password_path
decidim.change_password_path
elsif first_login_and_not_authorized?(user) && !user.admin? && !pending_redirect?(user)
decidim_verifications.first_login_authorizations_path
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<%= form_required_explanation %>
</div>

<%= decidim_form_for(@form, namespace: "registration", as: resource_name, url: omniauth_registrations_path(resource_name), html: { id: "omniauth-register-form" }) do |f| %>
<%= decidim_form_for(@form, namespace: "registration", as: resource_name, url: decidim.omniauth_registrations_path(resource_name), html: { id: "omniauth-register-form" }) do |f| %>

<div class="form__wrapper">
<%= f.text_field :name, help_text: t("decidim.devise.omniauth_registrations.new.username_help"), autocomplete: "name", placeholder: "John Doe" %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<h1 class="title-decorator my-12"><%= t("decidim.devise.omniauth_registrations.new_tos_fields.sign_up_title") %></h1>
</div>

<%= decidim_form_for(@form, namespace: "registration", as: resource_name, url: omniauth_registrations_path(resource_name), html: { id: "omniauth-register-form" }) do |f| %>
<%= decidim_form_for(@form, namespace: "registration", as: resource_name, url: decidim.omniauth_registrations_path(resource_name), html: { id: "omniauth-register-form" }) do |f| %>

<div class="form__wrapper">
<%= f.hidden_field :name %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<% end %>
</div>

<%= form.check_box :tos_agreement, label: t("decidim.devise.registrations.new.tos_agreement", link: link_to(t("decidim.devise.registrations.new.terms"), page_path("terms-of-service"))), label_options: { class: "form__wrapper-checkbox-label" } %>
<%= form.check_box :tos_agreement, label: t("decidim.devise.registrations.new.tos_agreement", link: link_to(t("decidim.devise.registrations.new.terms"), decidim.page_path("terms-of-service"))), label_options: { class: "form__wrapper-checkbox-label" } %>
</div>

<div id="card__newsletter" class="form__wrapper-block">
Expand Down
18 changes: 18 additions & 0 deletions decidim-dev/lib/decidim/dev/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@ class Engine < ::Rails::Engine
resources :nested_dummy_resources
get :foo, on: :member
end

devise_scope :user do
match(
"/users/auth/dev/callback",
to: "omniauth_callbacks#dev_callback",
as: "user_dev_omniauth_authorize",
via: [:get, :post]
)
end
end

initializer "decidim_dev.tools" do
Expand All @@ -25,6 +34,15 @@ class Engine < ::Rails::Engine
ActiveSupport.on_load(:action_controller) { include Decidim::Dev::NeedsDevelopmentTools } if Rails.env.development? || ENV.fetch("DECIDIM_DEV_ENGINE", nil)
end

initializer "decidim_dev.mount_test_routes", before: :add_routing_paths do
next unless Rails.env.test?

# Required for overriding the callback route.
Decidim::Core::Engine.routes.prepend do
mount Decidim::Dev::Engine => "/"
end
end

initializer "decidim_dev.webpacker.assets_path" do
Decidim.register_assets_path File.expand_path("app/packs", root)
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# frozen_string_literal: true

require "spec_helper"

module Decidim::Dev
class OmniauthCallbacksController < Decidim::Devise::OmniauthRegistrationsController
def callback; end
end
end

describe "ThirdPartyOmniauthRegistrationsController" do
routes { Decidim::Dev::Engine.routes }

let(:organization) { create(:organization) }

controller(Decidim::Dev::OmniauthCallbacksController) {}

before do
request.env["decidim.current_organization"] = organization
request.env["devise.mapping"] = Devise.mappings[:user]
end

describe "POST callback" do
let(:provider) { "custom" }
let(:uid) { "12345" }
let(:email) { "[email protected]" }
let!(:user) { create(:user, organization:, email:) }

before do
request.env["omniauth.auth"] = {
provider:,
uid:,
info: {
name: "Custom Auth",
nickname: "custom_auth",
email:
}
}
end

describe "after_sign_in_path_for" do
subject { controller.after_sign_in_path_for(user) }

context "when the user is admin who has a pending password change" do
let(:user) { build(:user, :admin, organization:, sign_in_count: 1, password_updated_at: 1.year.ago) }

it { is_expected.to eq("/change_password") }
end
end
end
end
79 changes: 79 additions & 0 deletions decidim-dev/spec/requests/third_party_omniauth_callback_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# frozen_string_literal: true

require "spec_helper"

module Decidim::Dev
# This controller simulates a customized Omniauth callback flow that would be
# used by 3rd party login providers. Such providers may need to customize how
# the login callback is handled based on conditions returned by the Omniauth
# provider. Customizing the Omniauth strategy is not enough when the login
# provider needs access to the Decidim context.
class OmniauthCallbacksController < Decidim::Devise::OmniauthRegistrationsController
def dev_callback
create
end
end
end

RSpec.describe "Omniauth callback" do
subject { response.body }

let(:organization) { create(:organization) }

let(:user) { create(:user, :confirmed, organization:, email: "[email protected]", password: "decidim123456789") }

let(:oauth_hash) do
{
provider: "dev",
uid:,
info: {
name: "Custom Auth",
nickname: "custom_auth",
email:
}
}
end

before do
host! organization.host
end

describe "POST callback" do
let(:request_path) { "/users/auth/dev/callback" }

let(:uid) { "12345" }
let(:email) { "[email protected]" }

context "with a new user" do
it "shows the create an account form" do
get(request_path, env: { "omniauth.auth" => oauth_hash })

expect(response).to have_http_status(:ok)
expect(response.body).to include("Create an account")
expect(response.body).to include("Terms of Service")
end
end

context "with existing user" do
let!(:user) { create(:user, organization:, email:) }

it "redirects to root" do
get(request_path, env: { "omniauth.auth" => oauth_hash })

expect(response).to have_http_status(:redirect)
expect(response).to redirect_to("/")
end
end

context "when the user is admin with a pending password change" do
let!(:user) { create(:user, :confirmed, :admin, organization:, email:, sign_in_count: 1, password_updated_at: 1.year.ago) }

it "redirects to the /change_password path" do
get(request_path, env: { "omniauth.auth" => oauth_hash })

expect(response).to have_http_status(:redirect)
expect(response).to redirect_to("/change_password")
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ test:
enabled: true
client_id:
client_secret:
dev:
enabled: true
icon: tools-line

# Do not keep production secrets in the repository,
# instead read values from the environment.
Expand Down

0 comments on commit dbd4411

Please sign in to comment.