T11699: Create a job to automate CNAME checks #40
Conversation
redbluegreenhat
changed the title
redbluegreenhat
changed the title
Add use User; |
redbluegreenhat
changed the title
|
@Universal-Omega This PR does the CNAME check, but more importantly, lays out how I want to approach the checks, with a job. What do you think? Also, in the future, the hardcoded strings at RequestSSL/includes/DomainCheckJob.php Lines 30 to 35 in c1483e6 |
| @@ -144,6 +148,10 @@ | |||
| "value": "", | |||
| "description": "If set, only allow users to request a SSL certificate on this wiki." | |||
| }, | |||
| "RequestSSLDomainCheckCNAME": { | |||
There was a problem hiding this comment.
Personally I think we should use a single associative array config, RequestSSLDomainChecks or something then an option for like CNAME, A, AAAA, and NS keys for supported options for checking.
This job is in charge of figuring out if a domain is pointed. Takes only one parameter, the requestID for use with RequestSSLManager
This config can be used, for example, to support DNS providers like Cloudflare, that do CNAME flattening on the root domain.
it is an array of associative arrays. We only care about the first result by the way, since there should be only one result: the configured CNAME record we're looking for
rDNS will be done in a separate PR
Universal-Omega
force-pushed
the
T11699-introduce-domaincheckjob
branch
from
3e90b27 to
0e1a6f4
Compare
WalkthroughThis update introduces a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
includes/Jobs/DomainCheckJob.php (1)
28-37: Ensure proper documentation for constructor parameters.The constructor's PHPDoc block should include descriptions for each parameter to improve code readability and maintainability.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- extension.json (3 hunks)
- includes/Jobs/DomainCheckJob.php (1 hunks)
- includes/Specials/SpecialRequestSSL.php (5 hunks)
Additional comments not posted (2)
extension.json (1)
41-49: Validate service dependencies forDomainCheckJob.Ensure that the services listed under
DomainCheckJobare correctly configured and necessary for the job's functionality. This includes verifying thatConfigFactoryandRequestSSLManagerare used appropriately within the job.Verification successful
Verification of Service Dependencies for
DomainCheckJobThe
DomainCheckJobclass uses bothConfigFactoryandRequestSSLManagerin its constructor, confirming that these services are correctly configured and necessary for the job's functionality.Key Findings:
- Class Definition:
DomainCheckJobextendsJoband implementsGenericParameterJob.- Constructor: The constructor of
DomainCheckJobacceptsConfigFactoryandRequestSSLManageras parameters and uses them to initialize the class properties.Relevant Code Snippets:
Class Definition:
class DomainCheckJob extends Job implements GenericParameterJob { /** @var Config */ private $config; /** @var int */ private $requestID; /** @var RequestSSLManager */ private $requestSslManager;Constructor:
public function __construct( array $params, ConfigFactory $configFactory, RequestSSLManager $requestSslManager ) { parent::__construct('DomainCheckJob', $params); $this->requestID = $params['requestID']; $this->config = $configFactory->makeConfig('RequestSSL'); $this->requestSslManager = $requestSslManager; }Conclusion:
The services
ConfigFactoryandRequestSSLManagerare appropriately used within theDomainCheckJobclass, confirming their necessity and correct configuration.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check service usage in DomainCheckJob. ast-grep --lang php --pattern $'class DomainCheckJob { $$$ __construct($_, $_) { $$$ } $$$ }'Length of output: 215
Script:
#!/bin/bash # Description: Check for the constructor and service usage in DomainCheckJob. # Search for the DomainCheckJob class definition rg 'class DomainCheckJob' -A 10 # Search for the constructor within DomainCheckJob rg 'function __construct' -A 10 # Search for the usage of ConfigFactory and RequestSSLManager within DomainCheckJob rg 'ConfigFactory|RequestSSLManager' -A 10Length of output: 19879
includes/Specials/SpecialRequestSSL.php (1)
233-235: Confirm job queue integration.Verify that the
JobSpecificationforDomainCheckJobis correctly configured and that the job is being pushed to the job queue as expected. This is crucial for ensuring that the domain check functionality is operational.Verification successful
Confirm job queue integration
The
JobSpecificationforDomainCheckJobis correctly instantiated and pushed to the job queue usinglazyPushin theSpecialRequestSSLclass.
JobSpecificationinstantiation:$domainCheckJob = new JobSpecification( 'DomainCheckJob', ['requestID' => $requestID] );
lazyPushmethod call:$this->jobQueueGroup->lazyPush( $domainCheckJob );These lines confirm that the domain check functionality is operational as expected.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify job queue integration for DomainCheckJob. ast-grep --lang php --pattern $'class SpecialRequestSSL { $$$ onSubmit($_) { $$$ new JobSpecification("DomainCheckJob", $$$) $$$ } $$$ }'Length of output: 267
Script:
#!/bin/bash # Description: Verify job queue integration for DomainCheckJob using ripgrep. # Search for the instantiation of JobSpecification with 'DomainCheckJob' and the lazyPush method call within the SpecialRequestSSL class. rg 'new JobSpecification\("DomainCheckJob"' -A 5 --context-separator='' --glob '**/SpecialRequestSSL.php'Length of output: 105
Script:
#!/bin/bash # Description: Verify job queue integration for DomainCheckJob using ripgrep. # Search for all instances of JobSpecification and lazyPush within the SpecialRequestSSL file. rg 'JobSpecification' -A 5 --context-separator='' --glob '**/SpecialRequestSSL.php' rg 'lazyPush' -A 5 --context-separator='' --glob '**/SpecialRequestSSL.php'Length of output: 1268
| public function run() { | ||
| $this->requestSslManager->fromID( $this->requestID ); | ||
| $customDomain = parse_url( $this->requestSslManager->getCustomDomain(), PHP_URL_HOST ); | ||
| if ( !$customDomain ) { | ||
| // Custom domain does not have a hostname, bail out. | ||
| $this->setLastError( 'Custom domain does not have a hostname.' ); | ||
| return true; | ||
| } | ||
| $cname = $this->config->get( 'RequestSSLDomainCheckCNAME' ); | ||
| // TODO: Support rDNS and NS checks | ||
| // CNAME check | ||
| $dnsCNAMEData = dns_get_record( $customDomain, DNS_CNAME ); | ||
| if ( !$dnsCNAMEData ) { | ||
| $this->requestSslManager->addComment( 'RequestSSL could not determine whether or not this domain is pointed: DNS returned no data during CNAME check.', User::newSystemUser( 'RequestSSL Extension' ) ); | ||
| } else { | ||
| if ( $dnsCNAMEData[0]['type'] === 'CNAME' && $dnsCNAMEData[0]['target'] === $cname ) { | ||
| $this->requestSslManager->addComment( 'Domain is pointed via CNAME.', User::newSystemUser( 'RequestSSL Extension' ) ); | ||
| } else { | ||
| $this->requestSslManager->addComment( 'Domain is not pointed via CNAME. It is possible it is pointed via other means.', User::newSystemUser( 'RequestSSL Extension' ) ); | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Address potential issues with DNS lookup failure handling.
The method run checks for CNAME records but does not handle potential DNS lookup failures gracefully. Consider adding error handling for cases where dns_get_record might fail due to network issues or misconfigurations.
| $this->requestSslManager->addComment( 'RequestSSL could not determine whether or not this domain is pointed: DNS returned no data during CNAME check.', User::newSystemUser( 'RequestSSL Extension' ) ); | ||
| } else { | ||
| if ( $dnsCNAMEData[0]['type'] === 'CNAME' && $dnsCNAMEData[0]['target'] === $cname ) { | ||
| $this->requestSslManager->addComment( 'Domain is pointed via CNAME.', User::newSystemUser( 'RequestSSL Extension' ) ); | ||
| } else { | ||
| $this->requestSslManager->addComment( 'Domain is not pointed via CNAME. It is possible it is pointed via other means.', User::newSystemUser( 'RequestSSL Extension' ) ); | ||
| } |
There was a problem hiding this comment.
Refactor to improve readability and reduce duplication.
The method run contains repeated calls to User::newSystemUser('RequestSSL Extension'). Consider storing this user in a variable at the start of the method to avoid multiple instantiations and improve code clarity.
+ $systemUser = User::newSystemUser('RequestSSL Extension');
...
- User::newSystemUser('RequestSSL Extension')
+ $systemUserCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| $this->requestSslManager->addComment( 'RequestSSL could not determine whether or not this domain is pointed: DNS returned no data during CNAME check.', User::newSystemUser( 'RequestSSL Extension' ) ); | |
| } else { | |
| if ( $dnsCNAMEData[0]['type'] === 'CNAME' && $dnsCNAMEData[0]['target'] === $cname ) { | |
| $this->requestSslManager->addComment( 'Domain is pointed via CNAME.', User::newSystemUser( 'RequestSSL Extension' ) ); | |
| } else { | |
| $this->requestSslManager->addComment( 'Domain is not pointed via CNAME. It is possible it is pointed via other means.', User::newSystemUser( 'RequestSSL Extension' ) ); | |
| } | |
| $systemUser = User::newSystemUser('RequestSSL Extension'); | |
| $this->requestSslManager->addComment( 'RequestSSL could not determine whether or not this domain is pointed: DNS returned no data during CNAME check.', $systemUser ); | |
| } else { | |
| if ( $dnsCNAMEData[0]['type'] === 'CNAME' && $dnsCNAMEData[0]['target'] === $cname ) { | |
| $this->requestSslManager->addComment( 'Domain is pointed via CNAME.', $systemUser ); | |
| } else { | |
| $this->requestSslManager->addComment( 'Domain is not pointed via CNAME. It is possible it is pointed via other means.', $systemUser ); | |
| } |
This PR does part of T11699. It introduces a job that is queued automatically when requests are created, and it automatically checks that the custom domain requested has the correct CNAME record.
https://issue-tracker.miraheze.org/T11699
Summary by CodeRabbit
New Features
Improvements
Backend Enhancements
These updates aim to streamline domain management and enhance reliability in domain verification processes.