diff --git a/assertion-service-2/scripts/query-fixes/demote_non_superadmin_admins.py b/assertion-service-2/scripts/query-fixes/demote_non_superadmin_admins.py new file mode 100644 index 000000000..6eb3afd57 --- /dev/null +++ b/assertion-service-2/scripts/query-fixes/demote_non_superadmin_admins.py @@ -0,0 +1,302 @@ +#!/usr/bin/env python3 +""" +Demote admin users that do not belong to a superadmin-enabled member. + +An admin user is only legitimate when the member it belongs to has +`superadmin_enabled == true`. Historically (before server-side validation was +added) admin users could be created against members that are not +superadmin-enabled - for example by ticking the "Admin" checkbox for a +superadmin-enabled member and then switching the organization to a +non-superadmin-enabled one before saving. This script cleans up that bad data. + +The check spans two databases: + - memberservice.member (source of truth for `superadmin_enabled`) + - userservice.jhi_user (holds the `admin` flag to be corrected) + +Logic: + 1. Load every superadmin-enabled member from `memberservice.member` and build + the set of their `_id` (hex) values. + 2. Scan every admin user (`admin == true`) in `userservice.jhi_user`. + 3. An admin user is kept ONLY if it clearly belongs to a superadmin-enabled + member - its `member_id` matches a superadmin member `_id`. + Every other admin user (non-superadmin member, orphaned member reference, + or no member at all) has `admin` set to false. + +Related to: https://orcid.clickup.com/t/PD-5857 + +Usage: + python demote_non_superadmin_admins.py # plan, confirm, apply, verify + python demote_non_superadmin_admins.py --dry-run # only report what would change + +Environment Variables: + SPRING_DATA_MONGODB_URI - MongoDB connection string (read from env) +""" + +import argparse +import sys +from pathlib import Path +from typing import Dict, List, Set, Tuple + +from pymongo import UpdateOne +from pymongo.collection import Collection +from pymongo.errors import BulkWriteError, OperationFailure + +CURRENT_DIR = Path(__file__).resolve().parent +UTILS_DIR = CURRENT_DIR.parent / "utils" + +if str(UTILS_DIR) not in sys.path: + sys.path.insert(0, str(UTILS_DIR)) + +# Import shared modules +from logger_config import setup_logger +from db_connection import MongoDBConnection +from config import Config + +# Set up logging +logger = setup_logger(__name__, log_file='demote-non-superadmin-admins.log') + +# Number of bulk operations to accumulate before flushing to MongoDB. +BATCH_SIZE = 1000 +# How often (in documents scanned) to emit a progress line. +PROGRESS_EVERY = 100_000 + + +class DemotionError(RuntimeError): + pass + + +class SuperadminMembers: + """Read-only view of superadmin-enabled members.""" + + def __init__(self, connection_to_db: MongoDBConnection): + self.collection_member = connection_to_db.get_collection('member') + self.member_ids: Set[str] = set() + + def load(self) -> None: + """Populate the allowed `_id` (hex) set.""" + logger.info("\n" + "=" * 80) + logger.info("Loading superadmin-enabled members...") + logger.info("=" * 80) + + try: + members = list( + self.collection_member.find( + {'superadmin_enabled': True}, + {'_id': 1, 'client_name': 1}, + ) + ) + except OperationFailure as e: + logger.error(f"Failed to query members: {e}") + raise + + for member in members: + raw_id = member.get('_id') + if raw_id is not None: + # Java stores memberId as the hex string of the ObjectId. + self.member_ids.add(str(raw_id)) + + logger.info( + "Superadmin-enabled members loaded: %d", + len(members), + ) + logger.info("=" * 80) + + def is_allowed_admin(self, user: Dict) -> bool: + """Return True if this user legitimately belongs to a superadmin member.""" + member_id = user.get('member_id') + return bool(member_id) and member_id in self.member_ids + + +class AdminUserDemoter: + """Find and demote admin users that do not belong to a superadmin member.""" + + def __init__(self, collection: Collection, superadmin_members: SuperadminMembers): + self.collection = collection + self.superadmin_members = superadmin_members + + def find_users_to_demote(self) -> List[Dict]: + """Single read-only pass over admin users; return those to demote.""" + logger.info(" Scanning admin users in jhi_user ...") + scanned = 0 + to_demote: List[Dict] = [] + cursor = self.collection.find( + {'admin': True}, + {'_id': 1, 'email': 1, 'member_id': 1, 'member_name': 1, 'salesforce_id': 1}, + no_cursor_timeout=True, + ) + try: + for user in cursor: + scanned += 1 + if scanned % PROGRESS_EVERY == 0: + logger.info(" ...jhi_user: scanned %d admin users, %d to demote", scanned, len(to_demote)) + if not self.superadmin_members.is_allowed_admin(user): + to_demote.append(user) + finally: + cursor.close() + + logger.info(" Admin users scanned: %d, to demote: %d", scanned, len(to_demote)) + return to_demote + + def demote(self, users: List[Dict]) -> int: + """Set admin=false for the given users in batched bulk writes.""" + modified = 0 + batch: List[UpdateOne] = [] + for user in users: + batch.append(UpdateOne({'_id': user['_id']}, {'$set': {'admin': False}})) + if len(batch) >= BATCH_SIZE: + modified += self._flush(batch) + batch = [] + modified += self._flush(batch) + return modified + + def _flush(self, batch: List[UpdateOne]) -> int: + if not batch: + return 0 + try: + return self.collection.bulk_write(batch, ordered=False).modified_count + except (BulkWriteError, OperationFailure) as e: + detail = getattr(e, 'details', e) + logger.error(f" Bulk write failed on jhi_user: {detail}") + raise DemotionError(f"jhi_user bulk write failed: {e}") from e + + +def _log_affected_users(users: List[Dict]) -> None: + logger.info("\nUsers that will have admin set to false:") + for user in users: + logger.info( + " - email=%s member_id=%s member_name=%s salesforce_id=%s", + user.get('email'), + user.get('member_id'), + user.get('member_name'), + user.get('salesforce_id'), + ) + + +def parse_arguments() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description="Demote admin users that do not belong to a superadmin-enabled member.", + formatter_class=argparse.RawDescriptionHelpFormatter, + epilog=""" +Examples: + # Report and, after confirmation, fix the data + python demote_non_superadmin_admins.py + + # Only report what would change (no writes, no prompt) + python demote_non_superadmin_admins.py --dry-run + +Environment Variables: + SPRING_DATA_MONGODB_URI - MongoDB connection string (read from env) + """, + ) + parser.add_argument( + '--dry-run', + action='store_true', + help='Only report the admin users that would be demoted; make no changes.', + ) + return parser.parse_args() + + +def main() -> int: + args = parse_arguments() + + config = Config() + mongo_uri = config.mongo_uri + + database_memberservice = 'memberservice' + database_userservice = 'userservice' + + logger.info("=" * 80) + logger.info("Demote admin users not belonging to a superadmin-enabled member") + logger.info("=" * 80) + logger.info(f"Databases: {database_memberservice}, {database_userservice}") + logger.info("Collections: member, jhi_user") + logger.info(f"MongoDB URI: {mongo_uri[:20]}..." if len(mongo_uri) > 20 else f"MongoDB URI: {mongo_uri}") + logger.info("Mode: %s", "DRY RUN (no writes)" if args.dry_run else "APPLY (after confirmation)") + logger.info("=" * 80 + "\n") + + connection_memberservice = MongoDBConnection(mongo_uri, database_memberservice) + connection_userservice = MongoDBConnection(mongo_uri, database_userservice) + + try: + if not connection_memberservice.connect(): + logger.error("Failed to connect to memberservice MongoDB. Exiting.") + return 1 + if not connection_userservice.connect(): + logger.error("Failed to connect to userservice MongoDB. Exiting.") + return 1 + + superadmin_members = SuperadminMembers(connection_memberservice) + superadmin_members.load() + + demoter = AdminUserDemoter( + connection_userservice.get_collection('jhi_user'), + superadmin_members, + ) + + # ---- Planning phase: one read-only pass, no writes ---- + logger.info("\n" + "=" * 80) + logger.info("PLANNING: scanning admin users to find those to demote") + logger.info("(no writes happen in this phase)") + logger.info("=" * 80) + + to_demote = demoter.find_users_to_demote() + + if not to_demote: + logger.info("\n No admin users need demoting. Nothing to do.") + return 0 + + _log_affected_users(to_demote) + + logger.info("\n" + "=" * 80) + logger.info(" WARNING: This will modify the database!") + logger.info(f" Admin users to demote (admin -> false): {len(to_demote)}") + logger.info(" Member documents will NOT be modified") + logger.info("=" * 80) + + if args.dry_run: + logger.info("\n Dry run - no changes made. Re-run without --dry-run to apply.") + return 0 + + try: + response = input("\nDo you want to proceed? (yes/no): ").strip().lower() + if response not in ('yes', 'y'): + logger.info("\n Operation cancelled by user") + return 0 + except (KeyboardInterrupt, EOFError): + logger.info("\n\n Operation cancelled by user") + return 1 + + # ---- Execution phase: batched bulk writes ---- + logger.info("\n" + "=" * 80) + logger.info("EXECUTING DEMOTION") + logger.info("=" * 80) + + modified = demoter.demote(to_demote) + logger.info(" jhi_user: %d admin users demoted", modified) + + # ---- Verification phase: re-scan, expect nothing left ---- + logger.info("\n" + "=" * 80) + logger.info("VERIFYING DEMOTION") + logger.info("=" * 80) + + remaining = demoter.find_users_to_demote() + if remaining: + logger.error( + " %d admin user(s) still do not belong to a superadmin-enabled member!", + len(remaining), + ) + return 1 + + logger.info("\n All admin users now belong to a superadmin-enabled member.") + return 0 + + except (DemotionError, OperationFailure) as e: + logger.error(f" Demotion failed: {e}") + return 1 + finally: + connection_userservice.disconnect() + connection_memberservice.disconnect() + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/ui-2/src/app/user/user-update.component.spec.ts b/ui-2/src/app/user/user-update.component.spec.ts index c8b6985d5..f354dc0fe 100644 --- a/ui-2/src/app/user/user-update.component.spec.ts +++ b/ui-2/src/app/user/user-update.component.spec.ts @@ -369,6 +369,26 @@ describe('UserUpdateComponent', () => { expect(enableSpy).toHaveBeenCalled() }) + it('should reset isAdmin when switching to a non-superadmin member', () => { + accountService.hasAnyAuthority.and.callFake((authorities: string[]) => authorities.includes('ROLE_ADMIN')) + memberService.getAllMembers.and.returnValue( + of([ + { id: 'member-superadmin', clientName: 'A', superadminEnabled: true } as Member, + { id: 'member-standard', clientName: 'B', superadminEnabled: false } as Member, + ]) + ) + activatedRoute.data = of({ user: null as unknown as IUser }) + + fixture.detectChanges() + + component.editForm.patchValue({ memberId: 'member-superadmin' }) + component.editForm.patchValue({ isAdmin: true }) + + component.editForm.patchValue({ memberId: 'member-standard' }) + + expect(component.editForm.get('isAdmin')?.value).toBe(false) + }) + it('should send activation email for existing user', () => { activatedRoute.data = of({ user: { memberId: 'test', id: 'id' } as IUser }) userService.sendActivate.and.returnValue(of(new User())) diff --git a/ui-2/src/app/user/user-update.component.ts b/ui-2/src/app/user/user-update.component.ts index b3e7262ac..3eb473bb0 100644 --- a/ui-2/src/app/user/user-update.component.ts +++ b/ui-2/src/app/user/user-update.component.ts @@ -128,6 +128,9 @@ export class UserUpdateComponent implements OnInit { if (this.hasRoleAdmin()) { if (selectedOrg) { this.showIsAdminCheckbox.set(selectedOrg.superadminEnabled || false) + if (!selectedOrg.superadminEnabled) { + this.editForm.patchValue({ isAdmin: false }) + } } else { this.showIsAdminCheckbox.set(false) } @@ -309,6 +312,10 @@ export class UserUpdateComponent implements OnInit { this.subscribeToSaveResponse(this.userService.create(userFromForm)) } } + const selectedOrg = this.memberList().find((cm) => cm.id === this.editForm.get(['memberId'])?.value) + if (selectedOrg?.superadminEnabled && this.hasRoleAdmin()) { + this.editForm.get('isAdmin')?.setValue(false) + } } else { this.isSaving.set(false) this.validation.set(data) diff --git a/user-service-2/src/main/java/org/orcid/mp/user/service/UserService.java b/user-service-2/src/main/java/org/orcid/mp/user/service/UserService.java index 6d9fd0557..03e7ba5e5 100644 --- a/user-service-2/src/main/java/org/orcid/mp/user/service/UserService.java +++ b/user-service-2/src/main/java/org/orcid/mp/user/service/UserService.java @@ -182,6 +182,8 @@ public UserDTO createUser(UserDTO userDTO) { LOG.warn("Attempt to create user with non existent member {}", userDTO.getMemberId()); throw new BadRequestAlertException("Member does not exist"); } + checkAdminAllowedForMember(userDTO, member); + String createdBy = SecurityUtil.getCurrentUserLogin().get(); // change the auth if the logged in user is org owner and this is set as // mainContact @@ -214,6 +216,12 @@ public UserDTO createUser(UserDTO userDTO) { return userMapper.toUserDTO(newUser); } + private void checkAdminAllowedForMember(UserDTO userDTO, Member member) { + if (userDTO.getIsAdmin() && (member == null || member.getSuperadminEnabled() == null || !member.getSuperadminEnabled())) { + throw new BadRequestAlertException("Admin users cannot be associated with this member"); + } + } + private User createUserRecord(UserDTO userDTO) { User user = userMapper.toUser(userDTO); user.setLangKey(Constants.DEFAULT_LANGUAGE); // default language @@ -265,6 +273,7 @@ public Optional updateUser(UserDTO userDTO) { if (!existingUser.isPresent()) { throw new EmailNotFoundException(); } + checkAdminAllowedForMember(userDTO, member); checkUpdateConstraints(existingUser, userDTO); User user = existingUser.get(); diff --git a/user-service-2/src/test/java/org/orcid/mp/user/service/UserServiceTest.java b/user-service-2/src/test/java/org/orcid/mp/user/service/UserServiceTest.java index 4cd443c28..756ccb560 100644 --- a/user-service-2/src/test/java/org/orcid/mp/user/service/UserServiceTest.java +++ b/user-service-2/src/test/java/org/orcid/mp/user/service/UserServiceTest.java @@ -358,7 +358,7 @@ public User answer(InvocationOnMock invocation) throws Throwable { userDTO.setId("id"); userDTO.setEmail("email@orcid.org"); userDTO.setMainContact(false); - userDTO.setIsAdmin(true); + userDTO.setIsAdmin(false); userService.updateUser(userDTO); verify(userRepository, Mockito.times(1)).save(userCaptor.capture()); @@ -367,7 +367,7 @@ public User answer(InvocationOnMock invocation) throws Throwable { assertEquals(userDTO.getFirstName(), user.getFirstName()); assertEquals(userDTO.getLastName(), user.getLastName()); assertEquals(userDTO.getEmail(), user.getEmail()); - assertTrue(user.getAdmin()); + assertFalse(user.getAdmin()); assertNotNull(user.getLastModifiedBy()); assertNotNull(user.getLastModifiedDate()); } @@ -823,6 +823,35 @@ private UserUpload getUserUpload() { return upload; } + @Test + void testCreateUserAsAdminForNonSuperadminMemberThrows() { + when(memberServiceClient.getMember(Mockito.anyString())).thenReturn(memberWithoutSuperadminEnabled()); + + UserDTO userDTO = getUserDTO(); + userDTO.setIsAdmin(true); + + assertThrows(BadRequestAlertException.class, () -> userService.createUser(userDTO)); + verify(userRepository, Mockito.never()).save(Mockito.any(User.class)); + } + + @Test + void testUpdateUserAsAdminForNonSuperadminMemberThrows() { + when(memberServiceClient.getMember(Mockito.anyString())).thenReturn(memberWithoutSuperadminEnabled()); + + User existing = new User(); + existing.setId("id"); + existing.setEmail("email@email.com"); + existing.setMainContact(false); + when(userRepository.findOneByEmailIgnoreCase(Mockito.anyString())).thenReturn(Optional.of(existing)); + + UserDTO userDTO = getUserDTO(); + userDTO.setId("id"); + userDTO.setIsAdmin(true); + + assertThrows(BadRequestAlertException.class, () -> userService.updateUser(userDTO)); + verify(userRepository, Mockito.never()).save(Mockito.any(User.class)); + } + private UserDTO getUserDTO() { UserDTO user = new UserDTO(); user.setActivated(false); @@ -851,4 +880,12 @@ private Member memberWithoutAMEnabled() { member.setAssertionServiceEnabled(false); return member; } -} \ No newline at end of file + + private Member memberWithoutSuperadminEnabled() { + Member member = new Member(); + member.setMemberId("member-id"); + member.setClientName("member"); + member.setSuperadminEnabled(false); + return member; + } +}