Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(auth): Single-User Multiple-Email Issue using CM-Keycloak #7728

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

fix(auth): Single-User Multiple-Email Issue using CM-Keycloak #7728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
013b167
feat(keycloak): use CM userId for keycloak user ID
bivanalhar Dec 27, 2024
e62be73
feat(user): prevent destroying primary email
bivanalhar Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions app/controllers/user/emails_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@ def create
end

def destroy
if @email.primary?
@email.errors.add(:base, I18n.t('user.emails.index.cannot_delete_primary'))
render json: { errors: @email.errors.full_messages.to_sentence }, status: :bad_request

return
end

if @email.destroy
render_emails
else
Expand Down
24 changes: 12 additions & 12 deletions authentication/import/coursemology_realm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1788,7 +1788,7 @@
"select encrypted_password as hash_pwd from users right join user_emails ue on users.id = ue.user_id where LOWER(ue.email) = LOWER(?)"
],
"cachePolicy": [
"DEFAULT"
"NO_CACHE"
],
"url": [
"jdbc:postgresql://host.docker.internal:5432/coursemology"
Expand All @@ -1800,22 +1800,22 @@
"false"
],
"findBySearchTerm": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\"from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) like LOWER(concat('%', ?, '%')) or LOWER(users.name) like LOWER(concat('%', ?, '%'))"
"SELECT users.id AS \"id\", users.id AS \"user_id\", pe.email AS \"username\", pe.email AS \"email\", users.name AS \"firstName\", CASE WHEN MAX(CASE WHEN pe.confirmed_at IS NOT NULL THEN 1 ELSE 0 END) = 1 THEN 'TRUE' ELSE 'FALSE' END AS \"EMAIL_VERIFIED\" FROM user_emails ue LEFT JOIN users ON ue.user_id = users.id LEFT JOIN user_emails pe ON ue.user_id = pe.user_id AND pe.primary = TRUE WHERE LOWER(pe.email) LIKE LOWER(CONCAT('%', ?, '%')) OR LOWER(users.name) LIKE LOWER(CONCAT('%', ?, '%')) OR (ue.primary = FALSE AND ue.confirmed_at IS NOT NULL AND LOWER(ue.email) LIKE LOWER(CONCAT('%', ?, '%'))) GROUP BY pe.id, users.id, pe.email, users.name"
],
"password": [
"password"
],
"findByUsername": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?)"
"SELECT users.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE 'FALSE' END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?) GROUP BY users.id, ue.email, users.name, ue.confirmed_at"
],
"findById": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id where cast(ue.id as character varying) = ?"
"SELECT users.id as \"id\", users.id as \"user_id\", pe.email as \"username\", pe.email as \"email\", users.name as \"firstName\" from user_emails pe left join users on pe.user_id = users.id where cast(users.id as character varying) = ? AND pe.primary = TRUE AND pe.confirmed_at IS NOT NULL GROUP BY users.id, pe.email, users.name"
],
"listAll": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id"
"SELECT users.id as \"id\", users.id as \"user_id\", pe.email as \"username\", pe.email as \"email\", users.name as \"firstName\", CASE WHEN pe.confirmed_at IS NOT NULL THEN 'TRUE' ELSE 'FALSE' END as \"EMAIL_VERIFIED\" from user_emails pe left join users on pe.user_id = users.id where pe.primary = TRUE"
],
"findByEmail": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?)"
"SELECT users.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE 'FALSE' END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?) GROUP BY users.id, ue.email, users.name, ue.confirmed_at"
],
"user": [
"postgres"
Expand Down Expand Up @@ -4180,7 +4180,7 @@
"select count(*) from users"
],
"cachePolicy": [
"DEFAULT"
"NO_CACHE"
],
"url": [
"jdbc:postgresql://host.docker.internal:5432/coursemology_test"
Expand All @@ -4192,19 +4192,19 @@
"false"
],
"findBySearchTerm": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\"from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) like LOWER(concat('%', ?, '%')) or LOWER(users.name) like LOWER(concat('%', ?, '%'))"
"SELECT users.id AS \"id\", users.id AS \"user_id\", pe.email AS \"username\", pe.email AS \"email\", users.name AS \"firstName\", CASE WHEN MAX(CASE WHEN pe.confirmed_at IS NOT NULL THEN 1 ELSE 0 END) = 1 THEN 'TRUE' ELSE NULL END AS \"EMAIL_VERIFIED\" FROM user_emails ue LEFT JOIN users ON ue.user_id = users.id LEFT JOIN user_emails pe ON ue.user_id = pe.user_id AND pe.primary = TRUE WHERE LOWER(pe.email) LIKE LOWER(CONCAT('%', ?, '%')) OR LOWER(users.name) LIKE LOWER(CONCAT('%', ?, '%')) OR (ue.primary = FALSE AND ue.confirmed_at IS NOT NULL AND LOWER(ue.email) LIKE LOWER(CONCAT('%', ?, '%'))) GROUP BY pe.id, users.id, pe.email, users.name"
],
"findByUsername": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?)"
"SELECT users.id as \"id\", users.id as \"user_id\", pe.email as \"username\", pe.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id left join user_emails pe on ue.user_id = pe.user_id AND pe.primary = TRUE where LOWER(ue.email) = LOWER(?) AND ue.confirmed_at IS NOT NULL GROUP BY pe.id, users.id, pe.email, users.name, ue.confirmed_at"
],
"findById": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id where cast(ue.id as character varying) = ?"
"SELECT users.id as \"id\", users.id as \"user_id\", pe.email as \"username\", pe.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id left join user_emails pe ON ue.user_id = pe.user_id AND pe.primary = TRUE where cast(users.id as character varying) = ? AND ue.confirmed_at IS NOT NULL GROUP BY pe.id, users.id, pe.email, users.name, ue.confirmed_at"
],
"listAll": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id"
"SELECT users.id as \"id\", users.id as \"user_id\", pe.email as \"username\", pe.email as \"email\", users.name as \"firstName\", CASE WHEN pe.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails pe left join users on pe.user_id = users.id where pe.primary = TRUE"
],
"findByEmail": [
"select ue.id as \"id\", users.id as \"user_id\", ue.email as \"username\", ue.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?)"
"SELECT users.id as \"id\", users.id as \"user_id\", pe.email as \"username\", pe.email as \"email\", users.name as \"firstName\", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as \"EMAIL_VERIFIED\" from user_emails ue left join users on ue.user_id = users.id left join user_emails pe on ue.user_id = pe.user_id AND pe.primary = TRUE where LOWER(ue.email) = LOWER(?) AND ue.confirmed_at IS NOT NULL GROUP BY pe.id, users.id, pe.email, users.name, ue.confirmed_at"
],
"user": [
"postgres"
Expand Down
10 changes: 5 additions & 5 deletions authentication/script/cm_db_federation.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,19 @@
select count(*) from users;

-- List All Users SQL query
select ue.id as "id", users.id as "user_id", ue.email as "username", ue.email as "email", users.name as "firstName", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as "EMAIL_VERIFIED" from user_emails ue left join users on ue.user_id = users.id
select users.id as "id", users.id as "user_id", pe.email as "username", pe.email as "email", users.name as "firstName", CASE WHEN pe.confirmed_at IS NOT NULL THEN 'TRUE' ELSE 'FALSE' END as "EMAIL_VERIFIED" from user_emails pe left join users on pe.user_id = users.id where pe.primary = TRUE

-- Find user by id SQL query
select ue.id as "id", users.id as "user_id", ue.email as "username", ue.email as "email", users.name as "firstName", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as "EMAIL_VERIFIED" from user_emails ue left join users on ue.user_id = users.id where cast(ue.id as character varying) = ?
select users.id as "id", users.id as "user_id", pe.email as "username", pe.email as "email", users.name as "firstName" from user_emails pe left join users on pe.user_id = users.id where cast(users.id as character varying) = ? AND pe.primary = TRUE and pe.confirmed_at IS NOT NULL GROUP BY users.id, pe.email, users.name

-- Find user by username SQL query
select ue.id as "id", users.id as "user_id", ue.email as "username", ue.email as "email", users.name as "firstName", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as "EMAIL_VERIFIED" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?)
select users.id as "id", users.id as "user_id", ue.email as "username", ue.email as "email", users.name as "firstName", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE 'FALSE' END as "EMAIL_VERIFIED" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?) GROUP BY users.id, ue.email, users.name, ue.confirmed_at

-- Find user by email SQL query
select ue.id as "id", users.id as "user_id", ue.email as "username", ue.email as "email", users.name as "firstName", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as "EMAIL_VERIFIED" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?)
select users.id as "id", users.id as "user_id", ue.email as "username", ue.email as "email", users.name as "firstName", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE 'FALSE' END as "EMAIL_VERIFIED" from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) = LOWER(?) GROUP BY users.id, ue.email, users.name, ue.confirmed_at

-- Find user by search term SQL query
select ue.id as "id", users.id as "user_id", ue.email as "username", ue.email as "email", users.name as "firstName", CASE WHEN ue.confirmed_at IS NOT NULL THEN 'TRUE' ELSE NULL END as "EMAIL_VERIFIED"from user_emails ue left join users on ue.user_id = users.id where LOWER(ue.email) like LOWER(concat('%', ?, '%')) or LOWER(users.name) like LOWER(concat('%', ?, '%'))
SELECT users.id AS "id", users.id AS "user_id", pe.email AS "username", pe.email AS "email", users.name AS "firstName", CASE WHEN MAX(CASE WHEN pe.confirmed_at IS NOT NULL THEN 1 ELSE 0 END) = 1 THEN 'TRUE' ELSE 'FALSE' END AS "EMAIL_VERIFIED" FROM user_emails ue LEFT JOIN users ON ue.user_id = users.id LEFT JOIN user_emails pe ON ue.user_id = pe.user_id AND pe.primary = TRUE WHERE LOWER(pe.email) LIKE LOWER(CONCAT('%', ?, '%')) OR LOWER(users.name) LIKE LOWER(CONCAT('%', ?, '%')) OR (ue.primary = FALSE AND ue.confirmed_at IS NOT NULL AND LOWER(ue.email) LIKE LOWER(CONCAT('%', ?, '%'))) GROUP BY pe.id, users.id, pe.email, users.name

-- Find password hash (blowfish or hash digest hex) SQL query
select encrypted_password as hash_pwd from users right join user_emails ue on users.id = ue.user_id where ue.email = ?
1 change: 1 addition & 0 deletions config/locales/en/user/emails.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ en:
emails:
index:
header: 'Emails'
cannot_delete_primary: 'You cannot delete the primary email.'
set_primary:
no_confirmed_emails: 'There are no confirmed emails to set as the primary email.'
send_confirmation:
Expand Down
1 change: 1 addition & 0 deletions config/locales/ko/user/emails.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ ko:
emails:
index:
header: '이메일'
cannot_delete_primary: '기본 이메일을 삭제할 수 없습니다.'
set_primary:
no_confirmed_emails: '기본 이메일로 설정할 수 있는 확인된 이메일 주소가 없습니다.'
send_confirmation:
Expand Down
1 change: 1 addition & 0 deletions config/locales/zh/user/emails.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ zh:
emails:
index:
header: '邮箱'
cannot_delete_primary: '不能删除主要邮箱。'
set_primary:
no_confirmed_emails: '没有已验证的邮箱地址可以设置为主要邮箱。'
send_confirmation:
Expand Down
25 changes: 17 additions & 8 deletions spec/controllers/user/emails_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@

with_tenant(:instance) do
let!(:user) { create(:administrator) }
let!(:primary_email) { user.send(:default_email_record) }
let!(:non_primary_email) { create(:user_email, user: user, primary: false) }
before { controller_sign_in(controller, user) }

describe '#destroy' do
subject { delete :destroy, as: :json, params: { id: user.send(:default_email_record) } }
describe '#destroy_primary_email' do
subject { delete :destroy, as: :json, params: { id: primary_email.id } }

context 'when the user only has one email address' do
it 'cannot be deleted' do
Expand All @@ -19,16 +21,23 @@
end

context 'when destroying a primary email' do
let!(:non_primary_email) { create(:user_email, user: user, primary: false) }
before { controller_sign_in(controller, user) }

it 'deletes the primary email' do
expect { subject }.to change { user.emails.count }.by(-1)
it 'cannot be deleted' do
expect { subject }.to change { user.emails.count }.by(0)
end
it { is_expected.to have_http_status(:bad_request) }
end
end

it 'sets another email as primary' do
subject
expect(non_primary_email.reload).to be_primary
Comment on lines -29 to -31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the old spec here, there's no issue deleting primary email since it will set another confirmed email as primary right?
In this case, it is unnecessary to introduce a user constraint to prevent primary email deletion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either this, or user.emails.set_primary.no_confirmed_emails is now redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, but as discussed before, we want to make the behavior in FE and BE in this regards to be consistent (FE does not allow deletion of primary email, and hence its related controller in BE should also have such requirements, and the testing needs to be done also to reflect on this change)

As for no_confirmed_emails, this is still relevant since actually even though controller prohibits deletion of primary email, it can still be done through other means (one example being to delete course_user, in which user_emails are one of its children and hence cascade delete takes place)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to update test case to change to disallow deletion of primary. But other test isn't necessary right, because user_emails tied to user, not course_user?

describe '#destroy_non_primary_email' do
subject { delete :destroy, as: :json, params: { id: non_primary_email.id } }

context 'when destroying a non-primary email' do
before { controller_sign_in(controller, user) }

it 'deletes that email' do
expect { subject }.to change { user.emails.count }.by(-1)
end
end
end
Expand Down