Skip to content

support email login_hint #4568

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mcalinghee
Copy link
Contributor

@mcalinghee mcalinghee commented May 15, 2025

It is now possible to forward the login_hint with a mxid to the upstream providers thanks to #4512

This PR allows us to support the login_hint with an email when login_with_email_allowed is activated.
In particular this PR will display the login_hint ie. email on the MAS login page.
It will also forward the email login_hint to upstream providers.

@mcalinghee mcalinghee marked this pull request as draft May 15, 2025 08:47
@mcalinghee mcalinghee force-pushed the feat/login_hint_with_email branch from 86e806c to 34762a6 Compare May 15, 2025 11:32
@mcalinghee mcalinghee marked this pull request as ready for review May 15, 2025 11:36
@mcalinghee mcalinghee force-pushed the feat/login_hint_with_email branch from 34762a6 to 9c9cc3d Compare June 3, 2025 13:15
@mcalinghee mcalinghee force-pushed the feat/login_hint_with_email branch 3 times, most recently from 0a1c9a2 to 52bb16b Compare June 5, 2025 13:41
@mcalinghee mcalinghee requested a review from sandhose June 5, 2025 13:42
@mcalinghee mcalinghee marked this pull request as draft June 5, 2025 16:09
@mcalinghee mcalinghee force-pushed the feat/login_hint_with_email branch from 52bb16b to 07ac7ca Compare June 5, 2025 16:17
@mcalinghee mcalinghee marked this pull request as ready for review June 5, 2025 16:18
@mcalinghee mcalinghee force-pushed the feat/login_hint_with_email branch 7 times, most recently from 2e23003 to 28a15d4 Compare June 12, 2025 14:14
@mcalinghee mcalinghee force-pushed the feat/login_hint_with_email branch from 28a15d4 to d18fcaa Compare June 16, 2025 10:11
@mcalinghee mcalinghee requested a review from MatMaul June 16, 2025 13:39
@odelcroi odelcroi force-pushed the feat/login_hint_with_email branch from d18fcaa to a5d3a2f Compare July 30, 2025 06:51
@mcalinghee mcalinghee changed the title email login_hint support when login_with_email_allowed is activated forward email login_hint to upstream providers Jul 30, 2025
@mcalinghee mcalinghee changed the title forward email login_hint to upstream providers support email login_hint Jul 30, 2025
#[must_use]
pub fn parse_login_hint(&self, homeserver: &str) -> LoginHint {
pub fn parse_login_hint(&self, homeserver: &str, login_with_email_allowed: bool) -> LoginHint {
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense to have the upper level handle this logic: always parse the email login hint, and let the caller decide what to do with it

Comment on lines 336 to 334
#[allow(clippy::disallowed_methods)]
let mut rng = thread_rng();

#[allow(clippy::disallowed_methods)]
let now = Utc::now();
Copy link
Member

Choose a reason for hiding this comment

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

Please use a static RNG and the MockClock in tests instead

Suggested change
#[allow(clippy::disallowed_methods)]
let mut rng = thread_rng();
#[allow(clippy::disallowed_methods)]
let now = Utc::now();
let now = MockClock::default().now();
let mut rng = rand_chacha::ChaChaRng::seed_from_u64(42);

There is a reason for those methods are not allowed: we want tests to have a stable outcome, so that we can use snapshots when needed

Copy link
Contributor Author

@mcalinghee mcalinghee Jul 30, 2025

Choose a reason for hiding this comment

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

agreed but I would need to add a dependency on mas-storage on the data-model crate and this would end up as a cyclic dependency.
If this is ok, I would extract these static function into data-model to be able to use them in both mas-storage and data-model but there will be a bit of code refactor in this PR

Copy link
Member

Choose a reason for hiding this comment

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

Hah, of course! If you feel like doing it, you can move Clock/MockClock/SystemClock/BoxClock/BoxRng to mas-data-model, either in a separate PR or in this one

I'm fine with keeping it like that if it's too annoying to refactor now

Copy link
Contributor Author

@mcalinghee mcalinghee Jul 31, 2025

Choose a reason for hiding this comment

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

Ok job done.

It was a bit annoying but I have done most of the refactoring in c314802 and brought minor corrections afterwards. I hope this is ok.

Hopefully, it will not be too painful to perform the review.
Let me know if you prefer to have this in another PR as the refacto is quite heavy.

Comment on lines 395 to 396
LoginHint::Email(email) => Some(email.to_string()),
LoginHint::None => None,
Copy link
Member

Choose a reason for hiding this comment

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

the logic would then be something like

let value = match grant.parse_login_hint(homeserver.homeserver()) {
    LoginHint::MXID(mxid) => Some(mxid.localpart().to_owned()),
    LoginHint::Email(email) if site_config.login_with_email_allowed => Some(email.to_string()),
    _ => None,
};

@mcalinghee mcalinghee force-pushed the feat/login_hint_with_email branch from 9b1c45f to cdf53ca Compare July 30, 2025 13:19
@sandhose
Copy link
Member

@mcalinghee after an initial review, I would appreciate if you don't force-push to the branch, as it often makes GitHub confused and makes it harder to understand what changed. If you ever need to update from the main branch, prefer merging it into your branch instead of rebasing :)

@mcalinghee mcalinghee requested a review from sandhose July 31, 2025 11:12
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.

3 participants