Add template properties for Datadog

[brant-livefront]

📔 Objective

We recently got a few more specifics from the Datadog team on the existing pull-based integration. In order to support identical data being sent as part of our new push-based integration, we need a few more dynamic fields to enrich the data.

This PR adds these new dynamic fields.

⏰ 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

[claude]

Claude finished @brant-livefront's task —— View job

---

Code Review Complete

Summary of Changes

This PR adds additional template properties for the Datadog integration to support push-based integration with richer event data. The changes include:

• New template properties: UserType, ActingUserType, and GroupName
• Architecture change: Switched from using User entities to OrganizationUserUserDetails to provide organization-specific user context
• New repository dependencies: Added IGroupRepository and IOrganizationUserRepository (replacing IUserRepository)
• Enhanced template processing: Added TemplateRequiresGroup() method
• Comprehensive test coverage: All new properties have corresponding unit tests

---

Critical Issues

1. ⚠️ Potential Null Reference in Template Processing

Location: src/Core/AdminConsole/Utilities/IntegrationTemplateProcessor.cs:29

return property?.GetValue(values)?.ToString() ?? "";

The conditional access operator prevents the immediate null reference, but there's a subtle issue with how the new template checks work:

Issue: In EventIntegrationHandler.cs:104-110 and 113-120, the code fetches OrganizationUserUserDetails but doesn't check if UserType or ActingUserType are actually referenced in the template before making expensive database calls. The TemplateRequiresUser and TemplateRequiresActingUser methods only check for #UserName# and #UserEmail#, not #UserType# or #ActingUserType#.

Impact: If a template contains only #UserType# without #UserName# or #UserEmail#, the user won't be loaded from the database, and the token won't be replaced.

Recommendation: Update the template requirement checks:

Suggested fix

public static bool TemplateRequiresUser(string template)
{
    if (string.IsNullOrEmpty(template))
    {
        return false;
    }

    return template.Contains("#UserName#", StringComparison.Ordinal)
           || template.Contains("#UserEmail#", StringComparison.Ordinal)
           || template.Contains("#UserType#", StringComparison.Ordinal);
}

public static bool TemplateRequiresActingUser(string template)
{
    if (string.IsNullOrEmpty(template))
    {
        return false;
    }

    return template.Contains("#ActingUserName#", StringComparison.Ordinal)
           || template.Contains("#ActingUserEmail#", StringComparison.Ordinal)
           || template.Contains("#ActingUserType#", StringComparison.Ordinal);
}

---

2. ⚠️ Missing Null Check for User Entity in BuildContextAsync

Location: src/Core/AdminConsole/Services/Implementations/EventIntegrations/EventIntegrationHandler.cs:106-110

var orgUser = await organizationUserRepository.GetByOrganizationAsync(organizationId: organizationId, userId: eventMessage.UserId.Value);
if (orgUser is not null)
{
    context.User = await organizationUserRepository.GetDetailsByIdAsync(orgUser.Id);
}

Issue: While there's a null check for orgUser, there's no null check for the result of GetDetailsByIdAsync(). If this returns null, context.User will be null, which is handled gracefully by the null-conditional properties. However, this could mask data issues.

Recommendation: Consider logging a warning if GetDetailsByIdAsync() returns null when orgUser is not null, as this indicates a data consistency issue.

---

Suggested Improvements

3. 🎨 Inconsistent Null Handling Pattern

Location: src/Core/AdminConsole/Services/Implementations/EventIntegrations/EventIntegrationHandler.cs:94-96

The Group fetching doesn't check if OrganizationId is present before attempting to fetch:

if (IntegrationTemplateProcessor.TemplateRequiresGroup(template) && eventMessage.GroupId.HasValue)
{
    context.Group = await groupRepository.GetByIdAsync(eventMessage.GroupId.Value);
}

Issue: Groups belong to organizations, but this code doesn't verify that eventMessage.OrganizationId is set before fetching the group. While this may work in practice (since groups without organizations wouldn't exist), it's inconsistent with the pattern used for User/ActingUser fetching.

Recommendation: Move the Group fetching inside the if (eventMessage.OrganizationId is not Guid organizationId) block for consistency, or verify that groups can be safely fetched without organization context.

---

4. 🎨 Test Coverage Gap: Missing Template Check Tests

Location: test/Core.Test/AdminConsole/Utilities/IntegrationTemplateProcessorTests.cs

Issue: The new tests for TemplateRequiresUser() and TemplateRequiresActingUser() don't verify that #UserType# and #ActingUserType# tokens are correctly detected. The tests only check for #UserName# and #UserEmail#.

Current test data:

[InlineData("User name is #UserName#")]
[InlineData("Email: #UserEmail#")]
public void TemplateRequiresUser_ContainingKeys_ReturnsTrue(string template)

Recommendation: Add test cases:

[InlineData("Type: #UserType#")]
[InlineData("Type: #ActingUserType#")]

(These tests will fail until issue #1 is fixed)

---

5. 💭 Two-Step Database Query for User Details

Location: src/Core/AdminConsole/Services/Implementations/EventIntegrations/EventIntegrationHandler.cs:106-110

var orgUser = await organizationUserRepository.GetByOrganizationAsync(organizationId: organizationId, userId: eventMessage.UserId.Value);
if (orgUser is not null)
{
    context.User = await organizationUserRepository.GetDetailsByIdAsync(orgUser.Id);
}

Issue: This performs two database queries when potentially one could suffice. First it fetches the OrganizationUser to get the ID, then fetches the details.

Consideration: This is a performance consideration rather than a bug. If IOrganizationUserRepository had a method like GetDetailsByOrganizationAndUserIdAsync(), it could eliminate the first query. However, this may be outside the scope of this PR.

---

6. 🎨 Missing Using Directive Cleanup in Test File

Location: test/Core.Test/AdminConsole/Services/EventIntegrationHandlerTests.cs:5

using Bit.Core.Entities;

Issue: The User entity is no longer used in this test file (replaced with OrganizationUserUserDetails), but the using directive for Bit.Core.Entities remains.

Recommendation: Remove unused using directives (though modern IDEs and build tools typically handle this automatically).

---

Security Implications

✅ No security issues identified

• The changes maintain the zero-knowledge principle by only exposing organization-scoped user data
• No sensitive data (passwords, keys, vault data) is being logged or exposed
• The switch from User to OrganizationUserUserDetails is actually more secure as it provides organization-specific context rather than global user data
• Template token replacement is safe and doesn't introduce injection vulnerabilities

---

Performance Considerations

7. ℹ️ Additional Database Queries

Impact: Each new template property that requires data fetching adds database queries:

• #UserType#, #UserName#, #UserEmail#: 2 queries (GetByOrganization + GetDetailsById)
• #ActingUserType#, #ActingUserName#, #ActingUserEmail#: 2 queries
• #GroupName#: 1 query
• #OrganizationName#: 1 query

Analysis: The existing optimization of checking template requirements before fetching is good practice. However, the two-step query for user details (mentioned in #5) could be optimized.

Overall Assessment: Performance impact is acceptable given that:

1. These are background event processing operations
2. The template-checking optimization prevents unnecessary queries
3. The queries are only executed when templates actually reference these properties

---

Good Practices Observed

• ✅ Comprehensive unit test coverage for all new properties
• ✅ Proper use of nullable reference types throughout
• ✅ Template requirement checking to avoid unnecessary database calls
• ✅ Consistent naming conventions and code style
• ✅ Proper dependency injection pattern maintained
• ✅ Test fixtures properly updated to use new types

---

Action Items for Author

1. MUST FIX: Update TemplateRequiresUser() and TemplateRequiresActingUser() to include checks for #UserType# and #ActingUserType# tokens (Issue Identity Server  #1)

2. SHOULD FIX: Add test cases for the new #UserType# and #ActingUserType# tokens to the template processor tests (Issue Licence #4)

3. CONSIDER: Add warning logging when GetDetailsByIdAsync() returns null after successfully fetching orgUser (Issue Password Requirements Validation #2)

4. CONSIDER: Review group fetching logic for consistency with user fetching pattern (Issue .NET Core Runtime #3)

5. OPTIONAL: Clean up unused using directives in test file (Issue LDAP authentication? #6)

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.2% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Checkmarx One – Scan Summary & Details – 828e7123-a9b9-456a-8885-6ddb6b0779fb

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs: 72	details Method at line 72 of /src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs gets a parameter from a user request from user. This pa... ID: ZWiNUnwMpO2ystke8b8neSGcojU%3D Attack Vector

[codecov]

Codecov Report

❌ Patch coverage is 81.08108% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.97%. Comparing base (2eccddf) to head (26553e2).

Files with missing lines	Patch %	Lines
...SharedWeb/Utilities/ServiceCollectionExtensions.cs	0.00%	4 Missing ⚠️
...tions/EventIntegrations/EventIntegrationHandler.cs	85.71%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##           brant/unit-test-coverage    #6528      +/-   ##
============================================================
+ Coverage                     51.96%   51.97%   +0.01%     
============================================================
  Files                          1905     1905              
  Lines                         84274    84302      +28     
  Branches                       7512     7520       +8     
============================================================
+ Hits                          43794    43817      +23     
- Misses                        38789    38793       +4     
- Partials                       1691     1692       +1     

☔ 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]

I assume my review is not required because this is merging into a feature branch (so codeowner requirements don't apply) and Architecture Team are handling this work. Please re-request my review if this is not the case.