feat(ui): standardize schema-driven credential file upload with drag-… - #33000
Conversation
…and-drop
Connector credential fields marked `uiFieldType: file | fileOrInput` in their
JSON Schema now render one shared picker with drag-and-drop, instead of two
divergent implementations that each read the file themselves.
Component library
- Add `CredentialFileInput`, composed from the existing `FileUploadDropZone`
and `PasswordInput`. It reads the file in the browser and submits its text
as the field value, so the secret keeps the masking and secrets-manager
handling every other password field gets — nothing is uploaded anywhere.
- Decode strictly with `TextDecoder('utf-8', { fatal: true })` plus a NUL scan.
`File.text()` decodes leniently, so a DER or PKCS#12 payload used to come
back as replacement characters, get saved as the secret, and only fail much
later at connection time.
- Reject wrong-extension, oversized, binary and unreadable files with a
`role="alert"` message, leaving the stored value untouched.
- Read through `FileReader` rather than `Blob.arrayBuffer()`: jsdom implements
the former and not the latter, so the path stays reachable from unit tests.
- Polyfill `DataTransfer` in the vitest setup. jsdom ships none, and
`filesToFileList` in `file-upload.tsx` builds every `FileList` through it, so
no drop-zone consumer was testable before.
Both form stacks
- `CorePasswordWidget` (FormBuilderV1) and `PasswordWidget` (legacy RJSF) both
delegate to the new component through one shared schema-to-props mapping.
- `file` and `fileOrInput` now behave differently. `CorePasswordWidget` treated
them identically — both rendered an editable textarea — and the second
branch's `isInputTypeFileOrInput` conditionals were unreachable dead code.
- Blank the readback mask in credential-file fields. The API returns `*********`
in place of the stored secret; rendering it invited the user to edit a value
that is not the credential. `formData` is left untouched, so an unmodified
field round-trips the mask and the backend keeps the existing secret.
- Delete `FileUploadWidget` and its antd `Upload` + `Radio.Group` wrapper, which
duplicated the file-reading behaviour and displayed the secret content as the
file name.
Known gap: the schemas still advertise `.der` and `.p12` in `accept`, which the
new decoder rejects. Removing them is the schema half of this work and lands
next; see #31677.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Jest test CoverageUI tests summary
|
…ations Both failures were mine, and both were tests pinned to behaviour this branch deliberately changed. **NATS unit tests.** `test_failed_temp_certificate_write_removes_file` and `test_zero_byte_certificate_write_reports_cleanup_failure` patched `nats.connection.tempfile.mkstemp` and `nats.connection.os.write`, neither of which the module imports any more — the write moved to `metadata.utils.secure_tempfile`. The zero-byte case tested a failure mode that no longer exists: the hand-rolled loop raised "Could not write" on a short write, where `handle.write` now handles partial writes itself. Replaced with the contract that still belongs to this module — a path joins the cleanup list only once it exists — plus a positive test that the certificate is written and tracked. Removing the partial file is the helper's job and is covered in tests/unit/utils/test_secure_tempfile.py. **ServiceForm Playwright spec.** "Verify SSL cert upload with long filename" asserted the uploaded content appeared in the paste box; an attached credential is now represented by its chip instead, so that locator no longer resolves. The assertion moves to the chip, which is where the long file name now lands and so is the better subject for a test about name overflow. The valuable half — that the submitted `caCertificate` equals the file content — is untouched, and it is the end-to-end proof that an uploaded file and pasted text submit the same value. Verified locally this time: 91 NATS tests pass, 137 of 138 ssl_manager tests pass (the one failure is a missing `cassandra` driver in this environment), and `yarn lint:playwright` exits 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Too many files changed for review (106 files, 100 file limit). Bypass the limit by tagging |
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
An edit form showed "Saved credential" as a file name, under a file icon, with a remove button — which reads as "you attached a file called Saved credential". Nothing was attached: the API returns a mask, so there is no name and no size to show. A credential the form cannot read now gets its own row — a key icon, "Saved credential", and a line saying it is hidden for security and must be removed before a new one can be uploaded or pasted. The file chip is reserved for a file actually attached in this session, which is the only case with a real name. The same row covers a value restored into an upload-only field, where there is likewise no provenance to show. Reported from a real edit form; the file-chip case is unchanged, so the ServiceForm E2E assertion on the long file name still holds.
…ential-file-upload # Conflicts: # openmetadata-ui/src/main/resources/ui/src/components/common/Form/JSONSchema/JsonSchemaWidgets/PasswordWidget.test.tsx # openmetadata-ui/src/main/resources/ui/src/components/common/Form/JSONSchema/JsonSchemaWidgets/PasswordWidget.tsx
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
The field took part in RJSF validation but nothing proved it: no test submitted a required credential field empty, so a silent failure at save would have gone unnoticed. Covers the whole chain — the widget forwards `rawErrors` as `isInvalid` plus the hint, the component renders that hint, and a file rejection replaces the form error while it is showing rather than stacking two messages. Also records what an audit of the issue's contract turned up: the CSV export of a service carries no connection config at all (columns are name, displayName, description, owner, tags, glossaryTerms, tiers, certification, domains, extension), so an uploaded credential is structurally absent from it rather than merely masked. Verified against a running instance with a service whose private key was a known sentinel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
`src/enums/*.enum.ts` is where this repo puts enums (30 files), and it is where the two files this PR deletes -- File.enum.ts and PasswordWidget.enum.ts -- lived. Folding their replacement enum into a utils module quietly broke the convention the rest of the PR is cleaning up, and left CredentialFileField.utils.ts holding something other than the pure functions DEVELOPER_HANDBOOK asks a *.utils.ts to hold. None of the four sibling src/utils/*.utils.ts files export an enum. No behaviour change: the enum moves, the three call sites import it from its new home. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tants, utils The component file held its own constants, types and file-reading helpers alongside the component. Split them into siblings so the .tsx defines only components, following the `<name>.types.ts` pattern the package already uses for form-field, tree-select, page-header and page-layout, each with its own barrel line. The public API is unchanged, so no consumer import moves. This surfaced a bug in the component's own test. It imported DEFAULT_CREDENTIAL_FILE_MAX_SIZE from './credential-file-input', which after the split no longer exports it -- so the constant was undefined, the fake file size became NaN, and "rejects a file over the size limit" was passing an oversized file straight through instead of rejecting it. `tsc --noEmit` stayed green because the test file is not in its program; only running the tests caught it. The import now points at the constants module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Splitting the component into siblings and barrelling each with `export *` widened this package's public surface from 6 exports to 14, publishing internals that were never part of the contract: the file-reading helpers, and `CREDENTIAL_ROW_CLASS`, which is a raw Tailwind class string. `DEFAULT_LABELS` and `DEFAULT_VALIDATION_MESSAGES` are also names a sibling (cover-image-upload-field) already uses locally, so putting them in the barrel invites a collision the moment either side exports one. Export only what was public before the split: the component, the types, and the size cap. Nothing outside the component folder used the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review 👍 Approved with suggestions 3 resolved / 4 findingsStandardizes credential file upload across form stacks with a unified drag-and-drop component, file picker, and validation. Resolves the fileOrInput dual-affordance issue, adds clear-to-remove semantics for saved credentials, and fixes an fd leak in secure tempfile creation. Consider correcting the Portuguese strings in the Persian locale file ( 💡 Quality: pr-pr.json (Persian) locale contains Portuguese strings📄 openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json:3159 📄 openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json:3476 📄 openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json:3478-3479 📄 openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json:3481-3483 📄 openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json:3485-3486 📄 openmetadata-ui/src/main/resources/ui/src/locale/languages/pr-pr.json:3488 Several newly added keys in the Persian locale file ✅ 3 resolved✅ Quality: fileOrInput shows chip and prefilled textarea at once
✅ Edge Case: No affordance to clear a saved credential in file mode
✅ Edge Case: fchmod failure leaks the open file descriptor
🤖 Prompt for agentsOptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
|
4 participants
Describe your changes:
Fixes #31677
Replaced the per-surface credential upload affordances with one schema-driven component, because the same JSON Schema annotation was rendering three different ways depending on which form stack drew it — and two of the three could not accept a file at all. A
format: passwordstring markeduiFieldType: file | fileOrInputnow renders the same drop zone, file picker, filename chip and validation in every connection form, in both the legacy RJSF stack and FormBuilderV1.Type of change:
High-level design:
Key files.
credential-file-input.tsx(new) — drop zone and picker share one handler, so the two input paths cannot diverge. Filename chip, remove-to-clear, optional paste box forfileOrInput. Four rejection kinds:unacceptedType,sizeLimit,binary,unreadable; 1 MiB default cap.CredentialFileField.utils.ts+enums/CredentialFileField.enum.ts(new) — the schema→props contract both stacks read, so they cannot drift.PasswordWidget.tsx,CorePasswordWidget.tsx— rewired to it.secure_tempfile.py(new) — one helper for writing secrets to disk, replacing copy-pastedmkstempblocks; 2 of 5 call sites migrated here.FileUploadWidget.tsx(+test),password-widget.less,File.enum.ts,PasswordWidget.enum.ts. These deletions are the standardisation.Schema contract fixes (the 5 source edits): dropped
.der/.p12fromvalidateSSLClientConfig— both are binary and are rejected at decode, so offering them was a lie; added the missingacceptlists to SnowflakeprivateKeyand SAP SuccessFactors; annotated threesslCertValuesfields; added.jsonto VertexAI.Backward compatibility. No schema shape changes — only
acceptanduiFieldTypeannotations, which are UI-only hints. Secret masking is unchanged: the API returns*********, the chip stands for the stored secret, and removing it clears the field, matching theallowClearsemantics #32945 gave plain passwords.Rollout note. Collate carries the same annotations in a companion PR and must merge after this one — Collate CI resolves the submodule with
--remoteagainst OSSmain, so its annotations are inert until this lands.Tests:
Use cases covered
fileOrInputfields still accept pasted content, but never alongside a chipUnit tests
credential-file-input.test.tsx(new) — 29 testsCredentialFileField.utils.test.ts(new) — including a contract suite that walks every generated connection schema and fails if an annotated field is not aformat: passwordstring, if a path-shaped field is annotated, ifacceptcontains a binary extension, or ifacceptappears without the marker. This is what stops the annotation set rotting as connectors are added.test_secure_tempfile.py(new, 222 lines)PasswordWidget.test.tsx,FormBuilderV1Widgets.test.tsx,ConnectionConfigForm.schema-render.test.tsx,test_nats.pyBackend integration tests
Ingestion integration tests
Playwright (UI) tests
playwright/e2e/Flow/CredentialFileUpload.spec.ts(new) — picker, drag-and-drop, non-UTF-8 rejectionplaywright/e2e/Flow/ServiceForm.spec.ts— the assertion moved from the textarea value to thecredential-file-namechip; the payload assertion is unchanged, so it still pins the submitted valueManual testing performed
maincommit this branch merged, confirming none is caused by this PR.ServiceFormfails 7 on baselinemainand 6 here — this branch fails one fewer.UI screen recording / screenshots:
Screen-Recording.47.mp4
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.acceptanduiFieldTypeannotations; no stored shape changes.