[feature] Fetch + create domain permissions from subscriptions nightly #3635
Conversation
|
|
||
| // Check for invalid characters | ||
| // after the punification process. | ||
| if strings.ContainsAny(domain, "*, \n") { |
There was a problem hiding this comment.
i don't think this fully checks whether this is a valid domain or not. i think it might be worth replacing both util.Punify() and this check with, idna.Lookup.ToASCII(...) which should both convert to punycode and validate it too. it might even remove the need for dns.IsDomainName()
There was a problem hiding this comment.
tbh we might just be able replace the util.Punify() definition to instead be idna.Lookup.ToASCII(...) so it does validation too.
There was a problem hiding this comment.
the currently idna profile we use doesn't do much validation at all:
// Punycode is a Profile that does raw punycode processing with a minimum
// of validation.
Punycode *Profile = punycode
// Lookup is the recommended profile for looking up domain names, according
// to Section 5 of RFC 5891. The exact configuration of this profile may
// change over time.
Lookup *Profile = lookupThere was a problem hiding this comment.
Mmm I see what you mean. Would it be alright with you if we did this in a separate PR? There's a few places in the codebase where we could probably use better domain validation, and I'd rather switch everything over at once.
| // Allow fallback in case target doesn't | ||
| // negotiate content type correctly. | ||
| req.Header.Add("Accept-Charset", "utf-8") | ||
| req.Header.Add("Accept", permSub.ContentType.String()+","+"*/*") |
There was a problem hiding this comment.
i'm just thinking, instead of us storing an expected content-type and sticking to that, would it be worth just parsing the response dependent on returned content-type and we just have a series of expected content-types that we support parsing? or is there another reason for storing the content-type i'm not seeing?
There was a problem hiding this comment.
otherwise at the moment we're quite rigid as to whether a subscription remains as a certain content-type or to whether we configure something as the correct content-type to begin with. when we could just support any of them concurrently just by removing that field, always sending the full list we support in "Accept: " and just having the response struct defined in this file return the set content-type.
There was a problem hiding this comment.
sorry i didn't pick this up in one of the earlier PRs where you defined the gtsmodel.DomainPermissionSubscription{} type, it just didn't really twig in my brain until i saw it in action :p
There was a problem hiding this comment.
tbh this is a relatively larger change so feel free to make this as a PR afterwards
There was a problem hiding this comment.
Ah, the reasoning was that some platforms where you can host things (github intself, for example) only provide the content type "text/plain" even when serving a csv or json file. So I wanted to make things a bit easier for people hosting files, where they don't need to mess about ensuring that their webserver or hosting service serves the right content type. But indeed if we should change this I'd rather address it in a separate PR :)
There was a problem hiding this comment.
oh github you bastard... in which case it seems we better leave it then :)
|
some comments, but looking real good otherwise 👀 |
tsmethurst
deleted the
domain_permission_subscriptions_2_the_subscriptioning
branch
Description
This pull request adds scheduling of domain permission creation from subscriptions, as well as functions to support that (dereference functions, functionality to let admins test domain permission subscriptions by doing a dummy fetch).
Closes #3
To make this work, I had to do some refactoring to extract the admin actions struct from the admin processor and put it in its own separate package, hence the many change lines.
The PR doesn't include logic for revoking permissions yet from a subscription, because this PR felt hefty enough without including that. I'll do that in a separate PR. The logic for that will not be very complicated.
Checklist
Please put an x inside each checkbox to indicate that you've read and followed it:
[ ]->[x]If this is a documentation change, only the first checkbox must be filled (you can delete the others if you want).
go fmt ./...andgolangci-lint run.