diff --git a/app/controllers/concerns/client_routable.rb b/app/controllers/concerns/client_routable.rb index db88e9f28872565562cfcebd9ca8b294c75b2b3c..32321642da4b0f68aa135bd41f426f6c6afa0724 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 cc8a5b67e13b31558337b0430bf2558ed6402579..a00b4c689a4f839b071d962b565a679f0b241837 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 24691f24a540dec3aea53d6a842a45879bb3895f..3bb4a3201142659134e737e412983cdbf2ef428e 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 dbcecb0fe12068cf57f1e24200eb43407bfd3ee4..98ed8676e09555ed0789bb35c0d45794ad25801d 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 2580de4a741ed5150e8601f86c6a4149587575db..62ab79493686c392a83c48d4d3f4067d659ebaaf 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 3d58536d9374ec6ef54fef82786b58a32c058455..ff071d219961b955489ccfa0f5cf6575904ee1e5 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 83a11e9c472b876c4856a566910efa56fee0ad75..1bd3839ca45e3ce4c263ede547139a67727ef6a7 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 8a6589592045c2b7c5d8eb351b7d93bc2e83f5ba..6330cbc21df5b365c17d67fc364ac2eae4b8bce6 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 d9b7a9abf38f5abf19cadba5058f2141d9731b40..8e7123e59feece0b29926e37d8bd9a8471358dd3 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