-
Notifications
You must be signed in to change notification settings - Fork 47
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
base: main
Are you sure you want to change the base?
support email login_hint
#4568
Conversation
86e806c
to
34762a6
Compare
34762a6
to
9c9cc3d
Compare
0a1c9a2
to
52bb16b
Compare
52bb16b
to
07ac7ca
Compare
2e23003
to
28a15d4
Compare
28a15d4
to
d18fcaa
Compare
d18fcaa
to
a5d3a2f
Compare
login_hint
to upstream providers
login_hint
to upstream providerslogin_hint
#[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 { |
There was a problem hiding this comment.
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
#[allow(clippy::disallowed_methods)] | ||
let mut rng = thread_rng(); | ||
|
||
#[allow(clippy::disallowed_methods)] | ||
let now = Utc::now(); |
There was a problem hiding this comment.
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
#[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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
crates/handlers/src/views/login.rs
Outdated
LoginHint::Email(email) => Some(email.to_string()), | ||
LoginHint::None => None, |
There was a problem hiding this comment.
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,
};
9b1c45f
to
cdf53ca
Compare
@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 :) |
…correct documentation
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 whenlogin_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.