Conversation
Summary by CodeRabbit
WalkthroughThis update introduces centralized GitHub App authentication for backend operations, replacing direct token usage with a new authentication module. The deployment and CI/CD configurations are updated to handle GitHub App credentials and private key file management. Related management commands and their tests are refactored to use the new authentication logic. Additional tests and configuration changes support these updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
backend/apps/github/management/commands/github_get_installation_id.py (1)
10-85: Consider using centralized authentication for consistency.While the direct GitHub API usage works, this command doesn't leverage the centralized
get_github_clientfunction fromapps.github.auththat other commands use. Consider refactoring to use the centralized authentication approach for consistency across the codebase.However, the current implementation is acceptable since this command specifically needs to work with GitHub App authentication at the integration level rather than the client level.
backend/apps/github/auth.py (3)
40-46: Add type hint and consider IOError handling.The method should include a return type hint for consistency with the rest of the codebase. Also consider catching
IOErrorin addition to the current exceptions for broader error coverage.-def _load_private_key(self): +def _load_private_key(self) -> str | None: """Load the GitHub App private key from a local file.""" try: with (Path(settings.BASE_DIR) / ".github.pem").open("r") as key_file: return key_file.read().strip() - except (FileNotFoundError, PermissionError): + except (FileNotFoundError, PermissionError, IOError): return None
63-74: Use logger.info instead of logger.warning for authentication method.Using
logger.warningfor normal authentication method selection is misleading. This should belogger.infoorlogger.debugsince it's expected behavior, not a warning condition.if self._is_app_configured(): - logger.warning("Using GitHub App authentication") + logger.info("Using GitHub App authentication") return Github(
76-78: Use logger.info instead of logger.warning for PAT authentication.Same issue as above - using PAT authentication is expected fallback behavior, not a warning condition.
if self.pat_token: - logger.warning("Using GitHub PAT token") + logger.info("Using GitHub PAT token") return Github(self.pat_token, per_page=per_page)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.dockerignore(1 hunks).github/ansible/production/nest.yaml(1 hunks).github/workflows/run-ci-cd.yaml(2 hunks)backend/apps/github/auth.py(1 hunks)backend/apps/github/management/commands/github_add_related_repositories.py(2 hunks)backend/apps/github/management/commands/github_get_installation_id.py(1 hunks)backend/apps/github/management/commands/github_update_owasp_organization.py(2 hunks)backend/apps/github/management/commands/github_update_related_organizations.py(2 hunks)backend/apps/owasp/management/commands/owasp_scrape_projects.py(2 hunks)backend/settings/production.py(1 hunks)backend/tests/apps/github/auth_test.py(1 hunks)backend/tests/apps/github/management/commands/github_add_related_repositories_test.py(2 hunks)backend/tests/apps/github/management/commands/github_get_installation_id_test.py(1 hunks)backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py(2 hunks)backend/tests/apps/github/management/commands/github_update_related_organizations_test.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_scrape_projects_test.py(1 hunks)docker-compose/production.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Learnt from: Rajgupta36
PR: OWASP/Nest#1634
File: backend/apps/nest/graphql/mutations/user.py:24-48
Timestamp: 2025-06-18T21:00:33.024Z
Learning: GitHub access tokens use specific prefixes to identify token types: OAuth tokens start with `gho_`, classic personal access tokens with `ghp_`, fine-grained personal access tokens with `github_pat_`, GitHub App user tokens with `ghu_`, GitHub App installation tokens with `ghs_`, and refresh tokens with `ghr_`. This is documented in GitHub's official authentication documentation and engineering blog.
docker-compose/production.yaml (2)
Learnt from: ahmedxgouda
PR: #1675
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:13-21
Timestamp: 2025-07-03T03:47:05.436Z
Learning: The OWASP Nest project does not use async operations anywhere in the codebase and doesn't have the required database driver for async operations. All database methods and GraphQL resolvers should remain synchronous to maintain consistency with the existing architecture.
Learnt from: yashgoyal0110
PR: OWASP/Nest#0
File: :0-0
Timestamp: 2025-04-06T06:57:42.144Z
Learning: The OWASP/Nest project uses mypy for static type checking, configured in .pre-commit-config.yaml with settings to exclude tests and use backend/pyproject.toml as the config file.
backend/apps/github/auth.py (1)
Learnt from: Rajgupta36
PR: #1634
File: backend/apps/nest/graphql/mutations/user.py:24-48
Timestamp: 2025-06-18T21:00:33.024Z
Learning: GitHub access tokens use specific prefixes to identify token types: OAuth tokens start with gho_, classic personal access tokens with ghp_, fine-grained personal access tokens with github_pat_, GitHub App user tokens with ghu_, GitHub App installation tokens with ghs_, and refresh tokens with ghr_. This is documented in GitHub's official authentication documentation and engineering blog.
🧬 Code Graph Analysis (6)
backend/apps/github/management/commands/github_add_related_repositories.py (1)
backend/apps/github/auth.py (2)
get_github_client(48-80)get_github_client(83-93)
backend/apps/github/management/commands/github_update_owasp_organization.py (1)
backend/apps/github/auth.py (2)
get_github_client(48-80)get_github_client(83-93)
backend/apps/github/management/commands/github_update_related_organizations.py (1)
backend/apps/github/auth.py (2)
get_github_client(48-80)get_github_client(83-93)
backend/tests/apps/github/auth_test.py (1)
backend/apps/github/auth.py (5)
GitHubAppAuth(16-80)get_github_client(48-80)get_github_client(83-93)_is_app_configured(36-38)_load_private_key(40-46)
backend/apps/owasp/management/commands/owasp_scrape_projects.py (1)
backend/apps/github/auth.py (2)
get_github_client(48-80)get_github_client(83-93)
backend/tests/apps/github/management/commands/github_get_installation_id_test.py (1)
backend/apps/github/management/commands/github_get_installation_id.py (2)
Command(15-113)handle(36-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (37)
.dockerignore (1)
18-18: LGTM! Good security practice.Excluding
.pemfiles from Docker build context prevents sensitive private key files from being accidentally included in Docker images. This aligns well with the GitHub App authentication implementation.backend/settings/production.py (1)
50-52: LGTM! Proper secret handling for GitHub App credentials.The addition of
GITHUB_APP_IDandGITHUB_APP_INSTALLATION_IDas secret values follows the correct pattern for sensitive configuration and supports the new GitHub App authentication mechanism.backend/tests/apps/owasp/management/commands/owasp_scrape_projects_test.py (1)
40-40: LGTM! Test correctly updated for centralized GitHub authentication.The mock patch target has been appropriately updated to reflect the refactoring from direct
github.Githubinstantiation to the centralizedget_github_clientfunction, maintaining test coverage.docker-compose/production.yaml (1)
18-18: Security chain for GitHub App private key is fully configuredI’ve verified each step of the deployment pipeline:
- Ansible (.github/ansible/production/nest.yaml) copies the key from the CI runner into
~/.github.pemon the remote host withmode: '0400'.- CI/CD workflow (.github/workflows/run-ci-cd.yaml) writes the
NEST_GITHUB_APP_PRIVATE_KEYsecret to.github.pemand setschmod 600.- Backend (backend/apps/github/auth.py) correctly loads the private key from
Path(settings.BASE_DIR) / ".github.pem"and falls back safely if missing.No additional changes are needed—this read-only mount and file-permission setup follow best practices. 🚀
.github/ansible/production/nest.yaml (1)
55-60: LGTM! Secure GitHub App private key deployment.The Ansible task correctly deploys the GitHub App private key with appropriate security measures:
- Restrictive file permissions (
0400) for owner-only read access- Clear task naming and proper placement in the deployment flow
- Secure handling of sensitive credential files
backend/apps/github/management/commands/github_update_related_organizations.py (1)
8-8: LGTM: Clean refactor to centralized authenticationThe import of
get_github_clientfromapps.github.authand its usage to replace direct GitHub client instantiation aligns perfectly with the GitHub App authentication implementation. This centralized approach provides better maintainability and supports both GitHub App and PAT authentication methods.Also applies to: 43-43
backend/apps/github/management/commands/github_update_owasp_organization.py (1)
8-8: LGTM: Consistent authentication refactorThe implementation follows the same clean pattern as other management commands, replacing direct GitHub client instantiation with the centralized
get_github_client()function. The usage is correct and maintains the existing functionality while enabling GitHub App authentication.Also applies to: 48-49
backend/apps/github/management/commands/github_add_related_repositories.py (1)
8-8: LGTM: Consistent with authentication refactor patternThe changes follow the established pattern across all GitHub management commands, replacing direct client instantiation with the centralized authentication approach. Implementation is clean and correct.
Also applies to: 40-40
.github/workflows/run-ci-cd.yaml (1)
654-655: LGTM: Proper GitHub App configurationAdding the GitHub App ID and installation ID environment variables is correct for supporting the new authentication method.
backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py (1)
75-75: LGTM: Test properly updated for authentication refactorThe test changes correctly adapt to the new centralized authentication approach:
- Mocking
get_github_clientinstead ofgithub.Github- Updating parameter names and assertions accordingly
- Maintaining the same test coverage and logic
The test will properly validate the command's functionality with the new authentication system.
Also applies to: 79-79, 86-86, 151-151
backend/apps/owasp/management/commands/owasp_scrape_projects.py (2)
9-9: LGTM! Centralized authentication import.The import change to use
get_github_clientfrom the new authentication module aligns with the architectural improvement to centralize GitHub authentication logic.
39-39: LGTM! Simplified client instantiation.The change from direct GitHub client instantiation to using the centralized
get_github_client()function is correct and maintains the same functionality while benefiting from centralized authentication handling.backend/tests/apps/github/management/commands/github_update_related_organizations_test.py (2)
26-30: LGTM! Updated fixture for centralized authentication.The fixture correctly updates the mocking strategy to patch
get_github_clientinstead of thegithub.Githubclass, aligning with the new centralized authentication approach.
97-97: LGTM! Consistent test mocking update.The monkeypatch update correctly reflects the command's refactoring to use
get_github_client, ensuring test coverage remains intact.backend/tests/apps/github/management/commands/github_add_related_repositories_test.py (3)
34-34: LGTM! Updated mock patching for centralized authentication.The mock patch correctly targets
get_github_clientinstead of thegithub.Githubclass, aligning with the command's refactoring to use centralized authentication.
40-47: LGTM! Consistent test setup updates.The test setup correctly updates the mock variable names and return value handling to match the new
get_github_clientfunction interface.
77-77: LGTM! Updated test assertion.The assertion correctly verifies that
get_github_clientis called once without arguments, matching the expected behavior of the centralized authentication function.backend/apps/github/management/commands/github_get_installation_id.py (2)
44-78: LGTM! Comprehensive error handling.The error handling is thorough and user-friendly:
- Validates app ID presence
- Check file existence and readability
- Handles empty private key files
- Provides clear error messages with guidance
The use of
sys.exit(1)is appropriate for management commands to indicate failure.
80-113: LGTM! Well-structured GitHub App integration.The GitHub App authentication and installation retrieval logic is correct:
- Proper use of
Auth.AppAuthandGithubIntegration- Good output formatting with installation details
- Appropriate exception handling with logging
The code follows GitHub API best practices.
backend/tests/apps/github/management/commands/github_get_installation_id_test.py (3)
10-55: LGTM! Comprehensive test for successful installation retrieval.The test correctly mocks all GitHub API components and verifies:
- Proper authentication setup with app ID and private key
- GitHub integration instantiation and method calls
- File reading operations
The test structure is well-organized and covers the happy path thoroughly.
79-96: LGTM! Good error condition testing.The tests for missing app ID and private key file not found correctly use
pytest.raises(SystemExit)to verify expected failure behavior. This ensures the command handles error conditions appropriately.
98-151: LGTM! Thorough edge case coverage.The tests cover important edge cases:
- Empty private key file handling
- Custom private key file path support
- Environment variable app ID sourcing
The mocking strategy is consistent and the assertions verify correct parameter passing to the GitHub API components.
backend/tests/apps/github/auth_test.py (10)
16-28: LGTM! Well-structured initialization test.The test properly mocks the dependencies and verifies all the expected attributes are set correctly during GitHub App configuration initialization.
30-40: LGTM! Good PAT fallback test coverage.The test correctly verifies the fallback behavior when GitHub App configuration is not available but a PAT token is provided via environment variable.
42-52: LGTM! Proper error handling test.The test correctly verifies that a ValueError is raised when neither GitHub App configuration nor PAT token is available, which matches the expected behavior from the implementation.
110-147: LGTM! Comprehensive GitHub App authentication test.The test properly mocks all the PyGithub authentication components and verifies the correct authentication flow is used when GitHub App credentials are configured. The assertions check that the correct parameters are passed to the authentication constructors.
149-163: LGTM! Good PAT fallback test.The test correctly verifies that when GitHub App configuration is not available, the system falls back to PAT authentication and creates the client with the expected parameters.
165-179: LGTM! Custom pagination test.The test properly verifies that custom per_page parameters are passed through correctly to the GitHub client constructor.
196-211: LGTM! Simple and effective convenience function test.The test properly verifies that the module-level
get_github_clientfunction returns the expected GitHub client instance when PAT authentication is available.
68-79: Fix the test logic - redundant initialization.Similar issue as the previous test - the test creates a GitHubAppAuth instance but then calls the private method directly instead of testing the integrated behavior.
def test_load_private_key_file_not_found(self): """Test private key loading when file doesn't exist.""" with ( mock.patch.object(Path, "exists", return_value=False), mock.patch("apps.github.auth.settings") as mock_settings, + mock.patch.dict(os.environ, {"GITHUB_TOKEN": "fallback-token"}), ): mock_settings.BASE_DIR = "/test/path" - mock_settings.GITHUB_APP_ID = None - mock_settings.GITHUB_APP_INSTALLATION_ID = None - auth = GitHubAppAuth() - private_key = auth._load_private_key() + mock_settings.GITHUB_APP_ID = "12345" + mock_settings.GITHUB_APP_INSTALLATION_ID = "67890" + auth = GitHubAppAuth() assert private_key is NoneLikely an incorrect or invalid review comment.
181-193: Fix the test logic - authentication error scenario.The test mocks
Githubto raiseBadCredentialsException, but this exception would be raised during client instantiation, not during the authentication process itself. The actual implementation doesn't catch this exception inget_github_client.def test_get_github_client_authentication_error(self): - """Test GitHub client creation with authentication error.""" + """Test GitHub client creation with no valid credentials.""" with ( mock.patch("apps.github.auth.settings") as mock_settings, - mock.patch.dict(os.environ, {"GITHUB_TOKEN": "invalid-token"}), - mock.patch("apps.github.auth.Github") as mock_github, + mock.patch.dict(os.environ, {}, clear=True), + mock.patch.object(GitHubAppAuth, "_load_private_key", return_value=None), ): mock_settings.GITHUB_APP_ID = None mock_settings.GITHUB_APP_INSTALLATION_ID = None auth = GitHubAppAuth() - mock_github.side_effect = BadCredentialsException(401, "Invalid token", None) with pytest.raises(BadCredentialsException): auth.get_github_client()Likely an incorrect or invalid review comment.
54-66: Fix the test logic - redundant initialization.The test calls
GitHubAppAuth()on line 64 but then callsauth._load_private_key()on line 65, which tests the method in isolation rather than the integrated behavior during initialization.def test_load_private_key_success(self): """Test successful private key loading from file.""" with ( mock.patch.object(Path, "exists", return_value=True), mock.patch.object(Path, "open", mock.mock_open(read_data="test-private-key")), mock.patch("apps.github.auth.settings") as mock_settings, + mock.patch.dict(os.environ, {"GITHUB_TOKEN": "fallback-token"}), ): mock_settings.BASE_DIR = "/test/path" - mock_settings.GITHUB_APP_ID = None - mock_settings.GITHUB_APP_INSTALLATION_ID = None - auth = GitHubAppAuth() - private_key = auth._load_private_key() + mock_settings.GITHUB_APP_ID = "12345" + mock_settings.GITHUB_APP_INSTALLATION_ID = "67890" + auth = GitHubAppAuth() assert private_key == "test-private-key"Likely an incorrect or invalid review comment.
backend/apps/github/auth.py (5)
1-14: LGTM! Clean module structure and imports.The module is well-organized with appropriate imports and logging setup. The docstring clearly describes the module's purpose.
19-34: LGTM! Comprehensive initialization with good error messaging.The initialization properly loads configuration from Django settings and environment variables, with clear error messaging when neither authentication method is configured. The error message provides helpful guidance for resolving configuration issues.
36-38: LGTM! Clear configuration validation.The method correctly checks that all required GitHub App configuration components are present and non-empty.
83-93: LGTM! Clean convenience function.The module-level function provides a clean interface for consumers who don't need to manage the GitHubAppAuth instance directly. The docstring is comprehensive and the implementation is straightforward.
80-80: Add GitHub token prefix validation before authenticationOur search didn’t find any existing checks for known GitHub token prefixes (
ghp_,github_pat_,gho_,ghu_,ghs_,ghr_). To surface malformed tokens earlier and provide clearer errors, add a prefix check inbackend/apps/github/auth.py(around line 80) before invoking GitHub’s API:--- a/backend/apps/github/auth.py +++ b/backend/apps/github/auth.py @@ -78,6 +78,14 @@ def authenticate_with_github(token: str) -> User: # existing setup code... + # Validate token format to catch invalid prefixes early + prefixes = ("ghp_", "github_pat_", "gho_", "ghu_", "ghs_", "ghr_") + if not any(token.startswith(p) for p in prefixes): + raise BadCredentialsException( + 400, + f"Invalid GitHub token prefix. Expected one of: {', '.join(prefixes)}", + None, + ) + # call out to GitHub API… if response.status_code == 401: raise BadCredentialsException(401, "Invalid GitHub credentials", None)• Location:
backend/apps/github/auth.pyaround line 80
• Purpose: Ensure PATs use a valid prefix and improve error messaging
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/ansible/staging/nest.yaml (1)
37-46: Addno_log+ keep module naming consistent
- Secrets copy will expose filenames and a “changed” diff in the task output. Add
no_log: trueto suppress any accidental leakage in verbose logs.- Elsewhere in this play you fully-qualify modules (
ansible.builtin.copy). For consistency (and future‐proofing against collections shadowing), do the same here.- - name: Copy secrets - copy: + - name: Copy secrets + ansible.builtin.copy: src: '{{ github_workspace }}/{{ item }}' dest: ~/ mode: '0400' + no_log: true
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/ansible/production/nest.yaml(1 hunks).github/ansible/staging/nest.yaml(1 hunks)backend/tests/apps/github/management/commands/github_get_installation_id_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/ansible/production/nest.yaml
- backend/tests/apps/github/management/commands/github_get_installation_id_test.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/module.py:28-29
Timestamp: 2025-07-16T13:49:58.648Z
Learning: In the OWASP Nest mentorship system, mentors can be created with only github_user initially (without nest_user) when assigned to modules. This allows mentors to be assigned before they've signed into the system. When these users eventually sign in, the nest_user is automatically linked to the existing github_user, creating a complete mentor profile. This design provides flexibility in mentor assignment workflows.
Learnt from: Rajgupta36
PR: OWASP/Nest#1634
File: backend/apps/nest/graphql/mutations/user.py:24-48
Timestamp: 2025-06-18T21:00:33.024Z
Learning: GitHub access tokens use specific prefixes to identify token types: OAuth tokens start with `gho_`, classic personal access tokens with `ghp_`, fine-grained personal access tokens with `github_pat_`, GitHub App user tokens with `ghu_`, GitHub App installation tokens with `ghs_`, and refresh tokens with `ghr_`. This is documented in GitHub's official authentication documentation and engineering blog.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
|
2 participants
Proposed change
Add GitHub application authentication in addition to PAT.
Checklist
make check-testlocally; all checks and tests passed.