-
Notifications
You must be signed in to change notification settings - Fork 294
Add a BulkDomainTransferAction #2893
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
base: master
Are you sure you want to change the base?
Conversation
6bd349e to
4ea6b77
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @gbrodman)
core/src/main/java/google/registry/request/RequestParameters.java line 153 at r1 (raw file):
* @param name the name of the parameter (should be in plural form. e.g., tlds=, not tld=) */ public static ImmutableList<String> extractListOfParameters(HttpServletRequest req, String name) {
What's the max length of a query string parameter? Is this approach even suitable for, say, transferring tens of thousands of domain names? I think an action that takes a file upload of newline-delimited domain name strings is safer?
core/src/main/java/google/registry/request/RequestParameters.java line 171 at r1 (raw file):
return ImmutableList.of(); } return Splitter.on(',').splitToList(parameter).stream()
I'm not sure this is the proper way to do it. URLs have all sorts of escape characters to consider too (and not just for correctness, for security as well). Look into URLDecoder, e.g.: https://www.baeldung.com/java-url-encoding-decoding
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 76 at r1 (raw file):
private static final String SUPERUSER_TRANSFER_XML_FORMAT = """
This should be a separate .xml file that's loaded as a resource.
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 127 at r1 (raw file):
@Parameter("gainingRegistrarId") String gainingRegistrarId, @Parameter("losingRegistrarId") String losingRegistrarId, @Parameter("requestedByRegistrar") boolean requestedByRegistrar,
Are we thinking this should be true or false for BTAPPA? If true, then it would be true for nearly all cases except, what, shutting a registrar down and forcibly taking over their inventory? Or for one-off cases of abuse / court orders? (Those wouldn't necessarily be handled by this action, but they would always be false.)
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 194 at r1 (raw file):
gainingRegistrarId, ProtocolDefinition.getVisibleServiceExtensionUris()), new PasswordOnlyTransportCredentials(), EppRequestSource.TOOL,
Should this be TOOL, or BACKEND? It was BACKEND for RemoveAllDomainContactsAction, for whatever that's worth. Does it have to be TOOL to use the superuser extension?
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 231 at r1 (raw file):
} if (domain.getStatusValues().contains(StatusValue.PENDING_DELETE)) { logger.atWarning().log("Domain '%s' has status value PENDING_DELETE", domainName);
May as well also check the deletion time is END_OF_TIME as well.
4ea6b77 to
6bbfd80
Compare
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @CydeWeys)
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 76 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
This should be a separate .xml file that's loaded as a resource.
i think i don't agree
this allows us to have the replacement-variables defined in the same file where they're used, minimizing the chances of transcription errors where the name of one gets changed in one place but not another
it means we don't need to load a file every time the action is called
it avoids confusion when searching through the codebase by filename (there can be prod or test XML files with the same name)
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 127 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Are we thinking this should be true or false for BTAPPA? If true, then it would be true for nearly all cases except, what, shutting a registrar down and forcibly taking over their inventory? Or for one-off cases of abuse / court orders? (Those wouldn't necessarily be handled by this action, but they would always be false.)
True for BTAPPA.
Yes, this is for the purposes of court orders or forced inventory transfers. Even for one-off court orders, it'll be easier and safer to call the superuser-bulk-transfer CLI command than to do it manually.
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 194 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Should this be TOOL, or BACKEND? It was BACKEND for
RemoveAllDomainContactsAction, for whatever that's worth. Does it have to be TOOL to use the superuser extension?
Yes, it has to be TOOL.
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 231 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
May as well also check the deletion time is
END_OF_TIMEas well.
sure, shouldn't matter
core/src/main/java/google/registry/request/RequestParameters.java line 153 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
What's the max length of a query string parameter? Is this approach even suitable for, say, transferring tens of thousands of domain names? I think an action that takes a file upload of newline-delimited domain name strings is safer?
i'm not sure if you mean a POST body, but it's probably better to do a POST body anyway
core/src/main/java/google/registry/request/RequestParameters.java line 171 at r1 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
I'm not sure this is the proper way to do it. URLs have all sorts of escape characters to consider too (and not just for correctness, for security as well). Look into URLDecoder, e.g.: https://www.baeldung.com/java-url-encoding-decoding
eh i'm going to remove this in favor of just using the POST body
though it's worth noting that i basically just copied this from the existing code, so any issues you may have with it still exist there
This will be necessary if we wish to do larger BTAPPA transfers (or other types of transfers, I suppose). The nomulus command-line tool is not fast enough to quickly transfer thousands of domains within a reasonable timeframe.
6bbfd80 to
5e09a35
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @gbrodman)
core/src/main/java/google/registry/batch/BatchModule.java line 181 at r2 (raw file):
Gson gson, @OptionalJsonPayload Optional<JsonElement> optionalJsonElement) { return optionalJsonElement .map(je -> ImmutableList.copyOf(gson.fromJson(je, new TypeToken<List<String>>() {})))
What format is being sent here exactly?
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 66 at r2 (raw file):
* <p>Consider passing in an "maxQps" parameter based on the number of domains being transferred, * otherwise the default is {@link BatchModule#DEFAULT_MAX_QPS}. */
I think you're envisioning this command will be called by nomulus curl? If so, include some example code here of how to do so, especially wrt setting the POST body. Or will there be a separate nomulus command to do so, not included in this PR?
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)
core/src/main/java/google/registry/batch/BatchModule.java line 181 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
What format is being sent here exactly?
it's just a JSON list of strings
core/src/main/java/google/registry/batch/BulkDomainTransferAction.java line 66 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
I think you're envisioning this command will be called by
nomulus curl? If so, include some example code here of how to do so, especially wrt setting the POST body. Or will there be a separate nomulus command to do so, not included in this PR?
I was planning on creating a separate Nomulus tool command, though presumably there's no reason why we couldn't also call this via curl if we wanted.
It seemed weird to, in this PR, mention a command that doesn't exist yet so I was just going to update the documentation in a future PR.
This will be necessary if we wish to do larger BTAPPA transfers (or other types of transfers, I suppose). The nomulus command-line tool is not fast enough to quickly transfer thousands of domains within a reasonable timeframe.
This change is