diff --git a/ansible-runner/project b/ansible-runner/project index d0b52bd..9497aed 160000 --- a/ansible-runner/project +++ b/ansible-runner/project @@ -1 +1 @@ -Subproject commit d0b52bd0d6274fff6316a3f09e80d014f040cca1 +Subproject commit 9497aed512ae9bd1d42b088fae748ddc2a24bc9a diff --git a/modules/coactd.py b/modules/coactd.py index 634d04f..53aedad 100644 --- a/modules/coactd.py +++ b/modules/coactd.py @@ -566,18 +566,63 @@ def do_new_repo( self.logger.warning(f"Failed to create grouper POSIX group for {facility}:{repo}: {e}") raise + # Query current users and leaders from database if repo already exists + repo_users = [principal] # Default to just principal for new repos + repo_leaders = [principal] # Default to principal as leader for new repos + repo_req = {'facility': facility, 'name': repo} + REPO_USERS_GQL = gql(""" + query repo($facility: String!, $name: String!) { + repo(filter: {facility: $facility, name: $name}) { + users + leaders + } + } + """) + try: + repo_data = self.back_channel.execute(REPO_USERS_GQL, repo_req) + + # repo: null means "not found" - use defaults + if repo_data.get('repo') is None: + self.logger.info(f"Repo {facility}:{repo} not found in database, creating new with principal only") + elif repo_data.get('repo'): + # Repo exists - preserve existing users and leaders + if repo_data['repo'].get('users'): + repo_users = repo_data['repo']['users'] + self.logger.info(f"Found existing repo {facility}:{repo} with {len(repo_users)} users: {repo_users}") + # Ensure principal is in the list + if principal not in repo_users: + repo_users.append(principal) + + # Preserve existing leaders + if repo_data['repo'].get('leaders'): + repo_leaders = repo_data['repo']['leaders'] + self.logger.info(f"Found existing repo {facility}:{repo} with {len(repo_leaders)} leaders: {repo_leaders}") + # Ensure principal is in the leaders list + if principal not in repo_leaders: + repo_leaders.append(principal) + except Exception as e: + # Query failed due to other error - do not proceed + error_msg = f"Failed to query existing repo data for {facility}:{repo}: {e}" + self.logger.error(error_msg) + raise RuntimeError( + f"Cannot safely proceed with NewRepo: database query failed. " + f"Proceeding with defaults could overwrite existing users/leaders. " + f"Original error: {e}" + ) from e + # run the facility tasks for this repo self.run_playbook( "coact/add_repo.yaml", - facility=facility, + facility=facility, repo=repo, repo_principal=principal, + repo_users=repo_users, gidNumber=repo_gid, groupName=grouper_name ) - leaders = [principal] - users = [principal] + leaders = repo_leaders + users = repo_users # write back to coact the repo information repo_create_req = { diff --git a/tests/test_repo_registration_gid.py b/tests/test_repo_registration_gid.py index b55c8d8..e3eea4e 100644 --- a/tests/test_repo_registration_gid.py +++ b/tests/test_repo_registration_gid.py @@ -59,6 +59,7 @@ def test_gid_extraction_success_cryoem(self, repo_registration: RepoRegistration # Mock extract_grouper_values to return the GID repo_registration.extract_grouper_values = Mock(return_value=('12345', 'sdf-cryoem-ct-test')) repo_registration.back_channel.execute.side_effect = [ + {'repo': None}, # Query returns null (repo not found) {'repoUpsert': {'Id': 'repo-456'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}}, {'repoUpsertFeature': {'Id': 'feature-posix'}} @@ -115,6 +116,7 @@ def test_non_grouper_facility_skips_gid(self, repo_registration, mock_ansible_ru # Setup repo_registration.run_playbook.return_value = mock_ansible_runner repo_registration.back_channel.execute.side_effect = [ + {'repo': None}, # Query returns null (repo not found) {'repoUpsert': {'Id': 'repo-123'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}} ] @@ -130,16 +132,22 @@ def test_non_grouper_facility_skips_gid(self, repo_registration, mock_ansible_ru assert result is True # Only add_repo.yaml should run — grouper.yml should NOT be called - # The new implementation passes repo_principal, gidNumber=None, and groupName='' + # The new implementation passes repo_principal, repo_users, gidNumber=None, and groupName='' repo_registration.run_playbook.assert_called_once_with( - 'coact/add_repo.yaml', facility='OTHER', repo='test-repo', repo_principal='test-user', gidNumber=None, groupName='' + 'coact/add_repo.yaml', + facility='OTHER', + repo='test-repo', + repo_principal='test-user', + repo_users=['test-user'], # New parameter added by idempotency fix + gidNumber=None, + groupName='' ) # extract_grouper_values should not be called for non-grouper facilities if hasattr(repo_registration, 'extract_grouper_values') and isinstance(repo_registration.extract_grouper_values, Mock): repo_registration.extract_grouper_values.assert_not_called() - # Should only create slurm feature (2 back_channel calls: repoUpsert + feature) - assert repo_registration.back_channel.execute.call_count == 2 + # Should only create slurm feature (3 back_channel calls: user query + repoUpsert + feature) + assert repo_registration.back_channel.execute.call_count == 3 def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runner): """Test that ALL cryoem repos (not just ct/ce) unconditionally use grouper.""" @@ -148,6 +156,7 @@ def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runn # Mock extract_grouper_values to return a valid GID for any CryoEM repo repo_registration.extract_grouper_values = Mock(return_value=('54321', 'sdf-cryoem-other-repo')) repo_registration.back_channel.execute.side_effect = [ + {'repo': None}, # Query returns null (repo not found) {'repoUpsert': {'Id': 'repo-123'}}, {'repoUpsertFeature': {'Id': 'feature-slurm'}}, {'repoUpsertFeature': {'Id': 'feature-posix'}} @@ -176,5 +185,119 @@ def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runn repo_registration.logger.info.assert_any_call( "Retrieved repo GID for cryoem:other-repo: 54321" ) - # Should create both slurm and posixgroup features (3 back_channel calls total) - assert repo_registration.back_channel.execute.call_count == 3 + # Should create both slurm and posixgroup features (4 back_channel calls: user query + repoUpsert + 2 features) + assert repo_registration.back_channel.execute.call_count == 4 + + +class TestRepoIdempotency: + """Test idempotency fixes for NewRepo request workflow.""" + + @pytest.fixture + def repo_registration(self) -> RepoRegistration: + """Create a RepoRegistration instance with mocked dependencies.""" + with patch('modules.coactd.GraphQlSubscriber.__init__'), \ + patch('modules.coactd.AnsibleRunner.__init__'): + reg = RepoRegistration( + username='test-user', + password_file='/tmp/test-password', + client_name='test-client', + grouper_password_file='/tmp/test-grouper-password' + ) + reg.logger = Mock() + reg.back_channel = Mock() + reg.run_playbook = Mock() + reg.extract_grouper_values = Mock(return_value=('12345', 'sdf-cryoem-test-repo')) + return reg + + @pytest.fixture + def mock_ansible_runner(self) -> Mock: + """Mock ansible runner with realistic structure.""" + runner = Mock() + runner.events = [] + return runner + + def test_fresh_newrepo_uses_principal_only(self, repo_registration: RepoRegistration, mock_ansible_runner: Mock): + """ + When creating a brand new repo that doesn't exist in the database, + the playbook should receive only the principal user. + """ + # Setup - repo doesn't exist in database + repo_registration.run_playbook.return_value = mock_ansible_runner + repo_registration.back_channel.execute.side_effect = [ + {'repo': None}, # Normal "not found" + {'repoUpsert': {'Id': 'repo-123'}}, # Repo creation succeeds + {'repoUpsertFeature': {'Id': 'feature-slurm'}}, + {'repoUpsertFeature': {'Id': 'feature-posix'}} + ] + + # Execute + result = repo_registration.do_new_repo( + repo='test-new-1', + facility='cryoem', + principal='user1' + ) + + # Verify + assert result is True + + # Check that the playbook was called with only the principal + playbook_call = [c for c in repo_registration.run_playbook.call_args_list + if c[0][0] == 'coact/add_repo.yaml'][0] + assert playbook_call[1]['repo_users'] == ['user1'] + assert playbook_call[1]['repo_principal'] == 'user1' + + # Verify logging indicates repo not found + log_messages = [str(c) for c in repo_registration.logger.info.call_args_list] + assert any('not found in database' in msg for msg in log_messages) + + def test_rerun_newrepo_preserves_existing_users_and_leaders(self, repo_registration: RepoRegistration, mock_ansible_runner: Mock): + """ + Test complete idempotency: re-running NewRepo on an existing repo must preserve + both users and leaders lists, not reset them to just the principal. + """ + # Setup - repo exists with 3 users and 2 leaders + repo_registration.run_playbook.return_value = mock_ansible_runner + repo_registration.back_channel.execute.side_effect = [ + { + 'repo': { + 'users': ['user1', 'user2', 'user3'], + 'leaders': ['user1', 'leader2'] # Multiple leaders + } + }, + {'repoUpsert': {'Id': 'repo-123'}}, + {'repoUpsertFeature': {'Id': 'feature-slurm'}}, + {'repoUpsertFeature': {'Id': 'feature-posix'}} + ] + + # Execute - re-run NewRepo for existing repo + result = repo_registration.do_new_repo( + repo='test-existing', + facility='cryoem', + principal='user1' + ) + + # Verify result + assert result is True + + # Verify Ansible playbook received ALL existing users (for LDAP sync) + playbook_call = [c for c in repo_registration.run_playbook.call_args_list + if c[0][0] == 'coact/add_repo.yaml'][0] + assert set(playbook_call[1]['repo_users']) == {'user1', 'user2', 'user3'}, \ + "Ansible playbook must receive all users for LDAP sync" + + # The second back_channel.execute call is the repoUpsert mutation + upsert_call = repo_registration.back_channel.execute.call_args_list[1] + upsert_data = upsert_call[0][1] # Second argument is the variables + + assert set(upsert_data['repo']['users']) == {'user1', 'user2', 'user3'}, \ + f"Database write must preserve all users, not reset to principal! Got: {upsert_data['repo']['users']}" + + assert set(upsert_data['repo']['leaders']) == {'user1', 'leader2'}, \ + f"Database write must preserve all leaders, not reset to principal! Got: {upsert_data['repo']['leaders']}" + + # Verify logging indicates existing data was found + log_messages = [str(c) for c in repo_registration.logger.info.call_args_list] + assert any('Found existing repo' in msg and '3 users' in msg for msg in log_messages), \ + "Should log that existing users were found" + assert any('2 leaders' in msg for msg in log_messages), \ + "Should log that existing leaders were found"