Page MenuHomePhabricator

SecurePoll doing DB writes to securepoll_voters on HTTP GET [Timebox: 8h]
Open, Needs TriagePublicPRODUCTION ERROR

Description

Following on from T283280: Special:SecurePoll makes primary database connections on GET requests when getting UI messages or checking for election admins

Error
normalized_message
Expectation (writes <=) 0 by MediaWiki::main not met (actual: {actual}):
{query}
exception.trace
from /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/TransactionProfiler.php(444)
#0 /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/TransactionProfiler.php(280): Wikimedia\Rdbms\TransactionProfiler->reportExpectationViolated(string, Wikimedia\Rdbms\GeneralizedSql, integer)
#1 /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/database/Database.php(1501): Wikimedia\Rdbms\TransactionProfiler->recordQueryCompletion(Wikimedia\Rdbms\GeneralizedSql, double, boolean, integer)
#2 /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/database/Database.php(1383): Wikimedia\Rdbms\Database->executeQueryAttempt(string, string, boolean, string, integer)
#3 /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/database/Database.php(1308): Wikimedia\Rdbms\Database->executeQuery(string, string, integer)
#4 /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/database/Database.php(2433): Wikimedia\Rdbms\Database->query(string, string, integer)
#5 /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/database/Database.php(2413): Wikimedia\Rdbms\Database->doInsert(string, array, string)
#6 /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/database/DBConnRef.php(68): Wikimedia\Rdbms\Database->insert(string, array, string)
#7 /srv/mediawiki/php-1.37.0-wmf.18/includes/libs/rdbms/database/DBConnRef.php(373): Wikimedia\Rdbms\DBConnRef->__call(string, array)
#8 /srv/mediawiki/php-1.37.0-wmf.18/extensions/SecurePoll/includes/User/Voter.php(116): Wikimedia\Rdbms\DBConnRef->insert(string, array, string)
#9 /srv/mediawiki/php-1.37.0-wmf.18/extensions/SecurePoll/includes/Context.php(222): MediaWiki\Extensions\SecurePoll\User\Voter::createVoter(MediaWiki\Extensions\SecurePoll\Context, array)
#10 /srv/mediawiki/php-1.37.0-wmf.18/extensions/SecurePoll/includes/User/Auth.php(143): MediaWiki\Extensions\SecurePoll\Context->createVoter(array)
#11 /srv/mediawiki/php-1.37.0-wmf.18/extensions/SecurePoll/includes/User/LocalAuth.php(37): MediaWiki\Extensions\SecurePoll\User\Auth->getVoter(array)
#12 /srv/mediawiki/php-1.37.0-wmf.18/extensions/SecurePoll/includes/User/Auth.php(156): MediaWiki\Extensions\SecurePoll\User\LocalAuth->autoLogin(MediaWiki\Extensions\SecurePoll\Entities\Election)
#13 /srv/mediawiki/php-1.37.0-wmf.18/extensions/SecurePoll/includes/Pages/VotePage.php(90): MediaWiki\Extensions\SecurePoll\User\Auth->newAutoSession(MediaWiki\Extensions\SecurePoll\Entities\Election)
#14 /srv/mediawiki/php-1.37.0-wmf.18/extensions/SecurePoll/includes/SpecialSecurePoll.php(70): MediaWiki\Extensions\SecurePoll\Pages\VotePage->execute(array)
#15 /srv/mediawiki/php-1.37.0-wmf.18/includes/specialpage/SpecialPage.php(646): MediaWiki\Extensions\SecurePoll\SpecialSecurePoll->execute(string)
#16 /srv/mediawiki/php-1.37.0-wmf.18/includes/specialpage/SpecialPageFactory.php(1365): SpecialPage->run(string)
#17 /srv/mediawiki/php-1.37.0-wmf.18/includes/MediaWiki.php(314): MediaWiki\SpecialPage\SpecialPageFactory->executePath(string, RequestContext)
#18 /srv/mediawiki/php-1.37.0-wmf.18/includes/MediaWiki.php(925): MediaWiki->performRequest()
#19 /srv/mediawiki/php-1.37.0-wmf.18/includes/MediaWiki.php(559): MediaWiki->main()
#20 /srv/mediawiki/php-1.37.0-wmf.18/index.php(53): MediaWiki->run()
#21 /srv/mediawiki/php-1.37.0-wmf.18/index.php(46): wfIndexMain()
#22 /srv/mediawiki/w/index.php(3): require(string)
#23 {main}

Query:

INSERT INTO `securepoll_voters` (voter_id,voter_election,voter_name,voter_type,voter_domain,voter_url,voter_properties) VALUES (NULL,'X')
Impact
Notes

Event Timeline

Niharika renamed this task from SecurePoll doing DB writes to securepoll_voters on HTTP GET to SecurePoll doing DB writes to securepoll_voters on HTTP GET [Timebox: 8h].Aug 18 2021, 4:43 PM

Following the stack trace above I found that
Auth.php::getVoter() is doing sort of create of not exist

if ( $row ) {
	$user = $this->context->newVoterFromRow( $row );
	}
       else {
          $user = $this->context->createVoter( $params );
}

The else block of the snippet about is what eventually does DB writes. in Voter::createVoter

I am not sure if we can change this behaviour as LocalAuth::autoLogin heavily depends on this and always assumes to bring back the user.

Change 715594 had a related patch set uploaded (by TsepoThoabala; author: TsepoThoabala):

[mediawiki/extensions/SecurePoll@master] SecurePoll doing DB writes to securepoll_voters on HTTP GET

https://gerrit.wikimedia.org/r/715594

Have been helping w/review for this and the current solution (make the write a job) makes the write asynchronous which makes anything synchronous susceptible to race conditions. For instance, if I try to vote but my voter row hasn't been created yet (for whatever reason), then the vote will fail. Solutions on top of this seem fragile, as other things downstream then all need a similar "stub out a voter" code snippet.

I don't have the entire context to be 100% confident that this is a good proposed solution, but what if instead of writing the voter as a job kicked off by the page get, you wrote the voter as part of voting? That would make creating a voter a synchronous function. As far as I can tell, that's the first "action" that actively depends on a voter existing. If a voter doesn't exist before then, it doesn't matter*.

*iirc creating the voter also sets the voter's language for the rest of the election so there might be some additional reads there if the voter doesn't immediately opt to vote?

A quick stub of where I'd look to make changes if I were to implement this solution.* Would love to get more eyes and ideas on this. This proposal is essentially "write the voter on POST." There's nothing stopping us from keeping the typical job solution (except all the downstream race conditions but we can choose to focus on fixing those instead).

diff --git a/includes/Pages/VotePage.php b/includes/Pages/VotePage.php
index 6a9d831..3e45e32 100644
--- a/includes/Pages/VotePage.php
+++ b/includes/Pages/VotePage.php
@@ -84,22 +84,32 @@ class VotePage extends ActionPage {
 
 		$this->auth = $this->election->getAuth();
 
-		// Get voter from session
-		$this->voter = $this->auth->getVoterFromSession( $this->election );
-
-		// If there's no session, try creating one.
-		// This will fail if the user is not authorized to vote in the election
-		if ( !$this->voter ) {
-			$status = $this->auth->newAutoSession( $this->election );
-			if ( $status->isOK() ) {
-				$this->voter = $status->value;
-			} else {
-				$out->addWikiTextAsInterface( $status->getWikiText() );
-
-				return;
-			}
-		}
-
+		// This chunk checks for a voter from getVoterFromSession which runs
+		// getVoter which will create a voter if one does not exist.
+		// If possible, remove this on-load voter dependency
+		// This assumes you've removed the createVoter portion of getVoter
+		// so it becomes possible for getVoterFromSession to return null because
+		// a voter isn't present in the database
+ 
+		// // Get voter from session
+		// $this->voter = $this->auth->getVoterFromSession( $this->election );
+
+		// // If there's no session, try creating one.
+		// // This will fail if the user is not authorized to vote in the election
+		// if ( !$this->voter ) {
+		// 	$status = $this->auth->newAutoSession( $this->election );
+		// 	if ( $status->isOK() ) {
+		// 		$this->voter = $status->value;
+		// 	} else {
+		// 		$out->addWikiTextAsInterface( $status->getWikiText() );
+
+		// 		return;
+		// 	}
+		// }
+
+		// Anywhere $this->voter isn't guaranteed, you're going to need to substitute the
+		// $user equivalent
+		// Maybe DefaultOptionsLookup->getDefaultOptions()?
 		$this->initLanguage( $this->voter, $this->election );
 		$language = $this->getUserLang();
 		$this->specialPage->getContext()->setLanguage( $language );
@@ -140,11 +150,13 @@ class VotePage extends ActionPage {
 		$out->disallowUserJs();
 
 		// Show welcome
+		// SecurePoll figures out isRemote()
 		if ( $this->voter->isRemote() ) {
 			$out->addWikiMsg( 'securepoll-welcome', $this->voter->getName() );
 		}
 
 		// Show change notice
+		// Could probably get away with just checking for $this->voter existing here
 		if ( $this->election->hasVoted( $this->voter ) && !$this->election->allowChange() ) {
 			$out->addWikiMsg( 'securepoll-change-disallowed' );
 
@@ -174,6 +186,7 @@ class VotePage extends ActionPage {
 		$out = $this->specialPage->getOutput();
 
 		// Show introduction
+		// same null check for $this->voter
 		if ( $this->election->hasVoted( $this->voter ) && $this->election->allowChange() ) {
 			$out->addWikiMsg( 'securepoll-change-allowed' );
 		}
@@ -242,12 +255,16 @@ class VotePage extends ActionPage {
 		$dbw = $this->loadBalancer->getConnectionRef( ILoadBalancer::DB_PRIMARY );
 		$dbw->startAtomic( __METHOD__ );
 
+		// Check for and create the voter here, getting the returned voter id
+		// either bc it exists or bc you just created it
+
 		// Mark previous votes as old
 		$dbw->update(
 			'securepoll_votes',
 			[ 'vote_current' => 0 ],
 			[
 				'vote_election' => $this->election->getId(),
+				// use that id here
 				'vote_voter' => $this->voter->getId(),
 			],
 			__METHOD__
@@ -268,6 +285,7 @@ class VotePage extends ActionPage {
 			[
 				'vote_id' => $voteId,
 				'vote_election' => $this->election->getId(),
+				// and here
 				'vote_voter' => $this->voter->getId(),
 				'vote_voter_name' => $this->voter->getName(),
 				'vote_voter_domain' => $this->voter->getDomain(),

Change 724456 had a related patch set uploaded (by TsepoThoabala; author: TsepoThoabala):

[mediawiki/extensions/SecurePoll@master] SecurePoll doing DB writes to securepoll_voters on HTTP GET

https://gerrit.wikimedia.org/r/724456

Change 715594 abandoned by TsepoThoabala:

[mediawiki/extensions/SecurePoll@master] SecurePoll doing DB writes to securepoll_voters on HTTP GET

Reason:

Messed up when trying to rebase, move here https://gerrit.wikimedia.org/r/c/mediawiki/extensions/SecurePoll/+/724456/

https://gerrit.wikimedia.org/r/715594

This is a really neat approach, which I think is reflected in the small size of the patch. I had thought that HTTP POSTing from the jump wiki to the voting wiki was a good alternative but, on reflection, I think this is superior as it puts the writes in the correct part of the request flow.

Moving to stalled, will be deployed after up coming elections(end of October 2021)
Needs to be tested on multiple wikis, one needs to set up a wiki farm to test this.

ARamirez_WMF subscribed.

In the past, AHT stepped in to support Trust & Safety with the time-sensitive matter of the Board Elections by providing help with SecurePoll. Unfortunately, at this time we can no longer support nor maintain SecurePoll. Per the Foundation leadership’s instructions, AHT is dedicating all of our time and energy to other critical efforts.

Change 724456 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] Remove DB writes to securepoll_voters on HTTP GET

https://gerrit.wikimedia.org/r/724456

Since we're now actively using the processes this task discusses for the Board elections, it seems like a great time to test if this works. I don't have the skills to do so but perhaps somehow who knows how to do this can?

Aklapper added a subscriber: TThoabala.

Removing inactive task assignee (please do so as part of offboarding steps).

Change 724456 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] Remove DB writes to securepoll_voters on HTTP GET

https://gerrit.wikimedia.org/r/724456

For the record, this commit was reverted on the same day that it was merged (18 Nov 2021)

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/SecurePoll/+/739642
Change by @phuedx:
Revert "Remove DB writes to securepoll_voters on HTTP GET"

Reason for revert: T288784#7482853

Change 724456 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] Remove DB writes to securepoll_voters on HTTP GET

https://gerrit.wikimedia.org/r/724456

For the record, this commit was reverted on the same day that it was merged (18 Nov 2021)

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/SecurePoll/+/739642
Change by @phuedx:
Revert "Remove DB writes to securepoll_voters on HTTP GET"

Reason for revert: T288784#7482853

Oh. :) I'm not sure I understand the reasoning for that revert if the code was otherwise sound?

@phuedx Would you be able to help shed some light on the above? In particular, is the revert due to any code issues or just a case of product ownership changes (i.e. could we just re-merge that patch again)?

@phuedx Would you be able to help shed some light on the above? In particular, is the revert due to any code issues or just a case of product ownership changes (i.e. could we just re-merge that patch again)?

IIRC it was just a case of product ownership changes and not having the bandwidth to thoroughly test the change after it was merged.