refactor(users): refactor find_by_role_id_in_merchant_scope query for custom roles #6701

Riddhiagrawal001 · 2024-11-29T06:19:36Z

Type of Change

Description

Refactor the find_by_role_id_in_merchant_scope query to make it a generic query that uses only org_id and role_id as parameters.

Additional Changes

This PR modifies the API contract
This PR modifies the database schema
This PR modifies application configuration/environment variables

Motivation and Context

Closes 6702

How did you test it?

Using dashboard . All the below should work as before
Cases to test :

Create a custom role from dashboard
Invite someone for that custom role
signin via that custom-role-user
List roles and list roles at entity level

Checklist

I formatted the code cargo +nightly fmt --all
I addressed lints thrown by cargo clippy
I reviewed the submitted code
I added unit tests for my changes where possible

… custom roles

semanticdiff-com · 2024-11-29T06:19:39Z

Review changes with

Changed Files

File	Status
crates/router/src/utils/user.rs	87% smaller
crates/router/src/analytics.rs	84% smaller
crates/diesel_models/src/user_role.rs	78% smaller
crates/router/src/utils/user_role.rs	75% smaller
crates/router/src/services/authorization.rs	65% smaller
crates/router/src/core/user_role/role.rs	65% smaller
crates/diesel_models/src/query/role.rs	27% smaller
crates/router/src/services/authorization/roles.rs	18% smaller
crates/router/src/db/role.rs	4% smaller
crates/router/src/db/kafka_store.rs	4% smaller
crates/router/src/core/user.rs	3% smaller
crates/router/src/core/user_role.rs	0% smaller
migrations/2024-12-02-110129_update-user-role-entity-type/down.sql	Unsupported file format
migrations/2024-12-02-110129_update-user-role-entity-type/up.sql	Unsupported file format

ThisIsMani · 2024-11-29T13:09:30Z

crates/router/src/core/user_role.rs

-    .await
-    .to_not_found_response(UserErrors::InvalidRoleId)?;
+    let role_info =
+        roles::RoleInfo::from_role_id_and_org_id(&state, &req.role_id, &user_from_token.org_id)


There should be lineage check here.

ThisIsMani · 2024-11-29T13:14:53Z

crates/router/src/core/user_role/role.rs

    if matches!(role_info.get_scope(), RoleScope::Organization)
        && user_from_token.role_id != common_utils::consts::ROLE_ID_ORGANIZATION_ADMIN


Can we check this with entity type instead of role_id.

ThisIsMani · 2024-11-29T13:28:32Z

crates/router/src/core/user_role/role.rs

+    let role_info =
+        roles::RoleInfo::from_role_id_and_org_id(&state, role_id, &user_from_token.org_id)
+            .await
+            .to_not_found_response(UserErrors::InvalidRoleOperation)?;


We should also check if user has access to update this role. This can be done by getting the role using the lineage.

crates/router/src/services/authorization/roles.rs

ThisIsMani · 2024-11-29T13:37:01Z

crates/router/src/utils/user_role.rs

 }

-pub async fn set_role_permissions_in_cache_by_role_id_merchant_id_org_id(
+pub async fn set_role_permissions_in_cache_by_role_id_org_id(


Can we rename these functions to set_role_info... instead of set_permissions....

…witch into change-roles-query

ThisIsMani · 2024-12-02T12:48:43Z

crates/diesel_models/src/query/role.rs

@@ -26,6 +26,7 @@ impl Role {
        .await
    }

+    // TODO:remove once find_by_role_id_in_lineage is stable


Suggested change

// TODO:remove once find_by_role_id_in_lineage is stable

// TODO: Remove once find_by_role_id_in_lineage is stable

ThisIsMani · 2024-12-02T12:52:13Z

crates/diesel_models/src/user_role.rs

+        match self.version {
+            enums::UserRoleVersion::V1 if self.entity_type.is_none() => {
+                match self.role_id.as_str() {
+                    consts::ROLE_ID_ORGANIZATION_ADMIN => {
+                        let org_id = self.org_id.clone()?.get_string_repr().to_string();
+                        Some((org_id, EntityType::Organization))
+                    }
+                    _ => {
+                        let merchant_id = self.merchant_id.clone()?.get_string_repr().to_string();
+                        Some((merchant_id, EntityType::Merchant))
+                    }
+                }
            }


Suggested change

match self.version {

enums::UserRoleVersion::V1 if self.entity_type.is_none() => {

match self.role_id.as_str() {

consts::ROLE_ID_ORGANIZATION_ADMIN => {

let org_id = self.org_id.clone()?.get_string_repr().to_string();

Some((org_id, EntityType::Organization))

}

_ => {

let merchant_id = self.merchant_id.clone()?.get_string_repr().to_string();

Some((merchant_id, EntityType::Merchant))

}

}

}

match (self.version, self.entity_type, self.role_id) {

(enums::UserRoleVersion::V1, None, ...) => ...

... => ...

ThisIsMani · 2024-12-02T12:56:30Z

crates/router/src/core/user_role.rs

@@ -489,7 +485,7 @@ pub async fn delete_user_role(
            .attach_printable("User deleting himself");
    }

-    let deletion_requestor_role_info = roles::RoleInfo::from_role_id_in_merchant_scope(
+    let deletion_requestor_role_info = roles::RoleInfo::from_role_id_in_lineage(


lineage check is not required here.

ThisIsMani · 2024-12-02T12:56:53Z

crates/router/src/core/user_role.rs

@@ -527,10 +523,9 @@ pub async fn delete_user_role(
    };

    if let Some(role_to_be_deleted) = user_role_v2 {
-        let target_role_info = roles::RoleInfo::from_role_id_in_merchant_scope(
+        let target_role_info = roles::RoleInfo::from_role_id_and_org_id(


lineage check should be here instead.

ThisIsMani · 2024-12-02T12:57:03Z

crates/router/src/core/user_role.rs

@@ -597,10 +592,9 @@ pub async fn delete_user_role(
    };

    if let Some(role_to_be_deleted) = user_role_v1 {
-        let target_role_info = roles::RoleInfo::from_role_id_in_merchant_scope(
+        let target_role_info = roles::RoleInfo::from_role_id_and_org_id(


Here as well.

ThisIsMani · 2024-12-02T12:58:13Z

crates/router/src/core/user_role/role.rs

+    let user_from_token_role_info = roles::RoleInfo::from_role_id_and_org_id(
+        &state,
+        &user_from_token.role_id,
+        &user_from_token.org_id,
+    )
+    .await
+    .to_not_found_response(UserErrors::InvalidRoleId)?;


There is a function already implemented on UserFromToken which will give the role_info.

You can use that instead.

ThisIsMani · 2024-12-02T12:58:52Z

crates/router/src/core/user_role/role.rs

@@ -216,8 +208,16 @@ pub async fn update_role(
    .await
    .to_not_found_response(UserErrors::InvalidRoleOperation)?;

+    let user_from_token_role_info = roles::RoleInfo::from_role_id_and_org_id(


Suggested change

let user_from_token_role_info = roles::RoleInfo::from_role_id_and_org_id(

let user_role_info = roles::RoleInfo::from_role_id_and_org_id(

ThisIsMani · 2024-12-02T13:00:37Z

crates/router/src/utils/user_role.rs

@@ -72,31 +72,21 @@ pub async fn set_role_permissions_in_cache_by_user_role(
    state: &SessionState,


This function name also should be changed.

ThisIsMani · 2024-12-02T13:00:46Z

crates/router/src/utils/user_role.rs

@@ -105,18 +95,16 @@ pub async fn set_role_permissions_in_cache_by_role_id_merchant_id_org_id(
 pub async fn set_role_permissions_in_cache_if_required(


Here as well.

ThisIsMani · 2024-12-02T13:01:38Z

Can you change the title of the PR.

ThisIsMani · 2024-12-02T13:05:14Z

As this supposed to be intermediate PR, shouldn't you add Profile in RoleScope?

Riddhiagrawal001 added 2 commits November 28, 2024 15:44

chore: rename find_role_by_role_id_in_org_scopr

9c04ad6

refactor(users): refactor find_by_role_id_in_merchant_scope query for…

35b89e8

… custom roles

Riddhiagrawal001 requested review from a team as code owners November 29, 2024 06:19

Riddhiagrawal001 and others added 2 commits November 29, 2024 12:46

fix: changes for list roles and list roles at entity-level

5cb5954

Merge branch 'main' into change-roles-query

16f6f59

Riddhiagrawal001 self-assigned this Nov 29, 2024

Riddhiagrawal001 linked an issue Nov 29, 2024 that may be closed by this pull request

refactor(users): refactor find_by_role_id_in_merchant_scope query for custom roles #6702

Open

Merge branch 'main' into change-roles-query

d18866e

ThisIsMani requested changes Nov 29, 2024

View reviewed changes

Riddhiagrawal001 and others added 3 commits December 2, 2024 16:34

fix: changes for backfilling user-roles

1137886

Merge branch 'change-roles-query' of https://github.com/juspay/hypers…

6606067

…witch into change-roles-query

Merge branch 'main' into change-roles-query

aeec4bf

hyperswitch-bot bot added the M-database-changes Metadata: This PR involves database schema changes label Dec 2, 2024

ThisIsMani reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(users): refactor find_by_role_id_in_merchant_scope query for custom roles #6701

refactor(users): refactor find_by_role_id_in_merchant_scope query for custom roles #6701

Riddhiagrawal001 commented Nov 29, 2024 •

edited

Loading

semanticdiff-com bot commented Nov 29, 2024 •

edited

Loading

ThisIsMani Nov 29, 2024

ThisIsMani Nov 29, 2024

ThisIsMani Nov 29, 2024

ThisIsMani Nov 29, 2024

ThisIsMani Dec 2, 2024

ThisIsMani Dec 2, 2024

ThisIsMani Dec 2, 2024

ThisIsMani Dec 2, 2024

ThisIsMani Dec 2, 2024

ThisIsMani Dec 2, 2024

ThisIsMani Dec 2, 2024

ThisIsMani Dec 2, 2024

ThisIsMani Dec 2, 2024

ThisIsMani commented Dec 2, 2024

ThisIsMani commented Dec 2, 2024

		if matches!(role_info.get_scope(), RoleScope::Organization)
		&& user_from_token.role_id != common_utils::consts::ROLE_ID_ORGANIZATION_ADMIN

	// TODO:remove once find_by_role_id_in_lineage is stable
	// TODO: Remove once find_by_role_id_in_lineage is stable

	let user_from_token_role_info = roles::RoleInfo::from_role_id_and_org_id(
	let user_role_info = roles::RoleInfo::from_role_id_and_org_id(

		@@ -72,31 +72,21 @@ pub async fn set_role_permissions_in_cache_by_user_role(
		state: &SessionState,

		@@ -105,18 +95,16 @@ pub async fn set_role_permissions_in_cache_by_role_id_merchant_id_org_id(
		pub async fn set_role_permissions_in_cache_if_required(

refactor(users): refactor find_by_role_id_in_merchant_scope query for custom roles #6701

Are you sure you want to change the base?

refactor(users): refactor find_by_role_id_in_merchant_scope query for custom roles #6701

Conversation

Riddhiagrawal001 commented Nov 29, 2024 • edited Loading

Type of Change

Description

Additional Changes

Motivation and Context

How did you test it?

Checklist

semanticdiff-com bot commented Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThisIsMani commented Dec 2, 2024

ThisIsMani commented Dec 2, 2024

Riddhiagrawal001 commented Nov 29, 2024 •

edited

Loading

semanticdiff-com bot commented Nov 29, 2024 •

edited

Loading