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

feat: Certificate Requests Table Query #21

Merged
merged 8 commits into from
Jun 20, 2024
Merged

Conversation

kayra1
Copy link
Contributor

@kayra1 kayra1 commented Jun 13, 2024

Description

e

This PR introduces the query required to get all of the CSR's to dynamically create the table. The frontend will show a very simple loading text when the query fires, a very simple error text when the query fails, and the expected table when the query succeeds. The table is loaded with an array of CSR objects, which the table will pass on to the rows.

There is also the code to ingest the CSR pem string into a native object using pkijs, but these functions are incomplete and only return the Common Name and the NotAfter field for now. I've scoped the task to split these out to different tickets named below.

There are some usability improvements that could also be done here, and I've scoped them out into a different ticket too.

Q: Where are the query unit tests?
A: neither vitest nor react-query provide facilities to mock queries. Tests for queries will be done in the integration tests that are scoped out in TLSENG-263

Testing

You may have noticed that there are no tests associated with this PR. This is because there are no react state changes introduced with this PR (no modal popups etc.) and react-query does not have unit tests. Any external query tests that need to be written should be through the integration tests, which will come with TLSENG-263

Since the query is written so that it's made to the same endpoint as the server (and to avoid cors errors etc.), the only way to test it is to build it and run gocert with it:

npm run build --prefix ui && go install ./...
gocert -config config.yaml

npm run dev will not work.
If you run into TLS errors, you will need to accept the TLS certificate that gocert sends before the query starts working properly.

This PR does NOT include:

extracting the values of the CSR into the expanded row: That's TLSENG-276
making it look nicer: That's also TLSENG-276. I also added some recommendations there like making the bg color of certificates that are expired red etc.
Any way to add CSR's from within the frontend: That's TLSENG-265

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that validate the behaviour of the software
  • I validated that new and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@kayra1 kayra1 force-pushed the TLSENG-265-certificate-table branch from afe7017 to 352678d Compare June 13, 2024 10:05
@kayra1 kayra1 marked this pull request as ready for review June 13, 2024 10:15
@kayra1 kayra1 requested a review from a team as a code owner June 13, 2024 10:15
@kayra1 kayra1 force-pushed the TLSENG-265-certificate-table branch 2 times, most recently from 7782bc3 to c8b903e Compare June 13, 2024 11:57
Copy link
Contributor

@ghislainbourgeois ghislainbourgeois left a comment

Choose a reason for hiding this comment

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

LGTM, I added a small comments, but it could also be done in another PR.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@kayra1 kayra1 requested a review from gruyaume June 14, 2024 16:09
Copy link
Collaborator

@gruyaume gruyaume left a comment

Choose a reason for hiding this comment

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

Looks good to me besides the two comments

.github/workflows/build-rock.yaml Outdated Show resolved Hide resolved
ui/src/app/certificate_requests/page.tsx Outdated Show resolved Hide resolved
@kayra1 kayra1 enabled auto-merge (squash) June 20, 2024 10:13
@kayra1 kayra1 merged commit 84ff608 into main Jun 20, 2024
11 checks passed
@kayra1 kayra1 deleted the TLSENG-265-certificate-table branch June 20, 2024 10:18
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.

3 participants