From be7ad1dca8febdbe0552095c50bec5629b793d47 Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Thu, 23 Oct 2025 07:59:12 +0200 Subject: [PATCH 1/4] Implement reusable Claude code review workflow --- .claude/CLAUDE.md | 9 +++++++++ .claude/prompts/review-code.md | 27 +++++++++++++++++++++++++++ .github/workflows/respond.yml | 28 ++++++++++++++++++++++++++++ .github/workflows/review-code.yml | 20 ++++++++++++++++++++ 4 files changed, 84 insertions(+) create mode 100644 .claude/CLAUDE.md create mode 100644 .claude/prompts/review-code.md create mode 100644 .github/workflows/respond.yml create mode 100644 .github/workflows/review-code.yml diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md new file mode 100644 index 000000000..c44d18fa3 --- /dev/null +++ b/.claude/CLAUDE.md @@ -0,0 +1,9 @@ +# Bitwarden Directory Connector + +## References + +- [Architectural Decision Records (ADRs)](https://contributing.bitwarden.com/architecture/adr/) +- [Contributing Guidelines](https://contributing.bitwarden.com/contributing/) +- [Code Style](https://contributing.bitwarden.com/contributing/code-style/) +- [Security Whitepaper](https://bitwarden.com/help/bitwarden-security-white-paper/) +- [Security Definitions](https://contributing.bitwarden.com/architecture/security/definitions) diff --git a/.claude/prompts/review-code.md b/.claude/prompts/review-code.md new file mode 100644 index 000000000..93b97cfad --- /dev/null +++ b/.claude/prompts/review-code.md @@ -0,0 +1,27 @@ +Please review this pull request with a focus on: + +- Code quality and best practices +- Potential bugs or issues +- Security implications +- Performance considerations + +Note: The PR branch is already checked out in the current working directory. + +Provide a comprehensive review including: + +- Summary of changes since last review +- Critical issues found (be thorough) +- Suggested improvements (be thorough) +- Good practices observed (be concise - list only the most notable items without elaboration) +- Action items for the author +- Leverage collapsible
sections where appropriate for lengthy explanations or code + snippets to enhance human readability + +When reviewing subsequent commits: + +- Track status of previously identified issues (fixed/unfixed/reopened) +- Identify NEW problems introduced since last review +- Note if fixes introduced new issues + +IMPORTANT: Be comprehensive about issues and improvements. For good practices, be brief - just note +what was done well without explaining why or praising excessively. diff --git a/.github/workflows/respond.yml b/.github/workflows/respond.yml new file mode 100644 index 000000000..d940ceee7 --- /dev/null +++ b/.github/workflows/respond.yml @@ -0,0 +1,28 @@ +name: Respond + +on: + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + issues: + types: [opened, assigned] + pull_request_review: + types: [submitted] + +permissions: {} + +jobs: + respond: + name: Respond + uses: bitwarden/gh-actions/.github/workflows/_respond.yml@main + secrets: + AZURE_SUBSCRIPTION_ID: ${{ secrets.AZURE_SUBSCRIPTION_ID }} + AZURE_TENANT_ID: ${{ secrets.AZURE_TENANT_ID }} + AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }} + permissions: + actions: read + contents: write + id-token: write + issues: write + pull-requests: write diff --git a/.github/workflows/review-code.yml b/.github/workflows/review-code.yml new file mode 100644 index 000000000..46309af38 --- /dev/null +++ b/.github/workflows/review-code.yml @@ -0,0 +1,20 @@ +name: Code Review + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + +permissions: {} + +jobs: + review: + name: Review + uses: bitwarden/gh-actions/.github/workflows/_review-code.yml@main + secrets: + AZURE_SUBSCRIPTION_ID: ${{ secrets.AZURE_SUBSCRIPTION_ID }} + AZURE_TENANT_ID: ${{ secrets.AZURE_TENANT_ID }} + AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }} + permissions: + contents: read + id-token: write + pull-requests: write From 16aef70b29eb9d8804d09cd92accc865c118b3b0 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 29 Oct 2025 13:53:52 +1000 Subject: [PATCH 2/4] Suggested claude.md changes --- .claude/CLAUDE.md | 248 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 248 insertions(+) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index c44d18fa3..afb38fe35 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -1,5 +1,253 @@ # Bitwarden Directory Connector +## Project Overview + +Directory Connector is a TypeScript application that synchronizes users and groups from directory services to Bitwarden organizations. It provides both a desktop GUI (built with Angular and Electron) and a CLI tool (bwdc). + +**Supported Directory Services:** + +- LDAP (Lightweight Directory Access Protocol) - includes Active Directory and general LDAP servers +- Microsoft Entra ID (formerly Azure Active Directory) +- Google Workspace +- Okta +- OneLogin + +**Technologies:** + +- TypeScript +- Angular (GUI) +- Electron (Desktop wrapper) +- Node +- Jest for testing + +## Code Architecture & Structure + +### Directory Organization + +``` +src/ +├── abstractions/ # Interface definitions (e.g., IDirectoryService) +├── services/ # Business logic implementations for directory services, sync, auth +├── models/ # Data models (UserEntry, GroupEntry, etc.) +├── commands/ # CLI command implementations +├── app/ # Angular GUI components +└── utils/ # Test utilities and fixtures + +src-cli/ # CLI-specific code (imports common code from src/) + +jslib/ # Legacy folder structure (mix of deprecated/unused and current code - new code should not be added here) +``` + +### Key Architectural Patterns + +1. **Abstractions = Interfaces**: All interfaces are defined in `/abstractions` +2. **Services = Business Logic**: Implementations live in `/services` +3. **Directory Service Pattern**: Each directory provider implements `IDirectoryService` interface +4. **Separation of Concerns**: GUI (Angular app) and CLI (commands) share the same service layer + +## Testing Conventions + +- **Framework**: Jest with jest-preset-angular +- **Mocking**: jest-mock-extended for type-safe mocks with `mock()` +- **Location**: Tests colocated with source files +- **Naming**: Descriptive, human-readable test names (e.g., `'should return empty array when no users exist in directory'`) +- **Test Helpers**: Located in `utils/` directory + +## Directory Integration Patterns + +### IDirectoryService Interface + +All directory services implement this core interface with methods: + +- `getUsers()` - Retrieve users from directory +- `getGroups()` - Retrieve groups from directory +- Connection and authentication handling + +### Service-Specific Implementations + +Each directory service has unique authentication and query patterns: + +- **LDAP**: Direct LDAP queries, bind authentication +- **Microsoft Entra ID**: Microsoft Graph API, OAuth tokens +- **Google Workspace**: Google Admin SDK, service account credentials +- **Okta/OneLogin**: REST APIs with API tokens + +## Code Review Guidelines + +### Security Considerations + +**Critical Security Areas:** + +1. **Credential Handling** + - Never log directory service credentials, API keys, or tokens + - Use secure storage mechanisms for sensitive data + - Credentials should never be hardcoded + +2. **Sensitive Data** + - User and group data from directories should be handled securely + - Avoid exposing sensitive information in error messages + - Sanitize data before logging + - Be cautious with data persistence + +3. **Input Validation** + - Validate and sanitize data from external directory services + - Check for injection vulnerabilities (LDAP injection, etc.) + - Validate configuration inputs from users + +4. **API Security** + - Ensure OAuth flows are implemented correctly + - Verify SSL/TLS is used for all external connections + - Check for secure token storage and refresh mechanisms + +### TypeScript Conventions + +**Import patterns** + +- Use path aliases (`@/`) for project imports + - `@/` - project root + - `@/jslib/` - jslib folder +- ESLint enforces alphabetized import ordering with newlines between groups + +**Type Safety:** + +- Avoid `any` types - use proper typing or `unknown` with type guards +- Prefer interfaces for contracts, types for unions/intersections +- Use strict null checks - handle `null` and `undefined` explicitly +- Leverage TypeScript's type inference where appropriate + +**Async Patterns:** + +```typescript +// Good - proper async/await with error handling +async function syncUsers(): Promise { + try { + const users = await directoryService.getUsers(); + return users.filter((u) => u.enabled); + } catch (error) { + logger.error("Failed to sync users", error); + throw new SyncError("User sync failed", error); + } +} + +// Avoid - unhandled promises +function syncUsers() { + directoryService.getUsers().then((users) => processUsers(users)); +} +``` + +### Error Handling Patterns + +**Required Practices:** + +1. **Try-catch for async operations** - Always wrap external API calls +2. **Meaningful error messages** - Provide context for debugging +3. **Error propagation** - Don't swallow errors silently +4. **User-facing errors** - Separate user messages from developer logs + +```typescript +// Good +try { + await directoryService.connect(); +} catch (error) { + logger.error("Directory connection failed", { service: "ldap", error }); + throw new ConnectionError("Unable to connect to directory service", error); +} + +// Avoid - swallowing errors +try { + await directoryService.connect(); +} catch (error) { + console.log("error"); // Too vague, no context +} +``` + +### Performance Considerations + +**Large Dataset Handling:** + +- Use pagination for large user/group lists +- Avoid loading entire datasets into memory at once +- Consider streaming or batch processing for large operations + +**API Rate Limiting:** + +- Respect rate limits for Microsoft Graph API, Google Admin SDK, etc. +- Consider batching large API calls where necessary + +**Memory Management:** + +- Close connections and clean up resources +- Remove event listeners when components are destroyed +- Be cautious with caching large datasets + +### Testing Requirements + +**Test File Types:** + +- `*.spec.ts` - Unit tests for individual components/services +- `*.integration.spec.ts` - Integration tests against live directory services + +**When Tests Are Required:** + +- New features must include tests +- Bug fixes should include regression tests +- Changes to core sync logic or directory specific logic require integration tests + +**Test Expectations:** + +- **Unit tests**: Mock external API calls using jest-mock-extended +- **Integration tests**: Use live directory services (Docker containers or configured cloud services) +- **Coverage focus**: Critical paths (authentication, sync, data transformation) +- **Test scenarios**: Error cases and edge cases (empty results, malformed data, connection failures), not just happy paths + +### Common Anti-Patterns to Flag + +1. **Hardcoded Values** + - No hardcoded credentials, URLs, or configuration + - Use configuration files or environment variables + +2. **Synchronous Operations** + - Don't use sync file operations in production code + - Avoid blocking operations in main thread + +3. **Missing Error Handling** + - Every external call should have error handling + - API calls, file operations, and network requests must be wrapped + +4. **Memory Leaks** + - Unclosed connections to directory services + - Event listeners not removed + - Large objects not released + +5. **Insecure Patterns** + - Logging sensitive data + - Storing credentials in plain text + - Missing input validation + +6. **Poor Performance Patterns** + - Loading entire directory into memory + - N+1 query patterns + - Inefficient loops with repeated operations + +### Code Organization Standards + +**File Naming:** + +- kebab-case for files: `ldap-directory.service.ts` +- Descriptive names that reflect purpose + +**Class/Function Naming:** + +- PascalCase for classes and interfaces +- camelCase for functions and variables +- Descriptive names that indicate purpose + +**When to Create New Files:** + +- Keep files focused on single responsibility +- Create new service files for distinct directory integrations +- Separate models into individual files when complex + ## References - [Architectural Decision Records (ADRs)](https://contributing.bitwarden.com/architecture/adr/) From 208cb3a3ff1e7c3aa97fd31a58275c2576aad76f Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Wed, 29 Oct 2025 14:04:42 +1000 Subject: [PATCH 3/4] Focus more on project structure than code review guidelines --- .claude/CLAUDE.md | 212 +++++++++++++++++----------------------------- 1 file changed, 79 insertions(+), 133 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index afb38fe35..b4df0cbe5 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -45,63 +45,30 @@ jslib/ # Legacy folder structure (mix of deprecated/unused and c 3. **Directory Service Pattern**: Each directory provider implements `IDirectoryService` interface 4. **Separation of Concerns**: GUI (Angular app) and CLI (commands) share the same service layer -## Testing Conventions +## Development Conventions -- **Framework**: Jest with jest-preset-angular -- **Mocking**: jest-mock-extended for type-safe mocks with `mock()` -- **Location**: Tests colocated with source files -- **Naming**: Descriptive, human-readable test names (e.g., `'should return empty array when no users exist in directory'`) -- **Test Helpers**: Located in `utils/` directory +### Code Organization -## Directory Integration Patterns - -### IDirectoryService Interface - -All directory services implement this core interface with methods: - -- `getUsers()` - Retrieve users from directory -- `getGroups()` - Retrieve groups from directory -- Connection and authentication handling - -### Service-Specific Implementations - -Each directory service has unique authentication and query patterns: - -- **LDAP**: Direct LDAP queries, bind authentication -- **Microsoft Entra ID**: Microsoft Graph API, OAuth tokens -- **Google Workspace**: Google Admin SDK, service account credentials -- **Okta/OneLogin**: REST APIs with API tokens - -## Code Review Guidelines - -### Security Considerations +**File Naming:** -**Critical Security Areas:** +- kebab-case for files: `ldap-directory.service.ts` +- Descriptive names that reflect purpose -1. **Credential Handling** - - Never log directory service credentials, API keys, or tokens - - Use secure storage mechanisms for sensitive data - - Credentials should never be hardcoded +**Class/Function Naming:** -2. **Sensitive Data** - - User and group data from directories should be handled securely - - Avoid exposing sensitive information in error messages - - Sanitize data before logging - - Be cautious with data persistence +- PascalCase for classes and interfaces +- camelCase for functions and variables +- Descriptive names that indicate purpose -3. **Input Validation** - - Validate and sanitize data from external directory services - - Check for injection vulnerabilities (LDAP injection, etc.) - - Validate configuration inputs from users +**File Structure:** -4. **API Security** - - Ensure OAuth flows are implemented correctly - - Verify SSL/TLS is used for all external connections - - Check for secure token storage and refresh mechanisms +- Keep files focused on single responsibility +- Create new service files for distinct directory integrations +- Separate models into individual files when complex ### TypeScript Conventions -**Import patterns** +**Import Patterns:** - Use path aliases (`@/`) for project imports - `@/` - project root @@ -115,53 +82,49 @@ Each directory service has unique authentication and query patterns: - Use strict null checks - handle `null` and `undefined` explicitly - Leverage TypeScript's type inference where appropriate -**Async Patterns:** - -```typescript -// Good - proper async/await with error handling -async function syncUsers(): Promise { - try { - const users = await directoryService.getUsers(); - return users.filter((u) => u.enabled); - } catch (error) { - logger.error("Failed to sync users", error); - throw new SyncError("User sync failed", error); - } -} - -// Avoid - unhandled promises -function syncUsers() { - directoryService.getUsers().then((users) => processUsers(users)); -} -``` +**Configuration:** + +- Use configuration files or environment variables +- Never hardcode URLs or configuration values + +## Security Best Practices + +**Credential Handling:** + +- Never log directory service credentials, API keys, or tokens +- Use secure storage mechanisms for sensitive data +- Credentials should never be hardcoded +- Store credentials encrypted, never in plain text + +**Sensitive Data:** + +- User and group data from directories should be handled securely +- Avoid exposing sensitive information in error messages +- Sanitize data before logging +- Be cautious with data persistence + +**Input Validation:** -### Error Handling Patterns +- Validate and sanitize data from external directory services +- Check for injection vulnerabilities (LDAP injection, etc.) +- Validate configuration inputs from users -**Required Practices:** +**API Security:** + +- Ensure authentication flows are implemented correctly +- Verify SSL/TLS is used for all external connections +- Check for secure token storage and refresh mechanisms + +## Error Handling + +**Best Practices:** 1. **Try-catch for async operations** - Always wrap external API calls 2. **Meaningful error messages** - Provide context for debugging 3. **Error propagation** - Don't swallow errors silently 4. **User-facing errors** - Separate user messages from developer logs -```typescript -// Good -try { - await directoryService.connect(); -} catch (error) { - logger.error("Directory connection failed", { service: "ldap", error }); - throw new ConnectionError("Unable to connect to directory service", error); -} - -// Avoid - swallowing errors -try { - await directoryService.connect(); -} catch (error) { - console.log("error"); // Too vague, no context -} -``` - -### Performance Considerations +## Performance Best Practices **Large Dataset Handling:** @@ -180,73 +143,56 @@ try { - Remove event listeners when components are destroyed - Be cautious with caching large datasets -### Testing Requirements +## Testing -**Test File Types:** +**Framework:** +- Jest with jest-preset-angular +- jest-mock-extended for type-safe mocks with `mock()` + +**Test Organization:** + +- Tests colocated with source files - `*.spec.ts` - Unit tests for individual components/services - `*.integration.spec.ts` - Integration tests against live directory services +- Test helpers located in `utils/` directory + +**Test Naming:** + +- Descriptive, human-readable test names +- Example: `'should return empty array when no users exist in directory'` -**When Tests Are Required:** +**Test Coverage:** - New features must include tests - Bug fixes should include regression tests - Changes to core sync logic or directory specific logic require integration tests -**Test Expectations:** +**Testing Approach:** - **Unit tests**: Mock external API calls using jest-mock-extended - **Integration tests**: Use live directory services (Docker containers or configured cloud services) -- **Coverage focus**: Critical paths (authentication, sync, data transformation) -- **Test scenarios**: Error cases and edge cases (empty results, malformed data, connection failures), not just happy paths +- Focus on critical paths (authentication, sync, data transformation) +- Test error scenarios and edge cases (empty results, malformed data, connection failures), not just happy paths -### Common Anti-Patterns to Flag +## Directory Service Patterns -1. **Hardcoded Values** - - No hardcoded credentials, URLs, or configuration - - Use configuration files or environment variables - -2. **Synchronous Operations** - - Don't use sync file operations in production code - - Avoid blocking operations in main thread - -3. **Missing Error Handling** - - Every external call should have error handling - - API calls, file operations, and network requests must be wrapped - -4. **Memory Leaks** - - Unclosed connections to directory services - - Event listeners not removed - - Large objects not released - -5. **Insecure Patterns** - - Logging sensitive data - - Storing credentials in plain text - - Missing input validation - -6. **Poor Performance Patterns** - - Loading entire directory into memory - - N+1 query patterns - - Inefficient loops with repeated operations - -### Code Organization Standards - -**File Naming:** +### IDirectoryService Interface -- kebab-case for files: `ldap-directory.service.ts` -- Descriptive names that reflect purpose +All directory services implement this core interface with methods: -**Class/Function Naming:** +- `getUsers()` - Retrieve users from directory and transform them into standard objects +- `getGroups()` - Retrieve groups from directory and transform them into standard objects +- Connection and authentication handling -- PascalCase for classes and interfaces -- camelCase for functions and variables -- Descriptive names that indicate purpose +### Service-Specific Implementations -**When to Create New Files:** +Each directory service has unique authentication and query patterns: -- Keep files focused on single responsibility -- Create new service files for distinct directory integrations -- Separate models into individual files when complex +- **LDAP**: Direct LDAP queries, bind authentication +- **Microsoft Entra ID**: Microsoft Graph API, OAuth tokens +- **Google Workspace**: Google Admin SDK, service account credentials +- **Okta/OneLogin**: REST APIs with API tokens ## References From bd9aae2b149cc6cb4963f8af86a3381e758c3323 Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Wed, 29 Oct 2025 14:41:44 +0100 Subject: [PATCH 4/4] Set claude related file code ownership --- .github/CODEOWNERS | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 29a2c3f53..beadc1cd5 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -12,3 +12,8 @@ **/*.dockerignore @bitwarden/team-appsec @bitwarden/dept-bre **/entrypoint.sh @bitwarden/team-appsec @bitwarden/dept-bre **/docker-compose.yml @bitwarden/team-appsec @bitwarden/dept-bre + +# Claude related files +.claude/ @bitwarden/team-ai-sme +.github/workflows/respond.yml @bitwarden/team-ai-sme +.github/workflows/review-code.yml @bitwarden/team-ai-sme