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.

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

Conversation

bivanalhar
Copy link
Contributor

Problem Statement

Natively, Keycloak handles the case of different emails as different entities entirely, even though they might actually be used by the same user (Coursemology supports multiple emails to be associated with one user). This leads to the problem of when a user logs in using 2 different emails, then in other services (such as Cikgo or Koditsu) those 2 emails will be regarded as belonging to 2 different users, which is wrong. This PR resolves this particular issue

Approach

Previously we setup the user ID inside Keycloak by using an ID registered under user_emails table. Because we want to regard all emails from a single user as the same entity under Keycloak, we instead use an ID from users table. In this way, no matter which email is being used by the user, they will all be directed towards one single entity by Keycloak and hence will be regarded correctly as being the same user across platform as well.

This approach has a drawback, though, in which should a user has an unconfirmed email being registered, then logging in using any confirmed email, then later on logging in using this unconfirmed one within different session will gives user a successful login (since Keycloak already cached the user's data inside their own DB)

Therefore, to mitigate this issue, we also add the filtering of all unconfirmed email out of the search results everytime Keycloak makes a query to Coursemology DB. This will ensure that user won't be able to login using unconfirmed email and hence only be able to login using confirmed one

Trade-Off

Even though the proposed approach resolves the single-user multiple-email issue, as well as keeping the logging in through unconfirmed email impossible, there is a trade-off in the behaviour of logging in through unconfirmed email.

Previously, if we do that, we will be redirected towards the Unconfirmed Email page, in which there will be a hyperlink to trigger Coursemology to resend the confirmation email to user. Now, doing so will only render the email invalid and hence only the warning of "Invalid username or password" will be generated, and no redirection will happen

Other Issues being Resolved

Other than single-user multiple-email issue, I also noticed that there is no validation that occurs while trying to delete the email through controller. By rights, we should not be able to delete a primary email (this has been handled well in our Frontend), but right now we can merely call the API for destroy primary email directly and it will be successful. Therefore, I added the gate-keeping of such in this PR as well

@bivanalhar bivanalhar changed the title Using CM's user ID to setup Keycloak UserID Resolve Single-User Multiple-Email Issue in using CM-Keycloak Dec 27, 2024
@bivanalhar bivanalhar force-pushed the bivan/keycloak-multi-email branch 2 times, most recently from 203f5ea to 57fc741 Compare December 27, 2024 06:52
Comment on lines -29 to -31
it 'sets another email as primary' do
subject
expect(non_primary_email.reload).to be_primary
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?

@cysjonathan cysjonathan changed the title Resolve Single-User Multiple-Email Issue in using CM-Keycloak fix(auth): Single-User Multiple-Email Issue using CM-Keycloak Jan 2, 2025
@bivanalhar bivanalhar force-pushed the bivan/keycloak-multi-email branch 2 times, most recently from e98a64e to 9a31752 Compare January 15, 2025 09:49
@bivanalhar bivanalhar force-pushed the bivan/keycloak-multi-email branch from 9a31752 to e62be73 Compare January 15, 2025 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants