Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update silo creation form to require a TLS certificate #2579

Merged
merged 8 commits into from
Dec 3, 2024

Conversation

charliepark
Copy link
Contributor

@charliepark charliepark commented Nov 26, 2024

This fixes a bug in the silo creation form, where a silo could be created without entering in TLS information.

The sub-form already had the TLS fields marked as required, but those requirements don't bubble up to the higher-order form. This adds a requirement to the body of the form, that the tls_certificates array be not-empty.

Augustus had an interesting idea about verifying Common Name and Subject Alternative Names in the cert fields, but this PR doesn't address that yet.

Closes #2578

Copy link

vercel bot commented Nov 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Dec 3, 2024 9:56pm

@charliepark
Copy link
Contributor Author

Chatting through this with Ben, and he's noting that it would be good for us to consider a minItems: 1 on the API for tls_certificates, so this is protected via CLI as well.

All of the current types in Nexus that have a minItems value also have an identical maxItems value, so are of a fixed length. So if approaching this in Rust, we might need to approach it differently than in other spots in the code.

@benjaminleonard
Copy link
Contributor

benjaminleonard commented Nov 26, 2024

Chatting through this with Ben, and he's noting that it would be good for us to consider a minItems: 1 on the API for tls_certificates, so this is protected via CLI as well.

Rather I meant that the minItems requirement is then documented in the API docs. Which we could probably also do with a comment.

@benjaminleonard
Copy link
Contributor

This is a simple one, worth squeezing into R12? @david-crespo @charliepark

@david-crespo
Copy link
Collaborator

Yeah, probably.

} = useController({
control,
name: 'tlsCertificates',
rules: { required: true },
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to work, but I was surprised about it since [] is truthy. Found this in the docs recommending validate over required for object and array values. I'll make that tweak now.

image

Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

This all looks good to me. I'm still a little concerned about requiring a field in the UI that the API does not actually require, but the fix for that is almost certainly to have the API 400 if the list of certs has less than one element.

This looks a little tight, could use some space @benjaminleonard ?

image

@benjaminleonard
Copy link
Contributor

Ship it, I feel like I want to rework that pattern where mini-table is hidden when there's no elements anyway, doesn't sit quite right.

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.

Uploading a TLS certificate should be mandatory during silo creation
3 participants