Conversation
This commit only consists of renaming files and moving some functions.
models/policy.go
Outdated
| return oldPolicy.Email != p.Email | ||
| } | ||
| // If old policy is manual and in testing, we can update it safely. | ||
| if !oldPolicy.MTASTS && oldPolicy.Policy.Mode == "testing" { |
There was a problem hiding this comment.
If new policy enforce and manual, don't upgrade either
models/policy_test.go
Outdated
|
|
||
| import ( | ||
| "errors" | ||
| // "strings" |
| @@ -30,6 +30,31 @@ type List struct { | |||
| Policies map[string]TLSPolicy `json:"policies"` | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
note: comment above TLSPolicy.Mode that it can only be one of none enforce testing
vbrown608
left a comment
vbrown608
left a comment
There was a problem hiding this comment.
This is great! I really like PolicySubmission - its role is much clearer than Domain. I also like the separation of pending vs. accepted and testing vs. enforce vs. none. That seems like clear up so much confusion from Domain.Status. The abstraction on top of the two policy tables is cool :D
api.go
Outdated
| // and prefers the latter if both are present. | ||
| type API struct { | ||
| Database db.Database | ||
| Database *db.SQLDatabase |
| func getDomainParams(r *http.Request) (models.Domain, error) { | ||
| // Extracts relevant parameters from http.Request for a POST to /api/queue into PolicySubmission | ||
| // If MTASTS is set, doesn't try to extract hostnames. Otherwise, expects between 1 and MaxHostnames | ||
| // valid hostnames to be given in |r|. |
| RemovePolicy(string) (PolicySubmission, error) | ||
| } | ||
|
|
||
| type policyList interface { |
There was a problem hiding this comment.
A comment distinguishing this from policyStore would be helpful
models/policy.go
Outdated
| } | ||
|
|
||
| // CanUpdate returns whether we can update the policyStore with this particular | ||
| // Policy Submission. In some cases, you should be able to update the |
There was a problem hiding this comment.
// CanUpdate returns whether we can update the policyStore with this particular
// Policy Submission. Domains that have already been added to the policy store
// can only:
// 1. Have their their contact e-mail address updated
// 2. Have their policy updated if they're manual and in testing.
| return fmt.Sprintf(query, p.tableName, "domain, email, mta_sts, mxs, mode") | ||
| } | ||
|
|
||
| type scanner interface { |
There was a problem hiding this comment.
Can we use https://golang.org/pkg/database/sql/#Scanner?
There was a problem hiding this comment.
Not quite, I think-- It looks like Scanner expects a single interface{} destination, whereas here we are scanning into a varied number: ...interface{}
db/policy.go
Outdated
| } | ||
|
|
||
| // GetPolicy returns the policy submission for the given domain. | ||
| func (p *PolicyDB) GetPolicy(domainName string) (models.PolicySubmission, bool, error) { |
There was a problem hiding this comment.
Named return values might be nice here, since the meaning of bool isn't immediately obvious.
| // PutOrUpdatePolicy upserts the given policy into the data store, if | ||
| // CanUpdate passes. | ||
| func (p *PolicyDB) PutOrUpdatePolicy(ps *models.PolicySubmission) error { | ||
| if p.strict && !ps.CanUpdate(p) { |
There was a problem hiding this comment.
I like these guards - this seems like a good place for this type of validation. It seems like we're never running this code with strict = true. Is there somewhere we should be?
models/policy.go
Outdated
| "Please use the STARTTLS checker to scan your domain's " + | ||
| "STARTTLS configuration so we can validate your submission" | ||
| } | ||
| if scan.Timestamp.Add(time.Minute * 10).Before(time.Now()) { |
There was a problem hiding this comment.
I don't have much UX experience around this process but 10 minutes seems a bit low to me - I could imagine a user stopping on the add-domain form to do some research before submitting. Maybe an hour?
models/token.go
Outdated
| store.RemoveDomain(domainData.Name, domainOnList.State) | ||
| } | ||
| err = store.SetStatus(domainData.Name, StateTesting) | ||
| _, err = pending.RemovePolicy(domain) |
There was a problem hiding this comment.
If store.PutOrUpdatePolicy and pending.RemovePolicy always happen together maybe we should wrap them in a single action on Policy so we can make sure they always both occur.
|
If this generally looks OK to you, I think there are two more things to do before landing this:
Let me know if you have any more comments on the meat of the code! I'll work on the above two concurrently, but they should be relatively easy to review. |
|
LGTM, ready for merge pending migration script |
2 participants
Tested, complete version of #228.
Sorry for the massive PR; been working on-and-off on testing the move for a while. Reorganized commits this week, so they should be nicely organized and easier to review individually.
One final TODO item is to migrate old DB into the new one.