Skip to content

Commit 0912108

Browse files
jenkins-botGerrit Code Review
jenkins-bot
authored and
Gerrit Code Review
committed
Merge "auth: Disable irrelevant account creation fields for temp users"
2 parents 6aa500c + d150c66 commit 0912108

File tree

9 files changed

+265
-9
lines changed

9 files changed

+265
-9
lines changed

‎includes/auth/AuthManager.php

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2524,7 +2524,13 @@ private function getAuthenticationRequestsInternal(
25242524
case self::ACTION_CREATE:
25252525
$reqs[] = new UsernameAuthenticationRequest;
25262526
$reqs[] = new UserDataAuthenticationRequest;
2527-
if ( $options['username'] !== null ) {
2527+
2528+
// Registered users should be prompted to provide a rationale for account creations,
2529+
// except for the case of a temporary user registering a full account (T328718).
2530+
if (
2531+
$options['username'] !== null &&
2532+
!$this->userNameUtils->isTemp( $options['username'] )
2533+
) {
25282534
$reqs[] = new CreationReasonAuthenticationRequest;
25292535
$options['username'] = null; // Don't fill in the username below
25302536
}

‎includes/auth/TemporaryPasswordPrimaryAuthenticationProvider.php

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,12 +114,17 @@ public function getAuthenticationRequests( $action, array $options ) {
114114
return [ TemporaryPasswordAuthenticationRequest::newRandom() ];
115115

116116
case AuthManager::ACTION_CREATE:
117-
if ( isset( $options['username'] ) && $this->emailEnabled ) {
118-
// Creating an account for someone else
117+
// Allow named users creating a new account to email a temporary password to a given address
118+
// in case they are creating an account for somebody else.
119+
// This isn't a likely scenario for account creations by anonymous or temporary users
120+
// and is therefore disabled for them (T328718).
121+
if (
122+
isset( $options['username'] ) &&
123+
!$this->userNameUtils->isTemp( $options['username'] ) &&
124+
$this->emailEnabled
125+
) {
119126
return [ TemporaryPasswordAuthenticationRequest::newRandom() ];
120127
} else {
121-
// It's not terribly likely that an anonymous user will
122-
// be creating an account for someone else.
123128
return [];
124129
}
125130

‎tests/phpunit/includes/auth/AuthManagerTest.php

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace MediaWiki\Tests\Auth;
44

5+
use Closure;
56
use DatabaseLogEntry;
67
use DomainException;
78
use DummySessionProvider;
@@ -25,8 +26,10 @@
2526
use MediaWiki\Auth\Hook\LocalUserCreatedHook;
2627
use MediaWiki\Auth\Hook\SecuritySensitiveOperationStatusHook;
2728
use MediaWiki\Auth\Hook\UserLoggedInHook;
29+
use MediaWiki\Auth\PasswordAuthenticationRequest;
2830
use MediaWiki\Auth\PrimaryAuthenticationProvider;
2931
use MediaWiki\Auth\RememberMeAuthenticationRequest;
32+
use MediaWiki\Auth\TemporaryPasswordAuthenticationRequest;
3033
use MediaWiki\Auth\UserDataAuthenticationRequest;
3134
use MediaWiki\Auth\UsernameAuthenticationRequest;
3235
use MediaWiki\Block\BlockManager;
@@ -4236,4 +4239,93 @@ public function testSetRequestContextUserFromSessionUser() {
42364239
$this->assertSame( $context->getRequest()->getSession()->getUser()->getName(), $newSessionUser->getName() );
42374240
$this->assertSame( $context->getRequest()->getSession()->getUser()->getName(), $context->getUser()->getName() );
42384241
}
4242+
4243+
/**
4244+
* @dataProvider provideAccountCreationAuthenticationRequestTestCases
4245+
*
4246+
* @param Closure $userProvider Closure returning the user performing the account creation
4247+
* @param string[] $expectedReqsByLevel Map of expected auth request classes keyed by requirement level
4248+
*/
4249+
public function testDefaultAccountCreationAuthenticationRequests(
4250+
Closure $userProvider,
4251+
array $expectedReqsByLevel
4252+
): void {
4253+
// Test the default primary and secondary authentication providers
4254+
// irrespective of any potentially conflicting local configuration.
4255+
$authConfig = $this->getServiceContainer()
4256+
->getConfigSchema()
4257+
->getDefaultFor( MainConfigNames::AuthManagerAutoConfig );
4258+
4259+
$this->overrideConfigValues( [
4260+
MainConfigNames::AuthManagerConfig => null,
4261+
MainConfigNames::AuthManagerAutoConfig => $authConfig
4262+
] );
4263+
4264+
$authManager = $this->getServiceContainer()->getAuthManager();
4265+
$userProvider = $userProvider->bindTo( $this );
4266+
4267+
$reqs = $authManager->getAuthenticationRequests( AuthManager::ACTION_CREATE, $userProvider() );
4268+
4269+
$reqsByLevel = [];
4270+
foreach ( $reqs as $req ) {
4271+
$reqsByLevel[$req->required][] = get_class( $req );
4272+
}
4273+
4274+
foreach ( $expectedReqsByLevel as $level => $expectedReqs ) {
4275+
$reqs = $reqsByLevel[$level] ?? [];
4276+
sort( $reqs );
4277+
sort( $expectedReqs );
4278+
4279+
$this->assertSame( $expectedReqs, $reqs );
4280+
}
4281+
}
4282+
4283+
public static function provideAccountCreationAuthenticationRequestTestCases(): iterable {
4284+
// phpcs:disable Squiz.Scope.StaticThisUsage.Found
4285+
yield 'account creation on behalf of anonymous user' => [
4286+
fn (): User => $this->getServiceContainer()->getUserFactory()->newAnonymous( '127.0.0.1' ),
4287+
[
4288+
AuthenticationRequest::OPTIONAL => [],
4289+
AuthenticationRequest::REQUIRED => [
4290+
UserDataAuthenticationRequest::class,
4291+
UsernameAuthenticationRequest::class
4292+
],
4293+
AuthenticationRequest::PRIMARY_REQUIRED => [ PasswordAuthenticationRequest::class ]
4294+
]
4295+
];
4296+
4297+
yield 'account creation on behalf of temporary user' => [
4298+
function (): User {
4299+
$req = new FauxRequest();
4300+
return $this->getServiceContainer()
4301+
->getTempUserCreator()
4302+
->create( null, $req )
4303+
->getUser();
4304+
},
4305+
[
4306+
AuthenticationRequest::OPTIONAL => [],
4307+
AuthenticationRequest::REQUIRED => [
4308+
UserDataAuthenticationRequest::class,
4309+
UsernameAuthenticationRequest::class
4310+
],
4311+
AuthenticationRequest::PRIMARY_REQUIRED => [ PasswordAuthenticationRequest::class ]
4312+
]
4313+
];
4314+
4315+
yield 'account creation on behalf of registered user' => [
4316+
fn (): User => $this->getTestUser()->getUser(),
4317+
[
4318+
AuthenticationRequest::OPTIONAL => [ CreationReasonAuthenticationRequest::class ],
4319+
AuthenticationRequest::REQUIRED => [
4320+
UserDataAuthenticationRequest::class,
4321+
UsernameAuthenticationRequest::class
4322+
],
4323+
AuthenticationRequest::PRIMARY_REQUIRED => [
4324+
PasswordAuthenticationRequest::class,
4325+
TemporaryPasswordAuthenticationRequest::class
4326+
]
4327+
]
4328+
];
4329+
// phpcs:enable
4330+
}
42394331
}

‎tests/phpunit/includes/auth/TemporaryPasswordPrimaryAuthenticationProviderTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ public static function provideGetAuthenticationRequests(): iterable {
340340
];
341341

342342
yield 'signup attempt as temporary user' => [
343-
AuthManager::ACTION_CREATE, true, true, [ new TemporaryPasswordAuthenticationRequest( 'random' ) ]
343+
AuthManager::ACTION_CREATE, true, true, []
344344
];
345345

346346
yield 'account linking attempt as anonymous user' => [

‎tests/phpunit/includes/specials/SpecialCreateAccountTest.php

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
11
<?php
22

3+
use HtmlFormatter\HtmlFormatter;
34
use MediaWiki\Auth\AbstractPreAuthenticationProvider;
45
use MediaWiki\Auth\AuthenticationRequest;
6+
use MediaWiki\Context\DerivativeContext;
7+
use MediaWiki\Context\IContextSource;
58
use MediaWiki\Context\RequestContext;
69
use MediaWiki\MainConfigNames;
10+
use MediaWiki\Request\FauxRequest;
711
use MediaWiki\Specials\SpecialCreateAccount;
812

913
/**
@@ -14,13 +18,14 @@ class SpecialCreateAccountTest extends SpecialPageTestBase {
1418
/**
1519
* @inheritDoc
1620
*/
17-
protected function newSpecialPage() {
21+
protected function newSpecialPage( ?IContextSource $context = null ) {
1822
$services = $this->getServiceContainer();
19-
$context = RequestContext::getMain();
23+
$context ??= RequestContext::getMain();
2024
$page = new SpecialCreateAccount(
2125
$services->getAuthManager(),
2226
$services->getFormatterFactory()
2327
);
28+
$page->setContext( $context );
2429
$context->setTitle( $page->getPageTitle() );
2530
return $page;
2631
}
@@ -51,6 +56,54 @@ public function testHiddenField() {
5156
$html
5257
);
5358
}
59+
60+
public function testShouldShowTemporaryPasswordAndCreationReasonFieldsForRegisteredUser(): void {
61+
$user = $this->getTestUser()->getUser();
62+
63+
$context = new DerivativeContext( RequestContext::getMain() );
64+
$context->setUser( $user );
65+
66+
$specialPage = $this->newSpecialPage( $context );
67+
$specialPage->execute( null );
68+
$doc = self::getOutputHtml( $specialPage );
69+
70+
$this->assertNotNull( $doc->getElementById( 'wpReason' ) );
71+
$this->assertNotNull( $doc->getElementById( 'wpCreateaccountMail' ) );
72+
}
73+
74+
public function testShouldNotShowTemporaryPasswordAndCreationReasonFieldsForTempUser(): void {
75+
$req = new FauxRequest();
76+
$tempUser = $this->getServiceContainer()
77+
->getTempUserCreator()
78+
->create( null, $req )
79+
->getUser();
80+
81+
$context = new DerivativeContext( RequestContext::getMain() );
82+
$context->setUser( $tempUser );
83+
84+
$specialPage = $this->newSpecialPage( $context );
85+
$specialPage->execute( null );
86+
$doc = self::getOutputHtml( $specialPage );
87+
88+
$this->assertNull(
89+
$doc->getElementById( 'wpReason' ),
90+
'Temporary users should not have to provide a reason for their account creation (T328718)'
91+
);
92+
$this->assertNull(
93+
$doc->getElementById( 'wpCreateaccountMail' ),
94+
'Temporary users should not have the option to have a temporary password sent on signup (T328718)'
95+
);
96+
}
97+
98+
/**
99+
* Convenience function to get the parsed DOM of the HTML generated by the given special page.
100+
* @param SpecialPage $page
101+
* @return DOMDocument
102+
*/
103+
private static function getOutputHtml( SpecialPage $page ): DOMDocument {
104+
$html = HtmlFormatter::wrapHTML( $page->getOutput()->getHTML() );
105+
return ( new HtmlFormatter( $html ) )->getDoc();
106+
}
54107
}
55108

56109
class MockAuthRequestWithHiddenField extends AuthenticationRequest {

‎tests/selenium/pageobjects/edit.page.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ class EditPage extends Page {
2828
return $( '#wpPreview' );
2929
}
3030

31+
get tempUserSignUpButton() {
32+
return $( '.mw-temp-user-banner-buttons > #pt-createaccount' );
33+
}
34+
3135
async openForEditing( title ) {
3236
await super.openTitle( title, { action: 'submit', vehidebetadialog: 1, hidewelcomedialog: 1 } );
3337
// Compatibility with CodeMirror extension (T324879)
@@ -55,6 +59,16 @@ class EditPage extends Page {
5559
await this.content.setValue( content );
5660
await this.save.click();
5761
}
62+
63+
/**
64+
* Navigate to Special:CreateAccount via the banner links if logged in as a temporary user.
65+
*
66+
* @return {Promise<void>}
67+
*/
68+
async openCreateAccountPageAsTempUser() {
69+
await this.tempUserSignUpButton.waitForDisplayed();
70+
await this.tempUserSignUpButton.click();
71+
}
5872
}
5973

6074
module.exports = new EditPage();

‎tests/selenium/specs/user.js

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
const assert = require( 'assert' );
77
const CreateAccountPage = require( 'wdio-mediawiki/CreateAccountPage' );
8+
const EditPage = require( '../pageobjects/edit.page' );
89
const UserLoginPage = require( 'wdio-mediawiki/LoginPage' );
910
const Api = require( 'wdio-mediawiki/Api' );
1011
const Util = require( 'wdio-mediawiki/Util' );
@@ -41,4 +42,56 @@ describe( 'User', () => {
4142
const actualUsername = await browser.execute( () => mw.config.get( 'wgUserName' ) );
4243
assert.strictEqual( await actualUsername, username );
4344
} );
45+
46+
it( 'named user should see extra signup form fields when creating an account', async () => {
47+
await Api.createAccount( bot, username, password );
48+
await UserLoginPage.login( username, password );
49+
50+
await CreateAccountPage.open();
51+
52+
assert.ok( await CreateAccountPage.username.isExisting() );
53+
assert.ok( await CreateAccountPage.password.isExisting() );
54+
assert.ok(
55+
await CreateAccountPage.tempPasswordInput.isExisting(),
56+
'Named users should have the option to have a temporary password sent on signup (T328718)'
57+
);
58+
assert.ok(
59+
await CreateAccountPage.reasonInput.isExisting(),
60+
'Named users should have to provide a reason for their account creation (T328718)'
61+
);
62+
} );
63+
64+
it( 'temporary user should not see signup form fields relevant to named users', async () => {
65+
const pageTitle = Util.getTestString( 'TempUserSignup-TestPage-' );
66+
const pageText = Util.getTestString();
67+
68+
await EditPage.edit( pageTitle, pageText );
69+
await EditPage.openCreateAccountPageAsTempUser();
70+
71+
assert.ok( await CreateAccountPage.username.isExisting() );
72+
assert.ok( await CreateAccountPage.password.isExisting() );
73+
assert.ok(
74+
!await CreateAccountPage.tempPasswordInput.isExisting(),
75+
'Temporary users should not have the option to have a temporary password sent on signup (T328718)'
76+
);
77+
assert.ok(
78+
!await CreateAccountPage.reasonInput.isExisting(),
79+
'Temporary users should not have to provide a reason for their account creation (T328718)'
80+
);
81+
} );
82+
83+
// NOTE: This test can't run parallel with other account creation tests (T199393)
84+
it( 'temporary user should be able to create account', async () => {
85+
const pageTitle = Util.getTestString( 'TempUserSignup-TestPage-' );
86+
const pageText = Util.getTestString();
87+
88+
await EditPage.edit( pageTitle, pageText );
89+
await EditPage.openCreateAccountPageAsTempUser();
90+
91+
await CreateAccountPage.submitForm( username, password );
92+
93+
const actualUsername = browser.execute( () => mw.config.get( 'wgUserName' ) );
94+
assert.strictEqual( await actualUsername, username );
95+
assert.strictEqual( await CreateAccountPage.heading.getText(), `Welcome, ${ username }!` );
96+
} );
4497
} );

‎tests/selenium/wdio-mediawiki/CreateAccountPage.js

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict';
22

33
const Page = require( './Page' );
4+
const Util = require( 'wdio-mediawiki/Util' );
45

56
class CreateAccountPage extends Page {
67
get username() {
@@ -23,12 +24,41 @@ class CreateAccountPage extends Page {
2324
return $( '#firstHeading' );
2425
}
2526

27+
get tempPasswordInput() {
28+
return $( '#wpCreateaccountMail' );
29+
}
30+
31+
get reasonInput() {
32+
return $( '#wpReason' );
33+
}
34+
2635
open() {
2736
super.openTitle( 'Special:CreateAccount' );
2837
}
2938

39+
/**
40+
* Navigate to Special:CreateAccount, then fill out and submit the account creation form.
41+
*
42+
* @param {string} username
43+
* @param {string} password
44+
* @return {Promise<void>}
45+
*/
3046
async createAccount( username, password ) {
3147
await this.open();
48+
await this.submitForm( username, password );
49+
}
50+
51+
/**
52+
* Fill out and submit the account creation form on Special:CreateAccount.
53+
* The browser is assumed to have already navigated to this page.
54+
*
55+
* @param {string} username
56+
* @param {string} password
57+
* @return {Promise<void>}
58+
*/
59+
async submitForm( username, password ) {
60+
await Util.waitForModuleState( 'mediawiki.special.createaccount', 'ready', 10000 );
61+
3262
await this.username.setValue( username );
3363
await this.password.setValue( password );
3464
await this.confirmPassword.setValue( password );

‎tests/selenium/wdio-mediawiki/wdio-defaults.conf.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,10 @@ exports.config = {
6666
'--enable-automation',
6767
...( process.env.DISPLAY ? [] : [ '--headless' ] ),
6868
// Chrome sandbox does not work in Docker
69-
...( fs.existsSync( '/.dockerenv' ) ? [ '--no-sandbox' ] : [] )
69+
...( fs.existsSync( '/.dockerenv' ) ? [ '--no-sandbox' ] : [] ),
70+
// Workaround inputs not working consistently post-navigation on Chrome 90
71+
// https://issuetracker.google.com/issues/42322798
72+
'--allow-pre-commit-input'
7073
]
7174
}
7275
} ],

0 commit comments

Comments
 (0)