Skip to content

T11699: Create a job to automate CNAME checks #40

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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
bd09967
Introduce DomainCheckJob
redbluegreenhat Mar 2, 2024
6fc0103
fix use statement
redbluegreenhat Mar 2, 2024
56975c6
Introduce RequestSSLDomainCheckCNAME config
redbluegreenhat Mar 2, 2024
ef0bd57
fix run function
redbluegreenhat Mar 2, 2024
f153e0b
Create RequestSSLDomainCheckReverseDNSRegex config
redbluegreenhat Mar 3, 2024
1b68ce9
oops
redbluegreenhat Mar 3, 2024
82d6934
Queue DomainCheckJob on request creation; fix CreateWikiHookRunner's …
redbluegreenhat Mar 3, 2024
aeb4153
fix usage of JobQueueGroup
redbluegreenhat Mar 3, 2024
afad519
Do CNAME checks on DomainCheckJob
redbluegreenhat Mar 6, 2024
10a203f
Fix accessing $dnsCNAMEData
redbluegreenhat Mar 6, 2024
c26841b
oops
redbluegreenhat Mar 6, 2024
150df09
return true; and use User
redbluegreenhat Mar 9, 2024
353b6ba
fix call to the parent class' constructor
redbluegreenhat Mar 9, 2024
f177229
no need for this
redbluegreenhat Mar 9, 2024
97f230b
remove ReverseDNSRegex
redbluegreenhat Mar 9, 2024
dd2c70d
fix
redbluegreenhat Mar 9, 2024
0b45bee
Inject ConfigFactory and RequestSSLManager on DomainCheckJob
redbluegreenhat Mar 10, 2024
a6ee8f0
Move DomainCheckJob under Jobs\RequestSSL
redbluegreenhat Mar 10, 2024
407a0be
Inject services into RequestSSLManager
redbluegreenhat Mar 10, 2024
ddbf437
use namespaced User class
redbluegreenhat Mar 10, 2024
fb5a4c7
use setLastError
redbluegreenhat Mar 10, 2024
051415b
use JobSpecification
redbluegreenhat Mar 11, 2024
6090427
oops
redbluegreenhat Mar 11, 2024
163d280
Inject JobQueueGroup directly
redbluegreenhat Mar 12, 2024
c3c9502
oops
redbluegreenhat Mar 12, 2024
97f1142
fix JobQueueGroup's namespace
redbluegreenhat Mar 12, 2024
0e1a6f4
Drop Title parameter
redbluegreenhat Mar 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions extension.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@
"request-ssl",
"view-private-ssl-requests"
],
"JobClasses": {
"DomainCheckJob": {
"class": "Miraheze\\RequestSSL\\Jobs\\DomainCheckJob",
"services": [
"ConfigFactory",
"RequestSSLManager"
],
"needsPage": false
}
},
"LogActionsHandlers": {
"requestssl/*": "LogFormatter",
"requestsslprivate/*": "LogFormatter"
Expand Down Expand Up @@ -81,6 +91,7 @@
"services": [
"CreateWikiHookRunner",
"DBLoadBalancerFactory",
"JobQueueGroup",
"MimeAnalyzer",
"RepoGroup",
"UserFactory"
Expand Down Expand Up @@ -144,6 +155,10 @@
"value": "",
"description": "If set, only allow users to request a SSL certificate on this wiki."
},
"RequestSSLDomainCheckCNAME": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"value": "",
"description": "String. Specifies the CNAME record that RequestSSL will look for on automatic domain checks. If a domain has this CNAME record, it is considered to be pointed"
},
"RequestSSLHelpUrl": {
"value": "",
"description": "Full URL. If set, adds a help URL to Special:RequestSSL."
Expand Down
65 changes: 65 additions & 0 deletions includes/Jobs/DomainCheckJob.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php

namespace Miraheze\RequestSSL\Jobs;

use Config;
use ConfigFactory;
use GenericParameterJob;
use Job;
use Miraheze\RequestSSL\RequestSSLManager;
use MediaWiki\User\User;

class DomainCheckJob extends Job implements GenericParameterJob {

/** @var Config */
private $config;

/** @var int */
private $requestID;

/** @var RequestSSLManager */
private $requestSslManager;

/**
* @param array $params
* @param ConfigFactory $configFactory
* @param RequestSSLManager $requestSslManager
*/
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;
}

/**
* @return bool
*/
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' ) );
}
Comment on lines +55 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')
+ $systemUser

Committable 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.

Suggested change
$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 );
}

}
return true;
Comment on lines +42 to +63
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
11 changes: 11 additions & 0 deletions includes/Specials/SpecialRequestSSL.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
use ExtensionRegistry;
use FormSpecialPage;
use Html;
use JobSpecification;
use ManualLogEntry;
use JobQueueGroup;
use MediaWiki\User\UserFactory;
use Message;
use MimeAnalyzer;
Expand All @@ -30,6 +32,9 @@ class SpecialRequestSSL extends FormSpecialPage {
/** @var ILBFactory */
private $dbLoadBalancerFactory;

/** @var JobQueueGroup */
private $jobQueueGroup;

/** @var MimeAnalyzer */
private $mimeAnalyzer;

Expand All @@ -42,13 +47,15 @@ class SpecialRequestSSL extends FormSpecialPage {
/**
* @param CreateWikiHookRunner $createWikiHookRunner
* @param ILBFactory $dbLoadBalancerFactory
* @param JobQueueGroup $jobQueueGroup
* @param MimeAnalyzer $mimeAnalyzer
* @param RepoGroup $repoGroup
* @param UserFactory $userFactory
*/
public function __construct(
CreateWikiHookRunner $createWikiHookRunner,
ILBFactory $dbLoadBalancerFactory,
JobQueueGroup $jobQueueGroup,
MimeAnalyzer $mimeAnalyzer,
RepoGroup $repoGroup,
UserFactory $userFactory
Expand All @@ -57,6 +64,7 @@ public function __construct(

$this->createWikiHookRunner = $createWikiHookRunner;
$this->dbLoadBalancerFactory = $dbLoadBalancerFactory;
$this->jobQueueGroup = $jobQueueGroup;
$this->mimeAnalyzer = $mimeAnalyzer;
$this->repoGroup = $repoGroup;
$this->userFactory = $userFactory;
Expand Down Expand Up @@ -222,6 +230,9 @@ public function onSubmit( array $data ) {
$this->sendNotifications( $data['reason'], $this->getUser()->getName(), $requestID, $data['target'] );
}

$domainCheckJob = new JobSpecification( 'DomainCheckJob', ['requestID' => $requestID] );
$this->jobQueueGroup->lazyPush( $domainCheckJob );

return Status::newGood();
}

Expand Down
Loading