-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(users): Add email domain based restriction for dashboard entry APIs #6940
Conversation
…ions in user APIs
…-domain-restriction
Changed Files
|
crates/router/src/types/domain/user/user_authentication_method.rs
Outdated
Show resolved
Hide resolved
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.
This can be later changed to have email domain as a nullable column instead.
We shouldn't force users to always add a domain based restriction.
pub auth_id: Option<String>, | ||
pub email_domain: Option<String>, |
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.
could have been an enum.
let (auth_id, email_domain) = if let Some(auth_method) = auth_methods.first() { | ||
let email_domain = match req.email_domain { | ||
Some(email_domain) => { | ||
if email_domain != auth_method.email_domain { | ||
return Err(report!(UserErrors::InvalidAuthMethodOperationWithMessage( | ||
"Email domain mismatch".to_string() | ||
))); | ||
} | ||
|
||
email_domain | ||
} | ||
None => auth_method.email_domain.clone(), | ||
}; |
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.
nesting can be avoided.
futures::future::try_join_all(auth_methods.iter().map(|auth_method| async { | ||
state | ||
.store | ||
.update_user_authentication_method( | ||
&auth_method.id, | ||
UserAuthenticationMethodUpdate::EmailDomain { | ||
email_domain: email_domain.clone(), | ||
}, | ||
) | ||
.await | ||
.to_duplicate_response(UserErrors::UserAuthMethodAlreadyExists) |
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.
can we make in
query for this ?
(Some(_), Some(_)) | (None, None) => { | ||
return Err(UserErrors::InvalidUserAuthMethodOperation.into()); |
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.
as mentioned above, if api model is enum
we can avoid such cases here.
StorageError::DuplicateValue { .. } => { | ||
UserErrors::UserAuthMethodAlreadyExists | ||
} | ||
_ => UserErrors::InternalServerError, |
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.
Can we have some context message included wherever we're raising internal server errors (here and elsewhere in the file)?
Type of Change
Description
There will be a mapping of email domain and auth methods.
This PR will restrict users to enter into the dashboard based on the email domain.
Additional Changes
Motivation and Context
Closes #6939.
How did you test it?
Create user auth method with email domain
Response will be 200 OK.
Update user auth method
All the following APIs will restrict the users based on the email domain and the corresponding auth method
If the auth method list for email domain is empty of consists of the above auth method, the request will be continued, or else the api will be stopped.
Checklist
cargo +nightly fmt --all
cargo clippy