fix(tibuild-v2): honor explicit createdBy in devbuild create

[wuhuizuo]

Summary

Previously devbuild.Create always overwrote the caller-supplied createdBy with the authenticated identity's email whenever an identity was present in the context. This made it impossible for an authenticated caller to attribute a build to another user.

This change only falls back to the authenticated identity when the request does not provide a createdBy. An explicit non-blank createdBy is now preserved, and a blank/missing value still yields DevBuildUnauthorizedError.

Behavior

• createdBy provided and non-blank -> used as-is, even when an identity is present.
• createdBy missing/blank and an authenticated identity exists -> identity email is used.
• createdBy missing/blank and no identity -> 401 DevBuildUnauthorizedError.

Note

This intentionally removes the anti-spoofing guard introduced in #644 (where the authenticated identity always won over a request-provided createdBy). The corresponding assertion in TestPortalDevBuildIdentityAndCapabilities has been updated to expect the explicit createdBy.

Test plan

• go test -short ./...
• go build ./cmd/tibuild ./cmd/tibuild-cli

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary
This PR fixes the devbuild.Create method to respect an explicitly provided createdBy value in the request payload, only defaulting to the authenticated user's email if createdBy is missing or blank. This enables authenticated callers to attribute builds to other users if desired. The approach modifies the conditional logic to only overwrite createdBy if it is missing, otherwise preserving it. Overall, the change is straightforward and improves functionality while maintaining the existing authorization behavior.

---

Critical Issues

• Test coverage is missing

  • File: experiments/tibuild-v2/internal/service/impl/devbuild.go (no test file changed)

  • Why: The PR description notes that tests were not run (go test ./... unchecked), and no new or updated tests are included to cover this behavioral change. This is a critical gap since the logic around authentication and authorization is security-sensitive and prone to regressions.

  • Suggestion: Add unit tests for devbuildsrvc.Create covering these cases:

    • When createdBy is explicitly provided (non-blank), it should be preserved regardless of authentication in context.
    • When createdBy is missing or blank and authenticated user is present, fallback to user email.
    • When createdBy is missing or blank and no authenticated user, return DevBuildUnauthorizedError.

    Example test snippet outline (using Go testing framework):

    func TestCreate_RespectsExplicitCreatedBy(t *testing.T) {
        // Setup context with authenticated identity
        // Create payload with explicit createdBy
        // Call Create and assert createdBy is as provided
    }
    
    func TestCreate_FallbackToAuthenticatedEmail(t *testing.T) {
        // Setup context with authenticated identity
        // Create payload with nil or blank createdBy
        // Call Create and assert createdBy equals user's email
    }
    
    func TestCreate_UnauthorizedWhenNoCreatedByOrIdentity(t *testing.T) {
        // Setup context without identity
        // Create payload with nil or blank createdBy
        // Call Create and expect DevBuildUnauthorizedError
    }

---

Code Improvements

• Simplify and clarify conditional logic

  • File: experiments/tibuild-v2/internal/service/impl/devbuild.go lines ~186-198

  • Why: The current logic nests conditions and mixes pointer checks and trimming in a way that could be clearer and more maintainable.

  • Suggestion: Refactor for readability by explicitly extracting createdBy string value, trimming once, and then applying the fallback logic. For example:

    func (s *devbuildsrvc) Create(ctx context.Context, p *devbuild.CreatePayload) (*devbuild.DevBuild, error) {
        s.logger.Info().Msgf("devbuild.create")
    
        var createdBy string
        if p.CreatedBy != nil {
            createdBy = strings.TrimSpace(*p.CreatedBy)
        }
    
        if createdBy == "" {
            if user, ok := identity.FromContext(ctx); ok {
                createdBy = user.Email
            } else {
                return nil, &devbuild.DevBuildUnauthorizedError{
                    Code: http.StatusUnauthorized,
                    Message: "createdBy or authenticated identity is required",
                }
            }
        }
    
        p.CreatedBy = &createdBy
        // proceed with build creation...
    }

    This makes the logic path easier to follow and avoids multiple pointer checks.

---

Best Practices

• Logging could include createdBy for auditability

  • File: experiments/tibuild-v2/internal/service/impl/devbuild.go line ~187

  • Why: Including the resolved createdBy in logs can help trace build attribution and debug issues.

  • Suggestion: Log the effective createdBy after resolving it:

    s.logger.Info().Str("createdBy", createdBy).Msg("devbuild.create")

• Update PR description and checklist after tests added

  • Mark the tests checkbox as done after adding coverage to ensure completeness.

---

Summary: Add missing unit tests to secure this important auth behavior fix, refactor the conditional logic for clarity, and improve logging for better observability. These changes will increase maintainability, correctness, and auditability of the code.

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary:
This PR fixes the devbuild.Create method to preserve an explicitly supplied createdBy field instead of overwriting it with the authenticated identity's email. The implementation changes the precedence logic to use the caller's explicit createdBy if present and non-blank, fallback to authenticated identity when missing, and return unauthorized if neither is available. The code changes are small, focused, and the added test coverage appropriately verifies the new behavior. Overall, this is a well-scoped fix with a clear rationale.

---

Code Improvements

• Create method logic clarity (file: devbuild.go, lines ~186-198)
  The current nested ifs could be simplified for clarity. For example:

  if p.CreatedBy != nil && strings.TrimSpace(*p.CreatedBy) != "" {
      // Use explicitly provided createdBy
  } else if user, ok := identity.FromContext(ctx); ok {
      p.CreatedBy = &user.Email
  }
  
  if p.CreatedBy == nil || strings.TrimSpace(*p.CreatedBy) == "" {
      return nil, &devbuild.DevBuildUnauthorizedError{...}
  }

  This would be more readable and explicitly reflect the precedence order.

• Trim spaces on explicit createdBy early
  Currently, the check for blank createdBy uses strings.TrimSpace but the value assigned remains untrimmed. Consider trimming the createdBy string upfront to avoid surprises where a string with spaces is considered "non-blank" but stored as-is:

  if p.CreatedBy != nil {
      trimmed := strings.TrimSpace(*p.CreatedBy)
      if trimmed != "" {
          p.CreatedBy = &trimmed
      } else {
          p.CreatedBy = nil
      }
  }

• Comment explaining the precedence logic
  Adding a short comment in the Create function about the precedence of createdBy vs authenticated identity will improve maintainability.

---

Best Practices

• Testing coverage completeness (file: devbuild_portal_test.go)
  The tests cover explicit createdBy and fallback to authenticated user, but there is no test verifying that missing createdBy and no authenticated identity yields a 401 error. Adding such a test would strengthen coverage:

  func TestCreate_AnonymousWithoutCreatedBy_ThrowsUnauthorized(t *testing.T) {
      env := setupTestEnv(t)
      defer teardownTestEnv(env)
  
      _, err := env.service.Create(context.Background(), &devbuild.CreatePayload{
          Request: &devbuild.DevBuildSpec{Product: "pd"},
      })
      require.Error(t, err)
      var unauthorized *devbuild.DevBuildUnauthorizedError
      require.ErrorAs(t, err, &unauthorized)
      assert.Equal(t, http.StatusUnauthorized, unauthorized.Code)
  }

• Test case naming and comments
  Some test code could benefit from clearer naming and inline comments to describe the intent, especially distinguishing the "explicit createdBy overrides auth" case.

• Logging message
  The log line s.logger.Info().Msgf("devbuild.create") could be enhanced to indicate if createdBy was set explicitly or derived, aiding debugging:

  s.logger.Info().
      Str("createdBy", *p.CreatedBy).
      Msg("devbuild.create")

---

Summary of Suggested Actions

• Refactor Create method for clearer precedence logic and trim createdBy strings.
• Add a comment explaining createdBy resolution rules.
• Add a test for missing createdBy with no authenticated user to confirm 401 error.
• Improve test naming and add clarifying comments.
• Augment logging with createdBy information for better observability.

These changes will improve code clarity, robustness, and test coverage without changing the core fix implemented.

[wuhuizuo]

/approve

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [wuhuizuo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment