From bab8cbce9968ee116530c86f7e179c793adccf9e Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Wed, 15 Oct 2025 13:25:02 +0200 Subject: [PATCH 1/4] Change repoToRoleMapping to repurn nil if entity does not exist --- ee/rbac/lib/rbac/repo/repo_to_role_mapping.ex | 16 ++-- .../rbac/repo/repo_to_role_mapping_test.exs | 74 +++++++++++++++++++ 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/ee/rbac/lib/rbac/repo/repo_to_role_mapping.ex b/ee/rbac/lib/rbac/repo/repo_to_role_mapping.ex index c51031e06..7b3463c80 100644 --- a/ee/rbac/lib/rbac/repo/repo_to_role_mapping.ex +++ b/ee/rbac/lib/rbac/repo/repo_to_role_mapping.ex @@ -23,14 +23,18 @@ defmodule Rbac.Repo.RepoToRoleMapping do - pull_acess (boolean) Whehter you have pull access on specific repo """ @spec get_project_role_from_repo_access_rights(String.t(), boolean(), boolean(), boolean()) :: - String.t() + String.t() | nil def get_project_role_from_repo_access_rights(org_id, admin_access, push_access, pull_access) do - repo_to_role_mapping = get_repo_to_role_mapping(org_id) + case get_repo_to_role_mapping(org_id) do + nil -> + nil - case {admin_access, push_access, pull_access} do - {true, _, _} -> Map.get(repo_to_role_mapping, :admin_access_role_id) - {false, true, _} -> Map.get(repo_to_role_mapping, :push_access_role_id) - {false, false, true} -> Map.get(repo_to_role_mapping, :pull_access_role_id) + mapping -> + case {admin_access, push_access, pull_access} do + {true, _, _} -> mapping.admin_access_role_id + {false, true, _} -> mapping.push_access_role_id + {false, false, true} -> mapping.pull_access_role_id + end end end diff --git a/ee/rbac/test/rbac/repo/repo_to_role_mapping_test.exs b/ee/rbac/test/rbac/repo/repo_to_role_mapping_test.exs index 55ba11b4c..83099cc6c 100644 --- a/ee/rbac/test/rbac/repo/repo_to_role_mapping_test.exs +++ b/ee/rbac/test/rbac/repo/repo_to_role_mapping_test.exs @@ -27,4 +27,78 @@ defmodule Rbac.Repo.RepoToRoleMapping.Test do assert state[:mapping_for_specific_org] === mapper end end + + describe "get_project_role_from_repo_access_rights/4" do + test "returns nil when org has no RepoToRoleMapping for admin access" do + role_id = + RepoToRoleMapping.get_project_role_from_repo_access_rights( + @non_existant_org_id, + true, + false, + false + ) + + assert role_id === nil + end + + test "returns admin role when user has admin access", state do + role_id = + RepoToRoleMapping.get_project_role_from_repo_access_rights( + @org_id, + true, + false, + false + ) + + assert role_id === state[:mapping_for_specific_org].admin_access_role_id + end + + test "returns admin role when user has admin, push, and pull access", state do + role_id = + RepoToRoleMapping.get_project_role_from_repo_access_rights( + @org_id, + true, + true, + true + ) + + assert role_id === state[:mapping_for_specific_org].admin_access_role_id + end + + test "returns push role when user has push access only", state do + role_id = + RepoToRoleMapping.get_project_role_from_repo_access_rights( + @org_id, + false, + true, + false + ) + + assert role_id === state[:mapping_for_specific_org].push_access_role_id + end + + test "returns push role when user has push and pull access", state do + role_id = + RepoToRoleMapping.get_project_role_from_repo_access_rights( + @org_id, + false, + true, + true + ) + + assert role_id === state[:mapping_for_specific_org].push_access_role_id + end + + test "returns pull role when user has pull access only", state do + role_id = + RepoToRoleMapping.get_project_role_from_repo_access_rights( + @org_id, + false, + false, + true + ) + + assert role_id === state[:mapping_for_specific_org].pull_access_role_id + end + end end From f91b7fecbe3d061b633d67c2580727588aaece9e Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Wed, 15 Oct 2025 13:25:27 +0200 Subject: [PATCH 2/4] Skip assigning role to GH collaborators if repoToRoleMapping does not exist for the org --- ee/rbac/lib/rbac/role_management.ex | 5 +++-- ee/rbac/test/rbac/role_management_test.exs | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/ee/rbac/lib/rbac/role_management.ex b/ee/rbac/lib/rbac/role_management.ex index 6dc0d78f1..24174a27a 100644 --- a/ee/rbac/lib/rbac/role_management.ex +++ b/ee/rbac/lib/rbac/role_management.ex @@ -315,12 +315,12 @@ defmodule Rbac.RoleManagement do "[Role Management] Assigning roles based of collaborators list for RBI: #{inspect(rbi)}" ) - roles_to_be_assign = + roles_to_be_assigned = gen_query_to_assign_roles_to_collaborators(rbi) |> Rbac.Repo.all() list_of_subject_role_bindings = - Enum.map(roles_to_be_assign, fn binding -> + Enum.map(roles_to_be_assigned, fn binding -> %{ role_id: Rbac.Repo.RepoToRoleMapping.get_project_role_from_repo_access_rights( @@ -335,6 +335,7 @@ defmodule Rbac.RoleManagement do binding_source: String.to_atom(binding[:provider]) } end) + |> Enum.filter(fn binding -> binding.role_id != nil end) assign_roles(list_of_subject_role_bindings, rbi) end diff --git a/ee/rbac/test/rbac/role_management_test.exs b/ee/rbac/test/rbac/role_management_test.exs index 55f2ad6f1..aed24c0eb 100644 --- a/ee/rbac/test/rbac/role_management_test.exs +++ b/ee/rbac/test/rbac/role_management_test.exs @@ -298,6 +298,25 @@ defmodule Rbac.RoleManagement.Test do assert user_has_access_to_projects?(@user_id, @org_id, [@project_id]) end + + test "Organization does not have RepoToRoleMapping" do + Support.Rbac.assign_org_role_by_name(@org_id, @user_id, "Member") + Collaborators.insert_user(user_id: @user_id, github_uid: "1") + Collaborators.insert(github_uid: "1", project_id: @project_id) + Support.Projects.insert(project_id: @project_id, org_id: @org_id) + + # Note: We deliberately do NOT create a RepoToRoleMapping for this org + + {:ok, project_rbi} = RBI.new(org_id: @org_id, project_id: @project_id) + + # Should succeed but skip role assignment due to missing RepoToRoleMapping + {:ok, _} = RoleManagement.assign_project_roles_to_repo_collaborators(project_rbi) + + # Verify no project role was assigned to the collaborator + {:ok, admin} = Rbac.Repo.RbacRole.get_role_by_name("Admin", "project_scope", @org_id) + refute has_role_binding?(@user_id, @org_id, @project_id, admin.id, :github) + refute user_has_access_to_projects?(@user_id, @org_id, [@project_id]) + end end describe "retract_role/2" do From 9dff0b8055e615f578de651be9090f917f82cdc0 Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Wed, 15 Oct 2025 17:12:08 +0200 Subject: [PATCH 3/4] Collaborator refresh worker wont do anything if there is no RepoToRoleMapping --- ee/rbac/lib/rbac/collaborators_refresher.ex | 18 ++-- .../rbac/collaborators_refresher_test.exs | 88 +++++++++++++++++++ 2 files changed, 98 insertions(+), 8 deletions(-) diff --git a/ee/rbac/lib/rbac/collaborators_refresher.ex b/ee/rbac/lib/rbac/collaborators_refresher.ex index f47636520..1bb4ebcfc 100644 --- a/ee/rbac/lib/rbac/collaborators_refresher.ex +++ b/ee/rbac/lib/rbac/collaborators_refresher.ex @@ -110,14 +110,16 @@ defmodule Rbac.CollaboratorsRefresher do collaborator["permissions"]["pull"] ) - Rbac.Repo.RbacRefreshProjectAccessRequest.add_request( - project.org_id, - user_id, - project_id, - :add, - source, - role_to_be_assigned - ) + if role_to_be_assigned do + Rbac.Repo.RbacRefreshProjectAccessRequest.add_request( + project.org_id, + user_id, + project_id, + :add, + source, + role_to_be_assigned + ) + end end end) end diff --git a/ee/rbac/test/rbac/collaborators_refresher_test.exs b/ee/rbac/test/rbac/collaborators_refresher_test.exs index 74fe7f5b4..4b6f95f7a 100644 --- a/ee/rbac/test/rbac/collaborators_refresher_test.exs +++ b/ee/rbac/test/rbac/collaborators_refresher_test.exs @@ -227,5 +227,93 @@ defmodule Rbac.CollaboratorsRefresher.Test do |> Rbac.Repo.one() == nil end end + + test "does not create refresh requests when org has no RepoToRoleMapping" do + alias InternalApi.Repository.Collaborator + + list_collaborators = %InternalApi.Repository.ListCollaboratorsResponse{ + next_page_token: "", + collaborators: [ + %Collaborator{id: "3", login: "baz", permission: :WRITE}, + %Collaborator{id: "4", login: "bam", permission: :READ} + ] + } + + GrpcMock.stub(RepositoryMock, :list_collaborators, fn _, _ -> + list_collaborators + end) + + project = Support.Factories.project() + + response = %InternalApi.Projecthub.DescribeResponse{ + metadata: Support.Factories.response_meta(), + project: project + } + + GrpcMock.stub(ProjecthubMock, :describe, fn _, _ -> response end) + + {:ok, project} = + Rbac.Store.Project.update( + project.metadata.id, + "renderedtext/semaphore2", + "15324ba0-1b20-49d0-8ff9-a2d91fa451e0", + "github", + "private" + ) + + [user1, user2, user3, user4] = + rbac_users = + 1..4 + |> Enum.map(fn _ -> + {:ok, user} = Support.Factories.RbacUser.insert() + user + end) + + Enum.each(rbac_users, fn user -> + Support.Members.insert_user( + id: user.id, + email: user.email, + name: user.name + ) + end) + + {:ok, _org_scope} = Support.Factories.Scope.insert("org_scope") + + Support.Factories.SubjectRoleBinding.insert( + subject_id: user3.id, + org_id: project.org_id, + binding_source: :manually_assigned + ) + + Support.Factories.SubjectRoleBinding.insert( + subject_id: user4.id, + org_id: project.org_id, + binding_source: :manually_assigned + ) + + with_mocks [ + {Rbac.Store.User, [], + [ + find_id_by_provider_uid: fn github_uid, _ -> + case github_uid do + "1" -> user1.id + "2" -> user2.id + "3" -> user3.id + "4" -> user4.id + end + end + ]} + ] do + assert :ok = Rbac.CollaboratorsRefresher.refresh(project) + + # Giving time for message broker to process + :timer.sleep(500) + Rbac.Workers.RefreshProjectAccess.perform_now() + :timer.sleep(1000) + + refresh_requests = Rbac.Repo.RbacRefreshProjectAccessRequest |> Rbac.Repo.all() + assert refresh_requests == [] + end + end end end From 57966b80fdf0b4ca6fcfe6bc7c31a31ac0dce2f1 Mon Sep 17 00:00:00 2001 From: VeljkoMaksimovic Date: Wed, 15 Oct 2025 17:25:40 +0200 Subject: [PATCH 4/4] Fix soblew warning --- ee/rbac/lib/rbac/okta/scim/api.ex | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/rbac/lib/rbac/okta/scim/api.ex b/ee/rbac/lib/rbac/okta/scim/api.ex index f31de9da7..e066ab27c 100644 --- a/ee/rbac/lib/rbac/okta/scim/api.ex +++ b/ee/rbac/lib/rbac/okta/scim/api.ex @@ -258,7 +258,9 @@ defmodule Rbac.Okta.Scim.Api do end defp json(conn, status, payload) do - send_resp(conn, status, Jason.encode!(payload)) + conn + |> put_resp_content_type("application/json") + |> send_resp(status, Jason.encode!(payload)) end defp serialize_user(okta_user) do