search

tech5usa / TLSential

A server for providing short-lived TLS certificates to all services within a firewall restricted network.
GNU General Public License v3.0
15 stars 2 forks source link

Add email validation/parsing on incoming NewCertificate requests. #30

Closed d1str0 closed 4 years ago

d1str0 commented 4 years ago

This PR addresses the following issues: #15

Context

This PR addresses the lack of validation of user input on email addresses for Certificates.

Approach

We use net/mail to ParseAddress(), check for errors, and if no errors arise, return a valid certificate. https://golang.org/pkg/net/mail/#ParseAddress

Testing

No tests added, and no manual testing. Another PR is currently addressing Unit tests for Certificate but that change is not included in this PR. :(

Misc.

Still need to validate domains submitted and also refactor this app logic out of the model and into the service.

d1str0 commented 4 years ago

@mklepp @skoganti-iws @gtoveyiws

This PR uses a template. The template can be found here: https://github.com/ImageWare/TLSential/blob/develop/.github/pull_request_template.md

Here is the template:

This PR addresses the following issues:

Context

In one or two sentences, what problem is this PR trying to solve?

Approach

Briefly, how does this PR solve the issue?

Testing

What was your strategy for testing the new code?

Misc.

Screenshots? Loose ends? Concerns?

debus commented 4 years ago

Btw I really like the template for PRs. I think it will really help to easily communicate what each PR is doing and why. As well as force people to consider all aspects of the PR before submitting (e.g. If the brief description gets too long they may consider breaking it into smaller chunks. If they totally blank out at the Testing Strategy they may remember unit tests are a thing lol).

brooks42 commented 4 years ago

FWIW I actually prefer to go a step further and name branches [dev initials]/[Jira ticket]-[short description], for example cb/gi-994-enroll-text-fix. Then in PR titles I specifically place affected Jira tickets in the name so we can more easily search for PRs that changed the code for related issues. Just an extra facet to consider.

debus commented 4 years ago

FWIW I actually prefer to go a step further and name branches [dev initials]/[Jira ticket]-[short description], for example cb/gi-994-enroll-text-fix. Then in PR titles I specifically place affected Jira tickets in the name so we can more easily search for PRs that changed the code for related issues. Just an extra facet to consider.

Although in this Repo's case we'd want to reference Github Issues instead of JIRA

d1str0 commented 4 years ago

Branch names and PR titles can't really be templated or enforced. Although I don't disagree with adding some more standards to branch names and titles, as well as commit names, etc.

(that and tagging all versions)

brooks42 commented 4 years ago

FWIW I actually prefer to go a step further and name branches [dev initials]/[Jira ticket]-[short description], for example cb/gi-994-enroll-text-fix. Then in PR titles I specifically place affected Jira tickets in the name so we can more easily search for PRs that changed the code for related issues. Just an extra facet to consider.

Although in this Repo's case we'd want to reference Github Issues instead of JIRA

Right :P