Skip to content
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(repository): add new check repository_merging_requires_conversation_resolution #6208

Open
wants to merge 4 commits into
base: PRWLR-5536-ensure-branch-protection-rules-are-enforced-for-administrators
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"Provider": "github",
"CheckID": "repository_merging_requires_conversation_resolution",
"CheckTitle": "Check if repository requires conversation resolution before merging",
"CheckType": [],
"ServiceName": "repository",
"SubServiceName": "",
"ResourceIdTemplate": "",
"Severity": "medium",
"ResourceType": "Other",
"Description": "Ensure that the repository requires conversation resolution before merging.",
"Risk": "Leaving comments unresolved before merging code can lead to overlooked issues, including potential bugs or security vulnerabilities, that might affect the quality and security of the codebase. Unaddressed concerns could result in a lower quality of code, increasing the risk of production failures or breaches.",
"RelatedUrl": "https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-conversation-resolution-before-merging",
"Remediation": {
"Code": {
"CLI": "",
"NativeIaC": "",
"Other": "",
"Terraform": ""
},
"Recommendation": {
"Text": "Ensure that all comments in a code change proposal are resolved before merging. This guarantees that every reviewer’s concern is addressed, improving code quality and security by preventing issues from being ignored or overlooked.",
"Url": "https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/managing-a-branch-protection-rule"
}
},
"Categories": [],
"DependsOn": [],
"RelatedTo": [],
"Notes": ""
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
from typing import List

from prowler.lib.check.models import Check, Check_Report_Github
from prowler.providers.github.services.repository.repository_client import (
repository_client,
)


class repository_merging_requires_conversation_resolution(Check):
"""Check if a repository requires conversation resolution

This class verifies whether each repository requires conversation resolution.
"""

def execute(self) -> List[Check_Report_Github]:
"""Execute the Github Repository Merging Requires Conversation Resolution check

Iterates over all repositories and checks if they require conversation resolution.

Returns:
List[Check_Report_Github]: A list of reports for each repository
"""
findings = []
for repo in repository_client.repositories.values():
report = Check_Report_Github(self.metadata())
report.resource_id = repo.id
report.resource_name = repo.name
report.status = "FAIL"
report.status_extended = (
f"Repository {repo.name} does not require conversation resolution."
)

if (
repo.default_branch_protection
and repo.default_branch_protection.conversation_resolution
):
report.status = "PASS"
report.status_extended = (
f"Repository {repo.name} does require conversation resolution."
)

findings.append(report)

return findings
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def _list_repositories(self):
allow_branch_deletion=protection.allow_deletions,
enforce_status_checks=protection.required_status_checks.strict,
enforce_admins=protection.enforce_admins,
conversation_resolution=protection.required_conversation_resolution,
)

except Exception as e:
Expand Down Expand Up @@ -86,6 +87,7 @@ class Protection(BaseModel):
allow_branch_deletion: bool = True
enforce_status_checks: bool = False
enforce_admins: bool = False
conversation_resolution: bool = False


class Repo(BaseModel):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
from unittest import mock

from prowler.providers.github.services.repository.repository_service import (
Protection,
Repo,
)
from tests.providers.github.github_fixtures import set_mocked_github_provider


class Test_repository_merging_requires_conversation_resolution_test:
def test_no_repositories(self):
repository_client = mock.MagicMock
repository_client.repositories = {}

with (
mock.patch(
"prowler.providers.common.provider.Provider.get_global_provider",
return_value=set_mocked_github_provider(),
),
mock.patch(
"prowler.providers.github.services.repository.repository_merging_requires_conversation_resolution.repository_merging_requires_conversation_resolution.repository_client",
new=repository_client,
),
):
from prowler.providers.github.services.repository.repository_merging_requires_conversation_resolution.repository_merging_requires_conversation_resolution import (
repository_merging_requires_conversation_resolution,
)

check = repository_merging_requires_conversation_resolution()
result = check.execute()
assert len(result) == 0

def test_conversation_resolution_disabled(self):
repository_client = mock.MagicMock
repo_name = "repo1"
default_branch = "main"
repository_client.repositories = {
1: Repo(
id=1,
name=repo_name,
full_name="account-name/repo1",
default_branch=default_branch,
default_branch_protection=Protection(
require_pull_request=True, approval_count=2, linear_history=False
),
private=False,
securitymd=False,
),
}

with (
mock.patch(
"prowler.providers.common.provider.Provider.get_global_provider",
return_value=set_mocked_github_provider(),
),
mock.patch(
"prowler.providers.github.services.repository.repository_merging_requires_conversation_resolution.repository_merging_requires_conversation_resolution.repository_client",
new=repository_client,
),
):
from prowler.providers.github.services.repository.repository_merging_requires_conversation_resolution.repository_merging_requires_conversation_resolution import (
repository_merging_requires_conversation_resolution,
)

check = repository_merging_requires_conversation_resolution()
result = check.execute()
assert len(result) == 1
assert result[0].resource_id == 1
assert result[0].resource_name == "repo1"
assert result[0].status == "FAIL"
assert (
result[0].status_extended
== f"Repository {repo_name} does not require conversation resolution."
)

def test_conversation_resolution_enabled(self):
repository_client = mock.MagicMock
repo_name = "repo1"
default_branch = "main"
repository_client.repositories = {
1: Repo(
id=1,
name=repo_name,
full_name="account-name/repo1",
private=False,
default_branch=default_branch,
default_branch_protection=Protection(
require_pull_request=True,
approval_count=2,
conversation_resolution=True,
),
securitymd=True,
),
}

with (
mock.patch(
"prowler.providers.common.provider.Provider.get_global_provider",
return_value=set_mocked_github_provider(),
),
mock.patch(
"prowler.providers.github.services.repository.repository_merging_requires_conversation_resolution.repository_merging_requires_conversation_resolution.repository_client",
new=repository_client,
),
):
from prowler.providers.github.services.repository.repository_merging_requires_conversation_resolution.repository_merging_requires_conversation_resolution import (
repository_merging_requires_conversation_resolution,
)

check = repository_merging_requires_conversation_resolution()
result = check.execute()
assert len(result) == 1
assert result[0].resource_id == 1
assert result[0].resource_name == "repo1"
assert result[0].status == "PASS"
assert (
result[0].status_extended
== f"Repository {repo_name} does require conversation resolution."
)
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def mock_list_repositories(_):
allow_branch_deletion=False,
enforce_status_checks=True,
enforce_admins=True,
conversation_resolution=True,
),
securitymd=True,
),
Expand Down Expand Up @@ -73,5 +74,8 @@ def test_list_repositories(self):
assert repository_service.repositories[
1
].default_branch_protection.enforce_admins
assert repository_service.repositories[
1
].default_branch_protection.conversation_resolution
# Repo
assert repository_service.repositories[1].securitymd
Loading