Skip to content

Conversation

@eliykat
Copy link
Member

@eliykat eliykat commented Dec 4, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28485

📔 Objective

Move code relating to organization events into DIRT code ownership.

Intentionally excluded:

  • Secrets Manager event code - owned by SM Team (this is a bit incongruous but I'll leave it to DIRT as to whether they want to follow this up)
  • the events integration infrastructure - this will be moved by the architecture team who developed this.

Namespaces have generally not been updated in order to minimize impact through the codebase.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@eliykat eliykat requested review from a team as code owners December 4, 2025 00:49
@eliykat eliykat requested a review from r-tome December 4, 2025 00:49
@claude
Copy link
Contributor

claude bot commented Dec 4, 2025

Claude finished @eliykat's task —— View job


Code Review Complete ✓

This PR successfully moves organization events domain code to DIRT team ownership. The refactoring is clean and well-structured, with most changes being file moves/renames to the new Dirt folder hierarchy.

👍 Strengths

  1. Clean organizational structure: All event-related code is now properly grouped under **/Dirt/ directories, aligning with the CODEOWNERS file (line 55: **/Dirt @bitwarden/team-data-insights-and-reporting-dev)

  2. Consistent file moves: The PR systematically moves files across all layers:

    • API controllers and models
    • Core entities, enums, and services
    • Infrastructure (Dapper and EF) repositories
    • SQL stored procedures, tables, and views
    • Test files
  3. Minimal code changes: Only necessary namespace updates were made in files that reference the moved code, reducing risk of introducing bugs

  4. Test coverage maintained: Codecov confirms all modified lines are covered by tests, and test files were moved alongside the code

📝 Observations

  1. Namespace preservation: As noted in the PR description, namespaces were intentionally not updated to minimize impact. This is a pragmatic choice but means there's a mismatch between physical file location and namespace:

    • Files are in src/Core/Dirt/ but use namespace Bit.Core.AdminConsole.*
    • Files are in src/Api/Dirt/ but may use namespace Bit.Api.AdminConsole.*
  2. Updated namespace imports: Files that import the moved types correctly updated their using statements:

    • EventDiagnosticLogger.cs updated: Bit.Api.Dirt.Public.Models and Dirt.Models.Response.EventResponseModel
    • SecretsManagerEventsController.cs added: using Bit.Api.Dirt.Models.Response;
  3. Checkmarx CSRF finding: The flagged CSRF issue in AccountsController.cs:427 is not introduced by this PR. The file has no changes in this PR, so this is a pre-existing issue that should be tracked separately.

ℹ️ Context Considerations

From the PR comments, @withinfocus mentioned concern about larger caching changes coming. Since this is primarily a file organization refactor with no functional changes, merge conflicts should be straightforward to resolve - the files will just be in new locations.

✅ Recommendation

This PR is well-executed for its stated goal: moving event logging code to DIRT team ownership. The approach of keeping namespaces unchanged is reasonable for minimizing blast radius, though it does create technical debt for a future cleanup.

Approve - This refactoring successfully achieves its objective with minimal risk. The code organization now properly reflects team ownership boundaries.

🌱 Future Considerations

  1. Consider a follow-up PR to align namespaces with the new folder structure (e.g., Bit.Core.Dirt.* instead of Bit.Core.AdminConsole.*)
  2. Update any documentation that references the old file paths
  3. Track the pre-existing CSRF issue separately if not already tracked

@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.71%. Comparing base (3de2f98) to head (e83ed86).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6685      +/-   ##
==========================================
+ Coverage   53.86%   57.71%   +3.84%     
==========================================
  Files        1917     1917              
  Lines       85126    85126              
  Branches     7620     7620              
==========================================
+ Hits        45853    49129    +3276     
+ Misses      37508    34159    -3349     
- Partials     1765     1838      +73     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@eliykat eliykat requested a review from ttalty December 4, 2025 00:59
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2025

Logo
Checkmarx One – Scan Summary & Details6f72bfc8-a5e4-4a2e-b83c-75f6b553c6e1

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1519
detailsMethod at line 1519 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
ID: dMGF5qNfAN72zlvQcA1MgbhHv%2Fc%3D
Attack Vector
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1395
detailsMethod at line 1395 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
ID: iOCFr11iI9znjDnv46yLfiS4aDY%3D
Attack Vector
Fixed Issues (2)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/AdminConsole/Public/Controllers/MembersController.cs: 207
MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 300

@withinfocus
Copy link
Contributor

We have some larger changes still coming in with our caching work so looking at this right now gives me some pause.

r-tome
r-tome previously approved these changes Dec 4, 2025
Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will keep iterating as needed on the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants