Skip to content

Commit 8e37420

Browse files
PatStLouisPatStLouis
authored
Fix revocation setup (openwallet-foundation#4047)
* fix revocation registry rotation Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca> * add tests and formatting Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca> --------- Signed-off-by: Patrick St-Louis <patrick.st-louis@opsecid.ca>
1 parent fa5a142 commit 8e37420

File tree

2 files changed

+199
-74
lines changed

2 files changed

+199
-74
lines changed

‎acapy_agent/anoncreds/revocation/revocation.py‎

Lines changed: 55 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1489,13 +1489,37 @@ async def handle_full_registry_event(
14891489
)
14901490
await self.notify(event)
14911491

1492+
async def _get_backup_registry_id(self, cred_def_id: str) -> str:
1493+
"""Return the rev reg def id of the current backup (finished, not active)."""
1494+
async with self.profile.session() as session:
1495+
entries = await session.handle.fetch_all(
1496+
CATEGORY_REV_REG_DEF,
1497+
{
1498+
"active": "false",
1499+
"cred_def_id": cred_def_id,
1500+
"state": RevRegDefState.STATE_FINISHED,
1501+
},
1502+
limit=1,
1503+
)
1504+
if not entries:
1505+
raise AnonCredsRevocationError(
1506+
"No backup registry available for rotation. "
1507+
"Ensure the credential definition was created with a backup registry."
1508+
)
1509+
return entries[0].name
1510+
14921511
async def decommission_registry(self, cred_def_id: str) -> list:
1493-
"""Decommission post-init registries and start the next registry generation."""
1512+
"""Rotate: set backup active, create new backup, decommission old active.
1513+
1514+
Works with endorsement: the new registry is sent to the endorser and may not be
1515+
on the ledger yet, so we promote the existing backup to active (it is already
1516+
on the ledger) and treat the newly created registry as the new backup.
1517+
"""
14941518
active_reg = await self.get_or_create_active_registry(cred_def_id)
1519+
backup_rev_reg_def_id = await self._get_backup_registry_id(cred_def_id)
14951520

1496-
# create new one and set active
1497-
LOGGER.debug("Creating new registry to replace active one")
1498-
new_reg = await asyncio.shield(
1521+
LOGGER.debug("Creating new backup registry")
1522+
new_backup_reg = await asyncio.shield(
14991523
self.create_and_register_revocation_registry_definition(
15001524
issuer_id=active_reg.rev_reg_def.issuer_id,
15011525
cred_def_id=active_reg.rev_reg_def.cred_def_id,
@@ -1504,66 +1528,44 @@ async def decommission_registry(self, cred_def_id: str) -> list:
15041528
max_cred_num=active_reg.rev_reg_def.value.max_cred_num,
15051529
)
15061530
)
1507-
# set new as active...
1508-
if new_reg and not isinstance(new_reg, str):
1509-
new_rev_reg_def_id = new_reg.rev_reg_def_id
1510-
# Store the registry definition synchronously before setting it as active
1511-
# This ensures the registry is available in the wallet when
1512-
# set_active_registry tries to fetch it, avoiding a race condition
1513-
# with async event processing
1514-
await self.store_revocation_registry_definition(new_reg)
1515-
await self.set_active_registry(new_rev_reg_def_id)
1531+
new_rev_reg_def_id = None
1532+
if new_backup_reg and not isinstance(new_backup_reg, str):
1533+
new_rev_reg_def_id = new_backup_reg.rev_reg_def_id
1534+
await self.store_revocation_registry_definition(new_backup_reg)
1535+
elif isinstance(new_backup_reg, str):
1536+
LOGGER.error("Failed to create new backup registry: %s", new_backup_reg)
15161537
else:
1517-
new_rev_reg_def_id = None
1518-
if isinstance(new_reg, str):
1519-
LOGGER.error(f"Failed to create new registry: {new_reg}")
1520-
else:
1521-
LOGGER.warning("No new registry created while decommissioning registry")
1538+
LOGGER.warning("No new backup registry created while decommissioning")
1539+
1540+
await self.set_active_registry(backup_rev_reg_def_id)
1541+
1542+
keep_ids = {backup_rev_reg_def_id}
1543+
if new_rev_reg_def_id:
1544+
keep_ids.add(new_rev_reg_def_id)
15221545

1523-
# decommission everything except init/wait
15241546
async with self.profile.transaction() as txn:
15251547
registries = await txn.handle.fetch_all(
15261548
CATEGORY_REV_REG_DEF,
1527-
{
1528-
"cred_def_id": cred_def_id,
1529-
},
1549+
{"cred_def_id": cred_def_id},
15301550
for_update=True,
15311551
)
1532-
1533-
def filter_registries(registry: Entry) -> bool:
1534-
return registry.tags.get("state") != RevRegDefState.STATE_WAIT
1535-
1536-
recs = list(filter(filter_registries, registries))
1537-
1552+
recs = [
1553+
r for r in registries if r.tags.get("state") != RevRegDefState.STATE_WAIT
1554+
]
15381555
for rec in recs:
1539-
if rec.name != new_rev_reg_def_id:
1540-
tags = rec.tags
1541-
tags["active"] = "false"
1542-
tags["state"] = RevRegDefState.STATE_DECOMMISSIONED
1543-
await txn.handle.replace(
1544-
CATEGORY_REV_REG_DEF,
1545-
rec.name,
1546-
rec.value,
1547-
tags,
1548-
)
1556+
if rec.name in keep_ids:
1557+
continue
1558+
tags = rec.tags
1559+
tags["active"] = "false"
1560+
tags["state"] = RevRegDefState.STATE_DECOMMISSIONED
1561+
await txn.handle.replace(CATEGORY_REV_REG_DEF, rec.name, rec.value, tags)
15491562
await txn.commit()
1550-
# create a second one for backup, don't make it active
1551-
LOGGER.debug("Creating backup registry")
1552-
backup_reg = await asyncio.shield(
1553-
self.create_and_register_revocation_registry_definition(
1554-
issuer_id=active_reg.rev_reg_def.issuer_id,
1555-
cred_def_id=active_reg.rev_reg_def.cred_def_id,
1556-
registry_type=active_reg.rev_reg_def.type,
1557-
tag=self._generate_backup_registry_tag(),
1558-
max_cred_num=active_reg.rev_reg_def.value.max_cred_num,
1559-
)
1560-
)
15611563

15621564
LOGGER.debug(
1563-
"New registry = %s.\nBackup registry = %s.\nDecommissioned registries = %s",
1564-
new_reg,
1565-
backup_reg,
1566-
recs,
1565+
"Rotation done: backup %s set active, new backup=%s, decommissioned=%s",
1566+
backup_rev_reg_def_id,
1567+
new_rev_reg_def_id,
1568+
[r.name for r in recs if r.name not in keep_ids],
15671569
)
15681570
return recs
15691571

‎acapy_agent/anoncreds/revocation/tests/test_revocation.py‎

Lines changed: 144 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -881,25 +881,48 @@ async def test_handle_full_registry(self, mock_set_active_registry, mock_handle)
881881

882882
@mock.patch.object(AskarAnonCredsProfileSession, "handle")
883883
async def test_decommission_registry(self, mock_handle):
884+
# First fetch_all: find backup (active=false, state=finished)
885+
# Second fetch_all: in transaction, get all registries for cred_def_id
884886
mock_handle.fetch_all = mock.CoroutineMock(
885-
return_value=[
886-
MockEntry(
887-
name="active-reg-reg",
888-
tags={
889-
"state": RevRegDefState.STATE_FINISHED,
890-
"active": True,
891-
},
892-
),
893-
MockEntry(
894-
name="new-rev-reg",
895-
tags={
896-
"state": RevRegDefState.STATE_FINISHED,
897-
"active": True,
898-
},
899-
),
887+
side_effect=[
888+
[
889+
MockEntry(
890+
name="backup-reg-reg",
891+
tags={
892+
"cred_def_id": "test-rev-reg-def-id",
893+
"state": RevRegDefState.STATE_FINISHED,
894+
"active": "false",
895+
},
896+
),
897+
],
898+
[
899+
MockEntry(
900+
name="active-reg-reg",
901+
tags={
902+
"cred_def_id": "test-rev-reg-def-id",
903+
"state": RevRegDefState.STATE_FINISHED,
904+
"active": "true",
905+
},
906+
),
907+
MockEntry(
908+
name="backup-reg-reg",
909+
tags={
910+
"cred_def_id": "test-rev-reg-def-id",
911+
"state": RevRegDefState.STATE_FINISHED,
912+
"active": "false",
913+
},
914+
),
915+
MockEntry(
916+
name="new-rev-reg",
917+
tags={
918+
"cred_def_id": "test-rev-reg-def-id",
919+
"state": RevRegDefState.STATE_FINISHED,
920+
"active": "false",
921+
},
922+
),
923+
],
900924
]
901925
)
902-
# active registry
903926
self.revocation.get_or_create_active_registry = mock.CoroutineMock(
904927
return_value=RevRegDefResult(
905928
job_id="test-job-id",
@@ -912,7 +935,7 @@ async def test_decommission_registry(self, mock_handle):
912935
revocation_registry_definition_metadata={},
913936
)
914937
)
915-
# new active
938+
# New backup only (one call); previous backup becomes active
916939
self.revocation.create_and_register_revocation_registry_definition = (
917940
mock.CoroutineMock(
918941
return_value=RevRegDefResult(
@@ -936,18 +959,118 @@ async def test_decommission_registry(self, mock_handle):
936959
result = await self.revocation.decommission_registry("test-rev-reg-def-id")
937960

938961
assert isinstance(result, list)
939-
assert len(result) == 2
962+
assert len(result) == 3
963+
# First entry (active-reg-reg) is decommissioned; backup and new backup kept
940964
assert result[0].tags["active"] == "false"
941965
assert result[0].tags["state"] == RevRegDefState.STATE_DECOMMISSIONED
942966
assert mock_handle.fetch_all.called
943967
assert mock_handle.replace.called
944-
# Verify store_revocation_registry_definition was called before set_active_registry
945968
self.revocation.store_revocation_registry_definition.assert_called_once()
946-
# # One for backup
969+
# Previous backup set as active (works with endorsement: backup already on ledger)
970+
self.revocation.set_active_registry.assert_called_once_with("backup-reg-reg")
971+
# One new backup registry created (not two)
947972
assert (
948973
self.revocation.create_and_register_revocation_registry_definition.call_count
949-
== 2
974+
== 1
975+
)
976+
977+
@mock.patch.object(AskarAnonCredsProfileSession, "handle")
978+
async def test_decommission_registry_no_backup_raises(self, mock_handle):
979+
"""When no backup registry exists, rotation raises."""
980+
mock_handle.fetch_all = mock.CoroutineMock(return_value=[])
981+
self.revocation.get_or_create_active_registry = mock.CoroutineMock(
982+
return_value=RevRegDefResult(
983+
job_id="test-job-id",
984+
revocation_registry_definition_state=RevRegDefState(
985+
state=RevRegDefState.STATE_FINISHED,
986+
revocation_registry_definition_id="active-reg-reg",
987+
revocation_registry_definition=rev_reg_def,
988+
),
989+
registration_metadata={},
990+
revocation_registry_definition_metadata={},
991+
)
992+
)
993+
with self.assertRaises(test_module.AnonCredsRevocationError) as cm:
994+
await self.revocation.decommission_registry("test-rev-reg-def-id")
995+
self.assertIn("No backup registry available", str(cm.exception))
996+
997+
@mock.patch.object(AskarAnonCredsProfileSession, "handle")
998+
async def test_decommission_registry_new_backup_creation_fails(self, mock_handle):
999+
"""When creating the new backup fails, we still promote backup to active and decommission old."""
1000+
backup_entry = MockEntry(
1001+
name="backup-reg-reg",
1002+
tags={
1003+
"cred_def_id": "test-rev-reg-def-id",
1004+
"state": RevRegDefState.STATE_FINISHED,
1005+
"active": "false",
1006+
},
1007+
)
1008+
mock_handle.fetch_all = mock.CoroutineMock(
1009+
side_effect=[
1010+
[backup_entry],
1011+
[
1012+
MockEntry(
1013+
name="active-reg-reg",
1014+
tags={
1015+
"cred_def_id": "test-rev-reg-def-id",
1016+
"state": RevRegDefState.STATE_FINISHED,
1017+
"active": "true",
1018+
},
1019+
),
1020+
backup_entry,
1021+
],
1022+
]
1023+
)
1024+
mock_handle.replace = mock.CoroutineMock(return_value=None)
1025+
self.revocation.get_or_create_active_registry = mock.CoroutineMock(
1026+
return_value=RevRegDefResult(
1027+
job_id="test-job-id",
1028+
revocation_registry_definition_state=RevRegDefState(
1029+
state=RevRegDefState.STATE_FINISHED,
1030+
revocation_registry_definition_id="active-reg-reg",
1031+
revocation_registry_definition=rev_reg_def,
1032+
),
1033+
registration_metadata={},
1034+
revocation_registry_definition_metadata={},
1035+
)
1036+
)
1037+
self.revocation.create_and_register_revocation_registry_definition = (
1038+
mock.CoroutineMock(return_value="Failed to create new registry")
1039+
)
1040+
self.revocation.set_active_registry = mock.CoroutineMock(return_value=None)
1041+
1042+
result = await self.revocation.decommission_registry("test-rev-reg-def-id")
1043+
1044+
self.revocation.set_active_registry.assert_called_once_with("backup-reg-reg")
1045+
assert mock_handle.replace.call_count == 1
1046+
assert len(result) == 2
1047+
assert result[0].tags["state"] == RevRegDefState.STATE_DECOMMISSIONED
1048+
1049+
@mock.patch.object(AskarAnonCredsProfileSession, "handle")
1050+
async def test_get_backup_registry_id_raises_when_no_backup(self, mock_handle):
1051+
"""_get_backup_registry_id raises when no finished backup exists."""
1052+
mock_handle.fetch_all = mock.CoroutineMock(return_value=[])
1053+
with self.assertRaises(test_module.AnonCredsRevocationError) as cm:
1054+
await self.revocation._get_backup_registry_id("test-cred-def-id")
1055+
self.assertIn("No backup registry available", str(cm.exception))
1056+
1057+
@mock.patch.object(AskarAnonCredsProfileSession, "handle")
1058+
async def test_get_backup_registry_id_returns_first_backup(self, mock_handle):
1059+
"""_get_backup_registry_id returns the name of the first matching backup."""
1060+
mock_handle.fetch_all = mock.CoroutineMock(
1061+
return_value=[
1062+
MockEntry(
1063+
name="backup-id-123",
1064+
tags={
1065+
"cred_def_id": "test-cred-def-id",
1066+
"state": RevRegDefState.STATE_FINISHED,
1067+
"active": "false",
1068+
},
1069+
),
1070+
]
9501071
)
1072+
result = await self.revocation._get_backup_registry_id("test-cred-def-id")
1073+
assert result == "backup-id-123"
9511074

9521075
@mock.patch.object(AskarAnonCredsProfileSession, "handle")
9531076
async def test_get_or_create_active_registry(self, mock_handle):

0 commit comments

Comments
 (0)