feat(users): Add email domain based restriction for dashboard entry APIs#6940
Merged
Gnanasundari24 merged 8 commits intomainfrom Dec 30, 2024
Merged
feat(users): Add email domain based restriction for dashboard entry APIs#6940Gnanasundari24 merged 8 commits intomainfrom
Gnanasundari24 merged 8 commits intomainfrom
Conversation
…ions in user APIs
…-domain-restriction
Changed Files
|
crates/router/src/types/domain/user/user_authentication_method.rs
Outdated
Show resolved
Hide resolved
apoorvdixit88
approved these changes
Dec 26, 2024
inventvenkat
approved these changes
Dec 26, 2024
racnan
approved these changes
Dec 26, 2024
Contributor
racnan
left a comment
There was a problem hiding this comment.
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.
Comment on lines
+326
to
+327
| pub auth_id: Option<String>, | ||
| pub email_domain: Option<String>, |
Comment on lines
+2350
to
+2362
| 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(), | ||
| }; |
Comment on lines
+2486
to
+2496
| 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) |
Contributor
There was a problem hiding this comment.
can we make in query for this ?
Comment on lines
+2520
to
+2521
| (Some(_), Some(_)) | (None, None) => { | ||
| return Err(UserErrors::InvalidUserAuthMethodOperation.into()); |
Contributor
There was a problem hiding this comment.
as mentioned above, if api model is enum we can avoid such cases here.
| StorageError::DuplicateValue { .. } => { | ||
| UserErrors::UserAuthMethodAlreadyExists | ||
| } | ||
| _ => UserErrors::InternalServerError, |
Member
There was a problem hiding this comment.
Can we have some context message included wherever we're raising internal server errors (here and elsewhere in the file)?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 --allcargo clippy