Skip to content

fix(server,ui): use relative expires_in for install key expiration - #6860

Merged
otavio merged 1 commit into
masterfrom
feat/ui/install-key-relative-expiration
Aug 5, 2026
Merged

fix(server,ui): use relative expires_in for install key expiration#6860
otavio merged 1 commit into
masterfrom
feat/ui/install-key-relative-expiration

Conversation

@luizhf42

@luizhf42 luizhf42 commented Aug 4, 2026

Copy link
Copy Markdown
Member

What

Install key create/update API now accepts expires_in (days from now) instead of expires_at (absolute ISO date-time). The server computes the absolute timestamp. The UI keeps the calendar day-picker but converts selections to relative days internally.

Why

Users west of UTC saw the wrong expiration date — picking Sep 3 showed Sep 2. The frontend built a midnight-UTC ISO string and date-fns format() rendered it in local time, shifting it back a day. Rather than patching the timezone math, the API was changed to accept relative days (matching the SSH identity and API key pattern), eliminating client-side date computation entirely.

Changes

  • OpenAPI spec: expires_at (string, date-time, nullable) replaced with expires_in (integer, minimum 1, optional) on create and update. Response expires_at changed from format: date to format: date-time.
  • Go request structs: ExpiresAt *time.TimeExpiresIn *int on create. OptionalTimeOptionalInt on update (same RFC 7396 three-state semantics: omit = unchanged, null = never, value = days from now).
  • Go service: validateInstallKeyExpiry replaced with installKeyExpiry(days *int) *time.Time, mirroring sshIdentityExpiry. The server computes the absolute timestamp via clock.Now().AddDate(0, 0, *days).
  • Go tests: create-path tests updated for the new field. Four new update-path tests (set/clear/leave-unchanged/reject-invalid) — this seam previously had zero expiry coverage.
  • UI ExpirationField: keeps the calendar day-picker but converts selections to day counts via differenceInCalendarDays. Presets (30d/60d/90d/1y) compare by day count instead of ISO strings.
  • Install key drawers: both drawers send expires_in via keyExpiryPayload (create) and keyExpiryUpdatePayload (update). An expiryTouched flag in the edit drawer ensures expires_in is only sent when the user changes the field, preventing silent expiry drift on unrelated edits. An expired-key Callout warns the user and clears when they pick a new date.

Testing

  • Create an install key with each preset (30d, 60d, 90d, 1y, Never) and the calendar — verify the list page shows the correct expiry.
  • Edit an existing key's expiration (preset → calendar pick, calendar → Never, Never → preset).
  • Edit an unrelated field (name, tags) on a key with an expiry — verify the expiry is unchanged after save.
  • Open an expired key's edit drawer — verify the warning Callout appears and clears when a new date is picked.
  • Verify in a timezone west of UTC (e.g. TZ=America/Sao_Paulo) — the displayed date should match what was chosen.

@luizhf42
luizhf42 requested review from a team as code owners August 4, 2026 19:23
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Claude finished @luizhf42's task in 11m 22s —— View job


Code Review Complete

Reviewed 13 files across code quality, security, testing, language patterns, and architecture. 3 inline issues posted (2 high, 1 medium) — all in the frontend edit flow.

  • [high] EditInstallKeyDrawer.tsx:81 — editing a key hardcodes expiresIn to "30", so saving any unrelated change silently resets the expiry to 30 days from now.
  • [high] EditInstallKeyDrawer.tsx:141 — selecting Never omits expires_in, but the update endpoint's RFC 7396 merge-patch treats omit as "unchanged"; the expiry is never cleared.
  • [medium] KeyExpiryField.tsx:21editingCustom is seeded from props once and never resyncs; because the Drawer uses inert rather than unmounting, the Custom/preset pill can desync from the actual value across reopens.

Additional notes

The Go/server side is clean: the OptionalTimeOptionalInt rename, the installKeyExpiry day→timestamp conversion, and the create/update validation (min=1 tag + manual service guard) are consistent and well covered by the new tests. Security review found no exploitable issues (the unbounded AddDate matches the existing api-key/ssh-identity pattern and expiry enforcement fails safe). The cross-repo check confirmed the enterprise cloud/ repo does not touch the install-key request contract, so the struct rename causes no compile breakage.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread ui/apps/console/src/pages/install-keys/EditInstallKeyDrawer.tsx Outdated
Comment thread ui/apps/console/src/pages/install-keys/EditInstallKeyDrawer.tsx Outdated
Comment thread ui/apps/console/src/components/common/KeyExpiryField.tsx Outdated
@luizhf42
luizhf42 force-pushed the feat/ui/install-key-relative-expiration branch from 336715d to 3750396 Compare August 4, 2026 19:47
@otavio

otavio commented Aug 4, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 3m 35s —— View job


Code Review Complete

Reviewed 13 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 medium) — a residual edge case in the frontend expiry field.

  • [medium] KeyExpiryField.tsx:25 — the Custom-pill resync only demotes custom→preset and never promotes preset→custom, so reopening a persisted Edit drawer for a key with a non-preset lifetime (after touching the field) shows no pill selected and hides the custom input. This is the inverse of the previously-fixed stale-editingCustom case.

Additional notes

The three issues from the prior review round are correctly resolved:

  • Edit initializer now derives expiresIn from the real expires_at via differenceInCalendarDays(...) clamped to ≥1, instead of hardcoding "30".
  • Never on update now emits expires_in: null (Number(expiresIn) > 0 ? … : null), so the RFC 7396 merge-patch actually clears the expiry.
  • editingCustom resync was added (custom→preset direction); the remaining gap is the inverse direction noted inline.

The Go/server side is clean: the OptionalTimeOptionalInt rename, installKeyExpiry(days) day→timestamp conversion (clock.Now().AddDate(0, 0, *days)), and the create/update validation (min=1 tag + service guard emitting expires_in: must be at least 1) are consistent. The four new update-path tests (set / clear-on-null / leave-on-omit / reject-below-1) cover the seam well. OpenAPI, the create-drawer wiring, and the ExpirationField deletion (with react-day-picker removal) are consistent. No security or cross-repo (cloud/) contract concerns.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.
feat/ui/install-key-relative-expiration

Comment thread ui/apps/console/src/components/common/KeyExpiryField.tsx Outdated
@luizhf42
luizhf42 force-pushed the feat/ui/install-key-relative-expiration branch 2 times, most recently from 3376ec3 to 0f68336 Compare August 4, 2026 21:26
@otavio

otavio commented Aug 4, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 8m 3s —— View job


Code Review Complete

Reviewed 14 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 low) — a residual edge case in the expiry field.

  • [low] ExpiryField.tsx:34 — the render-time resync only clears customError when the incoming value is a preset, so a stale "Must be at least 1 day." error keeps rendering when expiresIn resyncs to a valid non-preset value (e.g. reopening a 47-day key after typing an invalid custom value then cancelling). Cosmetic — it doesn't block submit (the parent's expiryError resets independently) — but the field shows a valid number under a contradicting error.

Additional notes

  • OpenAPI spec drift (low, not inlinable — file not in this PR's diff): this PR changed the install-key response expires_at from format: dateformat: date-time in openapi/spec/components/schemas/installKey.yaml, but the parallel create-key response schema openapi/spec/components/schemas/installKeyWithKey.yaml:54 still declares expires_at as format: date (example 2026-08-20). Both document the same models.InstallKey.ExpiresAt (*time.Time), so the create endpoint now advertises a different format than get/list. Consider updating installKeyWithKey.yaml to format: date-time (example 2026-08-20T00:00:00Z) to match.

The three issues from the prior rounds are correctly resolved: the Edit initializer derives expiresIn from the real expires_at (round-trips within calendar-day rounding), Never on update emits expires_in: null to clear via RFC 7396 merge-patch, and the editingCustom pill now resyncs in both directions.

The Go/server side is clean: the OptionalTimeOptionalInt rename, installKeyExpiry(days) conversion (clock.Now().AddDate(0, 0, *days)), and create/update validation (min=1 tag + service guard) are consistent and well covered by the four new update-path tests. Security review confirmed the unbounded AddDate matches the existing ssh-identity/api-key pattern and expiry enforcement fails safe. The cross-repo check confirmed cloud/ does not reference the renamed install-key request contract, so no compile breakage.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.
feat/ui/install-key-relative-expiration

Comment thread ui/apps/console/src/pages/install-keys/ExpiryField.tsx Outdated
@otavio

otavio commented Aug 4, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 2m 17s —— View job


Code Review Complete

Reviewed 14 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

The current HEAD is unchanged since the previous /review round, and the earlier feedback is correctly resolved: the Edit initializer derives expiresIn from the real expires_at via differenceInCalendarDays(...) clamped to ≥1, Never emits expires_in: null to clear via RFC 7396 merge-patch, the editingCustom pill resyncs in both directions, and the system-key branch omits expires_in entirely. The Go side (OptionalTimeOptionalInt, installKeyExpiry(days) day→timestamp conversion, min=1 tag + service guard) is consistent and well covered by the four new update-path tests. No security or cross-repo (cloud/) contract concerns.

Two low-severity items remain open from prior rounds (already noted in earlier threads, not re-posted): the ExpiryField stale-custom-error edge case on external resync, and the installKeyWithKey.yaml response schema still declaring expires_at as format: date while installKey.yaml now uses format: date-time.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.
feat/ui/install-key-relative-expiration

@luizhf42
luizhf42 force-pushed the feat/ui/install-key-relative-expiration branch 2 times, most recently from 4ca8f90 to 950fd3f Compare August 5, 2026 12:18
@otavio

otavio commented Aug 5, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 13m 46s —— View job


Code Review Complete

Reviewed 15 files across code quality, security, testing, language patterns, and architecture. 2 inline issues posted (1 high, 1 medium) — both in the frontend edit/expiry flow.

  • [high] EditInstallKeyDrawer.tsx:153 — the edit form always resends expires_in, and the initializer clamps an already-expired key's value to "1". Since Edit isn't gated by expiry, saving any unrelated change (rename, tags, mode) on an expired-but-not-revoked key silently recomputes ExpiresAt = now + 1 day server-side, reactivating it for device enrollment. The update endpoint's RFC 7396 OptionalInt already supports "omit = unchanged"; the field should only be sent when the user actually touched the expiry control. This is distinct from the previously-accepted ±1-day rounding tradeoff (which concerned still-valid keys).
  • [medium] ExpiryField.tsx:34 — the render-phase state-sync block calls setErr(""), which invokes the parent drawer's setExpiryError mid-render, triggering React's "Cannot update a component while rendering a different component" warning on the ordinary preset-pill click flow. Updating only the local setCustomError("") there is sufficient, since every external path already clears the parent error independently.

Additional notes

The Go/server side remains clean: the OptionalTimeOptionalInt rename, installKeyExpiry(days) day→timestamp conversion, the create/update validation (min=1 tag + service guard), and the four new update-path tests (set / clear-on-null / leave-on-omit / reject-below-1, all under a deterministic mocked clock) are consistent and well covered. Security review found no exploitable regressions (the unbounded AddDate matches the existing ssh-identity/api-key pattern, and the system-key guard still rejects ExpiresIn.Present). The OpenAPI drift and stale-custom-error items from prior rounds are resolved, and the cross-repo check confirmed cloud/ does not reference the renamed install-key request contract, so no compile breakage.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.
feat/ui/install-key-relative-expiration

Comment thread ui/apps/console/src/pages/install-keys/EditInstallKeyDrawer.tsx Outdated
Comment thread ui/apps/console/src/pages/install-keys/ExpiryField.tsx Outdated
@luizhf42
luizhf42 force-pushed the feat/ui/install-key-relative-expiration branch 3 times, most recently from e72f385 to 2328f56 Compare August 5, 2026 19:33
@gustavosbarreto

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @gustavosbarreto's task in 7m 35s —— View job


Code Review Complete

Reviewed 11 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 medium) — a client-side expiry-state inconsistency in the Edit drawer.

  • [medium] helpers.ts:216getRemainingDays uses differenceInCalendarDays and treats remaining < 1 as expired, so a key whose expiry is later the same calendar day is flagged expired: true (showing the "This key has expired" Callout) while the list view — using the precise isInstallKeyExpired (getTime() <= Date.now()) — still shows it as valid. It's gated behind expiryTouched, so it can't corrupt the persisted expiry; it's a misleading warning only.

Additional notes

The rest of the change is clean and the prior review rounds are fully resolved:

  • Go/server: the OptionalTimeOptionalInt rename, installKeyExpiry(days) (clock.Now().AddDate(0, 0, *days)), and validation on both paths are consistent — create enforces min=1 via the validate:"omitempty,min=1" tag through c.Validate(req) (routes/install-key.go:33, so a pointer-to-0 is rejected), update guards *Value < 1, and the system-key branch still rejects ExpiresIn.Present. The four new update-path tests run under a mocked clock and assert the computed ExpiresAt.
  • Security: no exploitable issues — the unbounded AddDate matches the existing api-key/ssh-identity pattern and overflow fails safe (a wrapped past instant is already-expired). The expiryTouched gate means unrelated edits omit expires_in, so an expired key is not silently reactivated.
  • OpenAPI drift resolved: installKeyWithKey.yaml response expires_at is now format: date-time, matching installKey.yaml.
  • Cross-repo: cloud/ does not reference the renamed install-key request contract — no compile breakage.

Two lower-signal items were considered and not posted: a dormant NaN path in ExpirationField/getRemainingDays (not reachable via any current caller) and the absence of a create-path min=1 validation test.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.
feat/ui/install-key-relative-expiration

Comment thread ui/apps/console/src/pages/install-keys/helpers.ts
@luizhf42
luizhf42 force-pushed the feat/ui/install-key-relative-expiration branch from 2328f56 to b75ed04 Compare August 5, 2026 19:56
@otavio

otavio commented Aug 5, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 14m 30s —— View job


Code Review Complete

Reviewed 11 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 medium) — a missing upper bound on the new expires_in field.

  • [medium] pkg/api/requests/install-key.go:44expires_in is validated only min=1 with no upper bound. installKeyExpiry feeds it straight into clock.Now().AddDate(0, 0, *days), and any value above ~2.9M days yields an ExpiresAt whose year exceeds 9999, which time.Time.MarshalJSON refuses to serialize. PostgreSQL stores the intermediate value fine, so a plainly valid expires_in: 3000000 persists the row but then breaks the CreateInstallKey response (losing the one-time key) and poisons ListInstallKeys for the whole namespace until the row is fixed in the DB. This is newly introduced — the prior absolute expires_at was implicitly capped at year 9999 by RFC 3339 parsing. Fix needs the max= tag on create and a matching guard on the update path (server/api/services/install-key.go:432).

Additional notes

The rest of the change is clean and prior review rounds are fully resolved:

  • Go/server: the OptionalTimeOptionalInt rename, installKeyExpiry(days) (clock.Now().AddDate(0, 0, *days)), the create (min=1 tag via c.Validate) / update (< 1 service guard) validation, and the system-key branch (req.ExpiresIn.Present) are consistent. The four new update-path tests (set / clear-on-null / leave-on-omit / reject-below-1) run under a mocked clock and assert the computed ExpiresAt.
  • Cross-repo: cloud/ does not reference OptionalTime or the renamed install-key request fields — no compile breakage. No stale OptionalTime/expires_at request references remain in shellhub/.
  • OpenAPI: response expires_at is now date-time in both installKey.yaml and installKeyWithKey.yaml; the create (min=1, non-nullable) and update (min=1, nullable) request schemas match the Go structs.
  • Frontend: the create/edit payload builders correctly omit vs. null vs. positive-integer per the RFC 7396 semantics; the expiryTouched gate keeps unrelated edits from resending expires_in.

One lower-signal UX observation (not posted inline): in the edit drawer, expiryTouched is set on any pick in ExpirationField, including re-clicking the already-active preset/day — that marks the field touched and resends expires_in, drifting the stored expiry forward from the save moment. Gating setExpiryTouched(true) on the value actually changing would tighten it, but it can't corrupt an untouched key so it's minor.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.
feat/ui/install-key-relative-expiration

Comment thread pkg/api/requests/install-key.go Outdated
…es_in`

Users west of UTC saw the wrong expiration date because the frontend
built a midnight-UTC ISO string that `date-fns` rendered in local time,
shifting it back a day. Instead of patching the timezone math, the API
now accepts `expires_in` (days from now) and the server computes the
absolute timestamp via `clock.Now().AddDate(0, 0, days)`.

Server: `OptionalTime` renamed to `OptionalInt` (same RFC 7396
three-state semantics). `installKeyExpiry` replaces the old
`validateInstallKeyExpiry`. Four new update-path tests cover
set/clear/leave-unchanged/reject-invalid.

UI: the calendar picker is kept but converts selections to day counts
via `differenceInCalendarDays`. An `expiryTouched` flag in the edit
drawer ensures `expires_in` is only sent when the user changes the
field, preventing silent expiry drift on unrelated edits. An expired-key
`Callout` warns the user and clears when they pick a new date.
@luizhf42
luizhf42 force-pushed the feat/ui/install-key-relative-expiration branch from b75ed04 to 0d70664 Compare August 5, 2026 20:24
@otavio

otavio commented Aug 5, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 7m 14s —— View job


Code Review Complete

Reviewed 11 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

The final commit (0d70664) resolves the last outstanding item (the missing upper bound on expires_in): max=36500 is now enforced on both paths — the validate:"omitempty,min=1,max=36500" tag via c.Validate on create, and the *req.ExpiresIn.Value < 1 || > 36500 service guard on update — with two new boundary tests (rejects expires_in below 1 / above 36500) under the mocked clock. installKeyExpiry can no longer overflow AddDate, and the OpenAPI create/update schemas match the Go tags.

All prior-round feedback remains correctly resolved: the OptionalTimeOptionalInt RFC 7396 rename, the expiryTouched gate that keeps unrelated edits from resending expires_in (so an expired key is never silently reactivated), the getRemainingDays elapsed-time expired check, and the response expires_at datedate-time alignment across installKey.yaml / installKeyWithKey.yaml. Security review confirmed expiry enforcement fails safe and the system-key guard still rejects ExpiresIn.Present; the cross-repo check confirmed cloud/ does not reference the renamed contract, so no compile breakage.

Two lower-signal items were considered and not posted (both already weighed in earlier rounds): a dormant NaNInvalid Date path in ExpirationField/getRemainingDays that is unreachable unless the API returns a malformed expires_at (which *time.Time marshaling never produces), and the duplicated 1/36500 magic numbers across the create tag and update guard.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.

@otavio
otavio merged commit f39fd4f into master Aug 5, 2026
39 checks passed
@otavio
otavio deleted the feat/ui/install-key-relative-expiration branch August 5, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants