Skip to content

HYPERFLEET-1278 - feat: remove email restriction for audit fields#64

Open
rh-amarin wants to merge 1 commit into
openshift-hyperfleet:mainfrom
rh-amarin:remove-email-constraint
Open

HYPERFLEET-1278 - feat: remove email restriction for audit fields#64
rh-amarin wants to merge 1 commit into
openshift-hyperfleet:mainfrom
rh-amarin:remove-email-constraint

Conversation

@rh-amarin

Copy link
Copy Markdown
Collaborator

Summary

Remove the email pattern restriction to *_by fields, so another JWT claim that doesn't match an email pattern can be used for identity, like adapters using a k8s token

@openshift-ci openshift-ci Bot requested review from pnguyen44 and vkareh June 25, 2026 11:55
@openshift-ci

openshift-ci Bot commented Jun 25, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vkareh for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
📝 Walkthrough

Summary by CodeRabbit

  • Chores
    • Bumped the service and package versions to 1.0.25.
  • Bug Fixes
    • Updated identity-related fields in the API schema to be plain text values instead of email-formatted strings, improving compatibility with a wider range of inputs.

Walkthrough

Bumped version metadata from 1.0.24 to 1.0.25 in main.tsp, package.json, and schemas/core/openapi.yaml. Removed @format("email") from APIMetadata.created_by, updated_by, and deleted_by, and changed the matching OpenAPI fields in Cluster, NodePool, NodePoolCreateResponse, and Resource to plain strings without email formatting constraints. CWE-20.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the main change: removing the email restriction on audit fields.
Description check ✅ Passed The description matches the change by explaining the email restriction removal for *_by identity fields.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Sec-02: Secrets In Log Output ✅ Passed No non-test log calls or fmt.Print* sites contain token/password/credential/secret; changed files are schema metadata only, so no CWE-532/312 leak.
No Hardcoded Secrets ✅ Passed No CWE-798/CWE-321 hardcoded secret indicators in the PR diff: only version bumps and email-format removal; no apiKey/secret/token/password literals or embedded creds.
No Weak Cryptography ✅ Passed No crypto/md5, crypto/des, crypto/rc4, SHA1-for-security, ECB, custom crypto, or secret compares found in touched files; no CWE-327/CWE-208.
No Injection Vectors ✅ Passed No CWE-89/78/79/502 vectors in the patch; only version bumps and removal of email format constraints in schema metadata.
No Privileged Containers ✅ Passed PASS: Touched files are version/spec/model metadata only; no Kubernetes/OpenShift manifests, Helm templates, or Dockerfiles, and no privileged flags found.
No Pii Or Sensitive Data In Logs ✅ Passed Touched files only bump versions and relax *_by schema formats; no slog/logr/zap/fmt.Print or other logging was added. No CWE-532 exposure.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shared/models/common/model.tsp`:
- Around line 159-168: The identity fields in the shared model were widened to
plain string, but downstream consumers still treat them as email-typed values.
Update the contract and any dependent presenters/serializers together,
especially the APIMetadata created_by and updated_by fields and their downstream
Email conversions, so the schema change is released only with matching type
handling in the HyperFleet API path.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 255ba7ff-3253-4654-9901-7ba4dcf5582e

📥 Commits

Reviewing files that changed from the base of the PR and between e527445 and f24d225.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • main.tsp
  • package.json
  • schemas/core/openapi.yaml
  • shared/models/common/model.tsp
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • openshift-hyperfleet/architecture (manual)
  • openshift-hyperfleet/hyperfleet-api (manual)
  • openshift-hyperfleet/hyperfleet-sentinel (manual)
  • openshift-hyperfleet/hyperfleet-adapter (manual)
  • openshift-hyperfleet/hyperfleet-broker (manual)

Comment on lines +159 to +168
created_by: string;

@doc("Identity that last updated the resource")
@format("email") updated_by: string;
updated_by: string;

@doc("Timestamp when deletion was requested; omitted if not marked for deletion")
@format("date-time") deleted_time?: string;

@doc("Identity that requested deletion; omitted if not marked for deletion")
@format("email") deleted_by?: string;
deleted_by?: string;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Cross-repo contract break risk: identity fields widened to string while downstream still enforces email types (CWE-436).

APIMetadata.*_by is now unconstrained string, but openshift-hyperfleet/hyperfleet-api presenters still cast these fields to openapi_types.Email and use email converters. Release this spec change only with coordinated downstream type updates to avoid integration and serialization mismatches.

As per path instructions, "Validate changes against HyperFleet architecture standards from the linked architecture repository."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shared/models/common/model.tsp` around lines 159 - 168, The identity fields
in the shared model were widened to plain string, but downstream consumers still
treat them as email-typed values. Update the contract and any dependent
presenters/serializers together, especially the APIMetadata created_by and
updated_by fields and their downstream Email conversions, so the schema change
is released only with matching type handling in the HyperFleet API path.

Sources: Path instructions, Linked repositories

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.

1 participant