Refuse single sign-on into an account the roster forgot

README.md

@@ -72,8 +72,10 @@ matches is provisioned exactly like a code login. For an account that is already
 address checked is the one recorded when they were admitted, so removing their entry ends this route
 too. Accounts that are not members are exempt, so staff signing in through the identity provider are
 unaffected; when such a login uses an address the allowlist would not admit, it is written to the log
-channel. A refusal is final: no other handler of the same hook can hand the login back. Without
-PluggableAuth the check never runs.
+channel. An account that carries the reader group without being on the roster is no staff account
+but a forgotten member account — a removed member's parked account, or one left behind by a failed
+provisioning — and is refused rather than exempted. A refusal is final: no other handler of the same
+hook can hand the login back. Without PluggableAuth the check never runs.
 
 ### Deactivation and removal
 
@@ -89,9 +91,11 @@ member out by itself, because it runs out or is only partial.
 
 Removing a member makes the roster forget them and renames their account to
 `Removed member <userId>`, so their address is free again and reaches a new account at the next
-login. The rename ends the account's open sessions, but not the member's admission: the allowlist
-entry that admits them stays, and a deactivation block stays behind on the renamed account rather
-than reaching the new one.
+code login. The rename ends the account's open sessions, but not the member's admission: the
+allowlist entry that admits them stays, and a deactivation block stays behind on the renamed
+account rather than reaching the new one. An identity provider that recorded the account still
+points at the parked one, so a removed member's single sign-on logins arrive there and are refused
+rather than reaching a fresh account.
 
 ### Rate limits and logging
 

src/EntryPoints/Auth/SsoAuthorizationHandler.php

@@ -6,6 +6,7 @@
 
 use MediaWiki\Auth\AuthManager;
 use MediaWiki\User\User;
+use MediaWiki\User\UserGroupManager;
 use MediaWiki\User\UserIdentity;
 use ProfessionalWiki\MemberAccess\Application\AllowlistMatcher;
 use ProfessionalWiki\MemberAccess\Application\MemberGroup;
@@ -26,15 +27,23 @@
  * Accounts that are no members are exempt: staff who prefer to sign in through the identity
  * provider were never admitted by the allowlist and are not meant to be on the member list. Every
  * such login of an address the allowlist would not admit is recorded.
+ *
+ * The exemption does not extend to an account that carries the reader group without a roster row.
+ * The group is the mark of an account the allowlist created — provisioning adds it before the
+ * roster row, and a removal leaves it on the parked account — so such an account is one the roster
+ * forgot: admitting it as staff would put it outside the allowlist for good, on a route whose
+ * identity provider can keep handing logins back to it.
  */
 class SsoAuthorizationHandler {
 
 	public function __construct(
 		private readonly bool $allowlistApplies,
 		private readonly AllowlistMatcher $matcher,
 		private readonly MemberRepository $members,
+		private readonly UserGroupManager $userGroups,
 		private readonly AuthManager $authManager,
-		private readonly LoggerInterface $logger
+		private readonly LoggerInterface $logger,
+		private readonly string $readerGroup
 	) {
 	}
 
@@ -52,7 +61,7 @@ public function onPluggableAuthUserAuthorization( UserIdentity $user, bool &$aut
 		$member = $user->isRegistered() ? $this->members->getMember( $user->getId(), ReadConsistency::UpToDate ) : null;
 
 		if ( $user->isRegistered() && $member === null ) {
-			return $this->admitAccountThatIsNoMember( $user );
+			return $this->authorizeAccountThatIsNoMember( $user, $authorized );
 		}
 
 		// The address the roster recorded is the one the allowlist admitted, so it is what removing
@@ -89,6 +98,32 @@ public function onPluggableAuthUserAuthorization( UserIdentity $user, bool &$aut
 		return true;
 	}
 
+	/**
+	 * The reader group is what tells a staff account from one the roster forgot: it is the mark of
+	 * an account the allowlist created, so carrying it without a roster row means the roster forgot
+	 * the account rather than never knew it.
+	 */
+	private function authorizeAccountThatIsNoMember( UserIdentity $user, bool &$authorized ): bool {
+		if ( $this->holdsTheReaderGroup( $user ) ) {
+			return $this->refuseAccountTheRosterForgot( $user, $authorized );
+		}
+
+		return $this->admitAccountThatIsNoMember( $user );
+	}
+
+	private function holdsTheReaderGroup( UserIdentity $user ): bool {
+		return in_array( $this->readerGroup, $this->userGroups->getUserGroups( $user ), true );
+	}
+
+	private function refuseAccountTheRosterForgot( UserIdentity $user, bool &$authorized ): bool {
+		$authorized = false;
+		$this->logger->info( 'Single sign-on login refused: the account was provisioned through the allowlist but is no longer on the roster', [
+			'user' => $user->getId()
+		] );
+
+		return false;
+	}
+
 	private function admitAccountThatIsNoMember( UserIdentity $user ): bool {
 		$address = $this->addressOf( $user );
 

src/EntryPoints/UserListApiHandler.php

@@ -46,7 +46,7 @@ public function onApiCheckCanExecute( $module, $user, &$message ): bool {
 
 		$blocked = $this->firstBlockedSubmodule( $module );
 
-		if ( $blocked === null || !$this->isMember( $user ) ) {
+		if ( $blocked === null || !$this->holdsTheReaderGroup( $user ) ) {
 			return true;
 		}
 
@@ -100,7 +100,7 @@ private function asStrings( mixed $value ): array {
 		return $names;
 	}
 
-	private function isMember( UserIdentity $user ): bool {
+	private function holdsTheReaderGroup( UserIdentity $user ): bool {
 		return in_array( $this->readerGroup, $this->userGroups->getUserGroups( $user ), true );
 	}
 

src/MemberAccessExtension.php

@@ -117,8 +117,10 @@ private function newSsoAuthorizationHandler(): SsoAuthorizationHandler {
 			allowlistApplies: $this->allowlistAppliesToSso(),
 			matcher: $this->newAllowlistMatcher(),
 			members: $this->newMemberRepository(),
+			userGroups: MediaWikiServices::getInstance()->getUserGroupManager(),
 			authManager: MediaWikiServices::getInstance()->getAuthManager(),
-			logger: $this->newLogger()
+			logger: $this->newLogger(),
+			readerGroup: $this->getReaderGroup()
 		);
 	}
 

src/Persistence/MediaWikiMemberRemover.php

@@ -103,6 +103,9 @@ private function forgetAndRename(
 			$this->stripAddress( $userId );
 			$renamed = $this->newRename( $currentName, $reservedName, $userId, $performerId )->rename();
 		} catch ( Throwable $failure ) {
+			// The REST framework answers the rethrown failure with an error response, after which
+			// the request's transaction round commits as usual: without this cancel, the forgotten
+			// row and the stripped address would be committed without their rename.
 			$database->cancelAtomic( __METHOD__, $section );
 
 			throw $failure;

tests/phpunit/Application/RemoveMemberUseCaseTest.php

@@ -98,6 +98,13 @@ public function testFailedRemovalIsLoggedAsAnError(): void {
 		$this->assertNotSame( [], $this->logger->getEntriesAtLevel( 'error' ) );
 	}
 
+	public function testFailedRemovalIsLoggedWithoutTheAddress(): void {
+		$this->newUseCase( new SpyMemberRemover( RemovalResult::RemovalFailed ) )
+			->remove( self::MEMBER_ID, self::ADMIN_ID );
+
+		$this->assertStringNotContainsString( self::EMAIL, $this->logger->getLog() );
+	}
+
 	private function newUseCase( SpyMemberRemover $remover ): RemoveMemberUseCase {
 		return new RemoveMemberUseCase(
 			members: $this->members,

tests/phpunit/Integration/Auth/SsoAuthorizationHandlerTest.php

@@ -127,6 +127,50 @@ public function testAccountThatIsNoMemberAndIsAdmittedAnywayIsNotRecorded(): voi
 		$this->assertSame( [], $this->logger->getEntriesAtLevel( 'info' ) );
 	}
 
+	/**
+	 * An account holding the reader group without a roster row is one the allowlist once created
+	 * and has since forgotten: a removed member's parked account, or one left behind by a failed
+	 * provisioning. An identity provider that recorded the account can hand its logins back to it
+	 * for good, so admitting it as staff would put a forgotten account permanently outside the
+	 * allowlist.
+	 */
+	public function testAccountThatWasProvisionedButLeftTheRosterIsRefused(): void {
+		$this->allow( '@example.com' );
+
+		$this->assertFalse( $this->authorize( $this->provisionedUserOffTheRoster( 'jane@example.com' ) ) );
+	}
+
+	public function testRefusalOfAProvisionedAccountStopsTheOtherHandlersOfTheHook(): void {
+		$this->allow( '@example.com' );
+
+		$authorized = true;
+		$continue = $this->newHandler()->onPluggableAuthUserAuthorization(
+			$this->provisionedUserOffTheRoster( 'jane@example.com' ),
+			$authorized
+		);
+
+		$this->assertFalse( $continue );
+	}
+
+	/**
+	 * The account a removal parks keeps the reader group on purpose: it is what marks the account
+	 * as one of ours, and what an identity provider that still points at it is refused by.
+	 */
+	public function testRemovedMembersParkedAccountIsRefused(): void {
+		$this->allow( '@example.com' );
+		$member = $this->existingMember( 'jane@example.com' );
+		$this->addToReaderGroup( $member );
+
+		MemberAccessExtension::getInstance()->newRemoveMemberUseCase()->remove(
+			$member->getId(),
+			$this->getTestSysop()->getUser()->getId()
+		);
+
+		$parked = $this->getServiceContainer()->getUserFactory()->newFromId( $member->getId() );
+
+		$this->assertFalse( $this->authorize( $parked ) );
+	}
+
 	public function testMemberWhoseAddressIsNoLongerAdmittedIsRefused(): void {
 		$this->allow( '@example.com' );
 
@@ -287,8 +331,10 @@ private function newHandlerWith( bool $allowlistApplies ): SsoAuthorizationHandl
 			allowlistApplies: $allowlistApplies,
 			matcher: $extension->newAllowlistMatcher(),
 			members: $extension->newMemberRepository(),
+			userGroups: $this->getServiceContainer()->getUserGroupManager(),
 			authManager: $this->getServiceContainer()->getAuthManager(),
-			logger: $this->logger
+			logger: $this->logger,
+			readerGroup: 'reader'
 		);
 	}
 
@@ -329,6 +375,17 @@ private function existingUser( string $email ): User {
 		return $user;
 	}
 
+	private function provisionedUserOffTheRoster( string $email ): User {
+		$user = $this->existingUser( $email );
+		$this->addToReaderGroup( $user );
+
+		return $user;
+	}
+
+	private function addToReaderGroup( User $user ): void {
+		$this->getServiceContainer()->getUserGroupManager()->addUserToGroup( $user, 'reader' );
+	}
+
 	private function existingMember( string $email ): User {
 		return $this->recordAsMember( $email, groupId: $this->newGroupId() );
 	}

tests/phpunit/Integration/DatabaseMemberRepositoryTest.php

@@ -143,6 +143,23 @@ public function testReactivatedMemberIsActiveAgain(): void {
 		$this->assertTrue( $this->members->getMember( 7, ReadConsistency::UpToDate )?->isActive() );
 	}
 
+	public function testForgottenMemberIsGone(): void {
+		$this->recordMember( userId: 7, email: 'jane@example.com', groupId: 3 );
+
+		$this->members->forgetMember( 7 );
+
+		$this->assertNull( $this->members->getMember( 7, ReadConsistency::UpToDate ) );
+	}
+
+	public function testForgettingAMemberLeavesOtherMembersAlone(): void {
+		$this->recordMember( userId: 7, email: 'jane@example.com', groupId: 3 );
+		$this->recordMember( userId: 8, email: 'john@example.com', groupId: 3 );
+
+		$this->members->forgetMember( 7 );
+
+		$this->assertNotNull( $this->members->getMember( 8, ReadConsistency::UpToDate ) );
+	}
+
 	public function testTotalsCountEveryMemberAcrossGroups(): void {
 		$this->recordMember( userId: 1, email: 'first@example.com', groupId: 1 );
 		$this->recordMember( userId: 2, email: 'second@example.com', groupId: 1 );

tests/phpunit/Integration/MemberLogVisibilityTest.php

@@ -106,6 +106,7 @@ public function testRenameLogNamesTheRemovedMemberToAnAdmin(): void {
 
 		$this->assertSame( [ 'User:' . self::MEMBER_NAME ], array_column( $entries, 'title' ) );
 		$this->assertSame( [ 'Member removed' ], array_column( $entries, 'comment' ) );
+		$this->assertSame( [ $this->getTestSysop()->getUser()->getName() ], array_column( $entries, 'user' ) );
 	}
 
 	public function testRenameLogIsClosedToAMember(): void {

tests/phpunit/Integration/REST/RemoveMemberApiTest.php

@@ -6,19 +6,23 @@
 
 use MediaWiki\Auth\AuthenticationResponse;
 use MediaWiki\Deferred\DeferredUpdates;
-use MediaWiki\MainConfigNames;
 use MediaWiki\RenameUser\RenameuserSQL;
 use MediaWiki\Rest\ResponseInterface;
-use ProfessionalWiki\MemberAccess\Application\AllowlistValue;
 use ProfessionalWiki\MemberAccess\Application\Member;
 use ProfessionalWiki\MemberAccess\Application\NormalizedEmail;
 use ProfessionalWiki\MemberAccess\Application\ReadConsistency;
+use ProfessionalWiki\MemberAccess\Application\RemovalResult;
+use ProfessionalWiki\MemberAccess\Application\RemoveMemberUseCase;
 use ProfessionalWiki\MemberAccess\EntryPoints\Auth\EnterCodeRequest;
+use ProfessionalWiki\MemberAccess\EntryPoints\REST\RemoveMemberApi;
 use ProfessionalWiki\MemberAccess\MemberAccessExtension;
 use ProfessionalWiki\MemberAccess\Tests\Integration\Auth\AuthenticationProviderRegistration;
 use ProfessionalWiki\MemberAccess\Tests\Integration\Auth\CodeRequestSubmission;
 use ProfessionalWiki\MemberAccess\Tests\TestDoubles\FixedSecretGenerator;
+use ProfessionalWiki\MemberAccess\Tests\TestDoubles\InMemoryMemberRepository;
 use ProfessionalWiki\MemberAccess\Tests\TestDoubles\SpyEmailer;
+use ProfessionalWiki\MemberAccess\Tests\TestDoubles\SpyMemberRemover;
+use Psr\Log\NullLogger;
 use Wikimedia\ObjectCache\HashBagOStuff;
 use Wikimedia\Rdbms\IDBAccessObject;
 
@@ -145,6 +149,46 @@ public function testRemovingWithoutTheRightToManageMembersIsRefused(): void {
 		$this->assertNotNull( $this->rosterRowOf( $userId ) );
 	}
 
+	public function testRemovingWithoutACsrfTokenIsRefused(): void {
+		$userId = $this->newMember( $this->groupId, 'jane@example.com' );
+
+		$response = $this->runHandler(
+			MemberAccessExtension::newRemoveMemberApi(),
+			$this->newRequest( 'DELETE', [], [ 'userId' => (string)$userId ] ),
+			null,
+			$this->getSession( false )
+		);
+
+		$this->assertError( 'invalid_csrf_token', 403, $response );
+		$this->assertNotNull( $this->rosterRowOf( $userId ) );
+	}
+
+	public function testRefusedRenameAnswersRemovalFailed(): void {
+		$members = new InMemoryMemberRepository();
+		$members->recordMember( userId: 7, email: $this->normalizedEmail( 'jane@example.com' ), groupId: null );
+
+		$handler = new RemoveMemberApi(
+			$this->csrfTokens(),
+			new RemoveMemberUseCase(
+				members: $members,
+				remover: new SpyMemberRemover( RemovalResult::RemovalFailed ),
+				logger: new NullLogger()
+			)
+		);
+
+		$response = $this->runHandler( $handler, $this->newRequest( 'DELETE', [], [ 'userId' => '7' ] ) );
+
+		$this->assertError( 'removal_failed', 500, $response );
+	}
+
+	private function normalizedEmail( string $email ): NormalizedEmail {
+		$normalized = NormalizedEmail::fromString( $email );
+
+		$this->assertNotNull( $normalized );
+
+		return $normalized;
+	}
+
 	/**
 	 * Renaming whoever holds the name out of the way is not this endpoint's to do.
 	 */
@@ -278,21 +322,12 @@ private function createAccountNamed( string $name ): void {
 	}
 
 	private function recordAsMember( int $userId, string $email ): void {
-		$normalized = NormalizedEmail::fromString( $email );
-
-		$this->assertNotNull( $normalized );
-
 		MemberAccessExtension::getInstance()->newMemberRepository()
-			->recordMember( userId: $userId, email: $normalized, groupId: $this->groupId );
+			->recordMember( userId: $userId, email: $this->normalizedEmail( $email ), groupId: $this->groupId );
 	}
 
 	private function admitTheDomain(): void {
-		$value = AllowlistValue::fromString( '@example.com' );
-
-		$this->assertNotNull( $value );
-
-		MemberAccessExtension::getInstance()->newAllowlistRepository()
-			->addEntry( groupId: $this->groupId, value: $value, actorId: 1 );
+		$this->newEntry( $this->groupId, '@example.com' );
 
 		$this->setService( 'Emailer', new SpyEmailer() );
 		$this->registerOurAuthenticationProvider();
@@ -301,10 +336,7 @@ private function admitTheDomain(): void {
 		// turn on.
 		$this->overrideConfigValue( 'MemberAccessCodeLogin', 'allowlisted' );
 
-		$this->overrideConfigValue( MainConfigNames::GroupPermissions, array_replace_recursive(
-			$this->getConfVar( MainConfigNames::GroupPermissions ),
-			[ '*' => [ 'autocreateaccount' => true ] ]
-		) );
+		$this->setGroupPermissions( '*', 'autocreateaccount', true );
 
 		MemberAccessExtension::getInstance()->setStashOverride( new HashBagOStuff() );
 		MemberAccessExtension::getInstance()->setSecretGeneratorOverride( new FixedSecretGenerator( self::CODE ) );