From bcbccd2ccc3a023109b6a9cb34511ac40188c2b4 Mon Sep 17 00:00:00 2001
From: Khemissi Amir <amir.khemissi@insat.ucar.tn>
Date: Wed, 7 Jun 2023 18:23:44 +0100
Subject: [PATCH] Fixed bad OmniAuth flow when deployed on a subdirectory
(Closes #5176). (#5197)
* Fixed static redirections on ExternalController#create API.
+ Fixed static OIDC URLs on the FE codebase.
* Changed other host redirection handeling on external authN.
---
app/controllers/concerns/client_routable.rb | 5 ++
app/controllers/external_controller.rb | 39 +++++++-----
.../components/home/AuthButtons.jsx | 2 +-
.../rooms/room/join/RequireAuthentication.jsx | 2 +-
.../components/rooms/room/join/RoomJoin.jsx | 7 +--
config/application.rb | 3 +-
esbuild.dev.mjs | 1 +
esbuild.mjs | 1 +
spec/controllers/external_controller_spec.rb | 61 +++++++++++++++----
9 files changed, 88 insertions(+), 33 deletions(-)
diff --git a/app/controllers/concerns/client_routable.rb b/app/controllers/concerns/client_routable.rb
index db88e9f2..32321642 100644
--- a/app/controllers/concerns/client_routable.rb
+++ b/app/controllers/concerns/client_routable.rb
@@ -28,4 +28,9 @@ module ClientRoutable
def reset_password_url(token)
"#{root_url}reset_password/#{token}"
end
+
+ # Generates a client side pending url.
+ def pending_path
+ "#{root_path}pending"
+ end
end
diff --git a/app/controllers/external_controller.rb b/app/controllers/external_controller.rb
index cc8a5b67..a00b4c68 100644
--- a/app/controllers/external_controller.rb
+++ b/app/controllers/external_controller.rb
@@ -17,6 +17,8 @@
# frozen_string_literal: true
class ExternalController < ApplicationController
+ include ClientRoutable
+
skip_before_action :verify_authenticity_token
# GET 'auth/:provider/callback'
@@ -25,13 +27,8 @@ class ExternalController < ApplicationController
provider = current_provider
credentials = request.env['omniauth.auth']
- user_info = {
- name: credentials['info']['name'],
- email: credentials['info']['email'],
- language: extract_language_code(credentials['info']['locale']),
- external_id: credentials['uid'],
- verified: true
- }
+
+ user_info = build_user_info(credentials)
user = User.find_by(external_id: credentials['uid'], provider:)
new_user = user.blank?
@@ -40,7 +37,7 @@ class ExternalController < ApplicationController
# Check if they have a valid token only if a new sign up
if new_user && registration_method == SiteSetting::REGISTRATION_METHODS[:invite] && !valid_invite_token(email: user_info[:email])
- return redirect_to "/?error=#{Rails.configuration.custom_error_msgs[:invite_token_invalid]}"
+ return redirect_to root_path(error: Rails.configuration.custom_error_msgs[:invite_token_invalid])
end
# Create the user if they dont exist
@@ -58,21 +55,25 @@ class ExternalController < ApplicationController
# Set to pending if registration method is approval
if registration_method == SiteSetting::REGISTRATION_METHODS[:approval]
user.pending! if new_user
- return redirect_to '/pending' if user.pending?
+ return redirect_to pending_path if user.pending?
end
user.generate_session_token!
session[:session_token] = user.session_token
# TODO: - Ahmad: deal with errors
- redirect_location = cookies[:location]
- cookies.delete(:location)
- return redirect_to redirect_location if redirect_location&.match?('\A\/rooms\/\w{3}-\w{3}-\w{3}-\w{3}\/join\z')
- redirect_to '/'
+ redirect_location = cookies.delete(:location)
+
+ return redirect_to redirect_location, allow_other_host: false if redirect_location&.match?('\/rooms\/\w{3}-\w{3}-\w{3}-\w{3}\/join\z')
+
+ redirect_to root_path
+ rescue ActionController::Redirecting::UnsafeRedirectError => e
+ Rails.logger.error("Unsafe redirection attempt: #{e}")
+ redirect_to root_path
rescue StandardError => e
Rails.logger.error("Error during authentication: #{e}")
- redirect_to '/?error=SignupError'
+ redirect_to root_path(error: Rails.configuration.custom_error_msgs[:external_signup_error])
end
# POST /recording_ready
@@ -126,4 +127,14 @@ class ExternalController < ApplicationController
# Try to delete the invitation and return true if it succeeds
Invitation.destroy_by(email:, provider: current_provider, token:).present?
end
+
+ def build_user_info(credentials)
+ {
+ name: credentials['info']['name'],
+ email: credentials['info']['email'],
+ language: extract_language_code(credentials['info']['locale']),
+ external_id: credentials['uid'],
+ verified: true
+ }
+ end
end
diff --git a/app/javascript/components/home/AuthButtons.jsx b/app/javascript/components/home/AuthButtons.jsx
index 24691f24..3bb4a320 100644
--- a/app/javascript/components/home/AuthButtons.jsx
+++ b/app/javascript/components/home/AuthButtons.jsx
@@ -42,7 +42,7 @@ export default function AuthButtons({ direction }) {
if (env?.OPENID_CONNECT) {
return (
- <Form action="/auth/openid_connect" method="POST" data-turbo="false">
+ <Form action={process.env.OMNIAUTH_PATH} method="POST" data-turbo="false">
<input type="hidden" name="authenticity_token" value={document.querySelector('meta[name="csrf-token"]').content} />
<input type="hidden" name="current_provider" value={env?.CURRENT_PROVIDER} />
<Stack direction={direction} gap={2}>
diff --git a/app/javascript/components/rooms/room/join/RequireAuthentication.jsx b/app/javascript/components/rooms/room/join/RequireAuthentication.jsx
index dbcecb0f..98ed8676 100644
--- a/app/javascript/components/rooms/room/join/RequireAuthentication.jsx
+++ b/app/javascript/components/rooms/room/join/RequireAuthentication.jsx
@@ -39,7 +39,7 @@ export default function RequireAuthentication({ path }) {
<Card.Footer className="bg-white">
{
env?.OPENID_CONNECT ? (
- <Form action="/auth/openid_connect" method="POST" data-turbo="false">
+ <Form action={process.env.OMNIAUTH_PATH} method="POST" data-turbo="false">
<input type="hidden" name="authenticity_token" value={document.querySelector('meta[name="csrf-token"]').content} />
<Button variant="brand-outline-color" className="btn btn-lg m-2" type="submit">{t('authentication.sign_up')}</Button>
<Button variant="brand" className="btn btn-lg m-2" type="submit">{t('authentication.sign_in')}</Button>
diff --git a/app/javascript/components/rooms/room/join/RoomJoin.jsx b/app/javascript/components/rooms/room/join/RoomJoin.jsx
index 2580de4a..62ab7949 100644
--- a/app/javascript/components/rooms/room/join/RoomJoin.jsx
+++ b/app/javascript/components/rooms/room/join/RoomJoin.jsx
@@ -18,7 +18,7 @@
import React, { useState, useEffect } from 'react';
import Card from 'react-bootstrap/Card';
import {
- Navigate, Link, useLocation, useParams,
+ Navigate, Link, useParams,
} from 'react-router-dom';
import {
Button, Col, Row, Stack, Form as RegularForm,
@@ -54,8 +54,7 @@ export default function RoomJoin() {
const { methods, fields } = useRoomJoinForm();
- const location = useLocation();
- const path = encodeURIComponent(location.pathname);
+ const path = encodeURIComponent(document.location.pathname);
useEffect(() => { // set cookie to return to if needed
const date = new Date();
@@ -243,7 +242,7 @@ export default function RoomJoin() {
{!currentUser?.signed_in && (
env?.OPENID_CONNECT ? (
<Stack direction="horizontal" className="d-flex justify-content-center text-muted mt-3"> {t('authentication.already_have_account')}
- <RegularForm action="/auth/openid_connect" method="POST" data-turbo="false">
+ <RegularForm action={process.env.OMNIAUTH_PATH} method="POST" data-turbo="false">
<input type="hidden" name="authenticity_token" value={document.querySelector('meta[name="csrf-token"]').content} />
<Button variant="link" className="btn-sm fs-6 cursor-pointer ms-2 ps-0" type="submit">{t('authentication.sign_in')}</Button>
</RegularForm>
diff --git a/config/application.rb b/config/application.rb
index 3d58536d..ff071d21 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -50,7 +50,8 @@ module Greenlight
room_limit: 'RoomLimitError',
pending_user: 'PendingUser',
banned_user: 'BannedUser',
- unverified_user: 'UnverifiedUser'
+ unverified_user: 'UnverifiedUser',
+ external_signup_error: 'SignupError'
}
config.uploads = {
diff --git a/esbuild.dev.mjs b/esbuild.dev.mjs
index 83a11e9c..1bd3839c 100644
--- a/esbuild.dev.mjs
+++ b/esbuild.dev.mjs
@@ -20,6 +20,7 @@ await esbuild.build({
},
define: {
'process.env.RELATIVE_URL_ROOT': `"${relativeUrlRoot}"`,
+ 'process.env.OMNIAUTH_PATH': `"${relativeUrlRoot}/auth/openid_connect"`,
},
});
diff --git a/esbuild.mjs b/esbuild.mjs
index 8a658959..6330cbc2 100644
--- a/esbuild.mjs
+++ b/esbuild.mjs
@@ -14,6 +14,7 @@ await esbuild.build({
},
define: {
'process.env.RELATIVE_URL_ROOT': `"${relativeUrlRoot}"`,
+ 'process.env.OMNIAUTH_PATH': `"${relativeUrlRoot}/auth/openid_connect"`,
},
});
diff --git a/spec/controllers/external_controller_spec.rb b/spec/controllers/external_controller_spec.rb
index d9b7a9ab..8e7123e5 100644
--- a/spec/controllers/external_controller_spec.rb
+++ b/spec/controllers/external_controller_spec.rb
@@ -54,7 +54,7 @@ RSpec.describe ExternalController, type: :controller do
get :create_user, params: { provider: 'openid_connect' }
expect(session[:session_token]).to eq(User.find_by(email: OmniAuth.config.mock_auth[:openid_connect][:info][:email]).session_token)
- expect(response).to redirect_to('/')
+ expect(response).to redirect_to(root_path)
end
it 'assigns the User role to the user' do
@@ -74,7 +74,7 @@ RSpec.describe ExternalController, type: :controller do
end
context 'redirect' do
- it 'redirects to the location cookie if the format is valid' do
+ it 'redirects to the location cookie if a relative redirection 1' do
request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect]
cookies[:location] = {
@@ -86,19 +86,31 @@ RSpec.describe ExternalController, type: :controller do
expect(response).to redirect_to('/rooms/o5g-hvb-s44-p5t/join')
end
- it 'doesnt redirect if it doesnt match a room joins format' do
+ it 'redirects to the location cookie if a relative redirection 2' do
request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect]
cookies[:location] = {
- value: 'https://google.com',
+ value: '/a/b/c/d/rooms/o5g-hvb-s44-p5t/join',
path: '/'
}
get :create_user, params: { provider: 'openid_connect' }
- expect(response).to redirect_to('/')
+ expect(response).to redirect_to('/a/b/c/d/rooms/o5g-hvb-s44-p5t/join')
end
- it 'doesnt redirect if it doesnt match a room joins format check 2' do
+ it 'doesnt redirect if NOT a relative redirection check 1' do
+ request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect]
+
+ cookies[:location] = {
+ value: Faker::Internet.url,
+ path: '/'
+ }
+ get :create_user, params: { provider: 'openid_connect' }
+
+ expect(response).to redirect_to(root_path)
+ end
+
+ it 'doesnt redirect if it NOT a relative redirection format check 2' do
request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect]
cookies[:location] = {
@@ -107,10 +119,10 @@ RSpec.describe ExternalController, type: :controller do
}
get :create_user, params: { provider: 'openid_connect' }
- expect(response).to redirect_to('/')
+ expect(response).to redirect_to(root_path)
end
- it 'doesnt redirect if it doesnt match a room joins format check 3' do
+ it 'doesnt redirect if it NOT a relative redirection format check 3' do
request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect]
cookies[:location] = {
@@ -119,7 +131,31 @@ RSpec.describe ExternalController, type: :controller do
}
get :create_user, params: { provider: 'openid_connect' }
- expect(response).to redirect_to('/')
+ expect(response).to redirect_to(root_path)
+ end
+
+ it 'doesnt redirect if NOT a relative redirection check 4' do
+ request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect]
+
+ cookies[:location] = {
+ value: Faker::Internet.url(path: '/rooms/o5g-hvb-s44-p5t/join'),
+ path: '/'
+ }
+ get :create_user, params: { provider: 'openid_connect' }
+
+ expect(response).to redirect_to(root_path)
+ end
+
+ it 'doesnt redirect if NOT a valid room join link check 5' do
+ request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect]
+
+ cookies[:location] = {
+ value: '/romios/o5g-hvb-s44-p5t/join',
+ path: '/'
+ }
+ get :create_user, params: { provider: 'openid_connect' }
+
+ expect(response).to redirect_to(root_path)
end
it 'deletes the cookie after reading' do
@@ -215,7 +251,7 @@ RSpec.describe ExternalController, type: :controller do
create(:user, external_id: OmniAuth.config.mock_auth[:openid_connect][:uid])
expect { get :create_user, params: { provider: 'openid_connect' } }.not_to raise_error
- expect(response).to redirect_to('/')
+ expect(response).to redirect_to(root_path)
end
it 'returns an InviteInvalid error if no invite is passed' do
@@ -223,7 +259,7 @@ RSpec.describe ExternalController, type: :controller do
get :create_user, params: { provider: 'openid_connect' }
- expect(response).to redirect_to('/?error=InviteInvalid')
+ expect(response).to redirect_to(root_path(error: Rails.configuration.custom_error_msgs[:invite_token_invalid]))
end
it 'returns an InviteInvalid error if the token is wrong' do
@@ -235,7 +271,7 @@ RSpec.describe ExternalController, type: :controller do
get :create_user, params: { provider: 'openid_connect' }
- expect(response).to redirect_to('/?error=InviteInvalid')
+ expect(response).to redirect_to(root_path(error: Rails.configuration.custom_error_msgs[:invite_token_invalid]))
end
end
@@ -252,6 +288,7 @@ RSpec.describe ExternalController, type: :controller do
expect { get :create_user, params: { provider: 'openid_connect' } }.to change(User, :count).by(1)
expect(User.find_by(email: OmniAuth.config.mock_auth[:openid_connect][:info][:email])).to be_pending
+ expect(response).to redirect_to(controller.pending_path)
end
end
end
--
GitLab