Skip to content

Commit 5fe058f

Browse files
ff137ff137
authored
🐛 Improve efficiency of revoking credentials (#3795)
* ✨ Retrieve cred rev records with list of ids Signed-off-by: ff137 <ff137@proton.me> * ✨ Set revoke state efficiently. Resolves #3792 Signed-off-by: ff137 <ff137@proton.me> * 🎨 Update route to handle list return type Signed-off-by: ff137 <ff137@proton.me> * 🧪 Update tests Signed-off-by: ff137 <ff137@proton.me> --------- Signed-off-by: ff137 <ff137@proton.me>
1 parent b0696aa commit 5fe058f

5 files changed

Lines changed: 147 additions & 58 deletions

File tree

‎acapy_agent/revocation_anoncreds/manager.py‎

Lines changed: 80 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
"""Classes to manage credential revocation."""
22

33
import logging
4-
from typing import Mapping, Optional, Sequence, Text, Tuple
4+
from collections.abc import Mapping, Sequence
5+
from typing import Optional, Text, Tuple, Type
56

67
from ..anoncreds.default.legacy_indy.registry import LegacyIndyRegistry
78
from ..anoncreds.revocation import AnonCredsRevocation
@@ -334,50 +335,91 @@ async def set_cred_revoked_state(
334335
None
335336
336337
"""
337-
for cred_rev_id in cred_rev_ids:
338-
cred_ex_id = None
339-
340-
try:
341-
async with self._profile.transaction() as txn:
342-
rev_rec = await IssuerCredRevRecord.retrieve_by_ids(
343-
txn, rev_reg_id, str(cred_rev_id), for_update=True
344-
)
345-
cred_ex_id = rev_rec.cred_ex_id
346-
cred_ex_version = rev_rec.cred_ex_version
347-
rev_rec.state = IssuerCredRevRecord.STATE_REVOKED
348-
await rev_rec.save(txn, reason="revoke credential")
349-
await txn.commit()
350-
except StorageNotFoundError:
351-
continue
338+
self._logger.debug(
339+
"Setting credential revoked state for %d credentials in rev_reg_id=%s",
340+
len(cred_rev_ids),
341+
rev_reg_id,
342+
)
343+
cred_rev_ids = [str(_id) for _id in cred_rev_ids] # Method expects strings
344+
updated_cred_rev_ids = [] # Track updated to know if any were not found
345+
346+
async with self._profile.transaction() as txn:
347+
# Retrieve all requested credential revocation records
348+
cred_rev_records = await IssuerCredRevRecord.retrieve_by_ids(
349+
txn, rev_reg_id, cred_rev_ids, for_update=True
350+
)
351+
352+
# Update each record to indicate revoked
353+
for record in cred_rev_records:
354+
cred_rev_id = record.cred_rev_id
355+
self._logger.debug(
356+
"Updating IssuerCredRevRecord for cred_rev_id=%s", cred_rev_id
357+
)
358+
record.state = IssuerCredRevRecord.STATE_REVOKED
359+
await record.save(txn, reason="revoke credential")
360+
self._logger.debug(
361+
"Updated IssuerCredRevRecord state to REVOKED for cred_rev_id=%s",
362+
cred_rev_id,
363+
)
364+
updated_cred_rev_ids.append(cred_rev_id)
365+
366+
await txn.commit()
367+
368+
# Check if any requested cred rev ids were not found
369+
missing_cred_rev_ids = [
370+
_id for _id in cred_rev_ids if _id not in updated_cred_rev_ids
371+
]
372+
if missing_cred_rev_ids:
373+
self._logger.warning(
374+
"IssuerCredRevRecord not found for cred_rev_id=%s. Could not revoke.",
375+
missing_cred_rev_ids,
376+
)
352377

378+
# Map cred_ex_version to the record type
379+
_cred_ex_version_map: dict[str, Type[V10CredentialExchange | V20CredExRecord]] = {
380+
IssuerCredRevRecord.VERSION_1: V10CredentialExchange,
381+
IssuerCredRevRecord.VERSION_2: V20CredExRecord,
382+
}
383+
384+
# Update CredEx records for each credential revocation record
385+
for cred_rev_record in cred_rev_records:
353386
async with self._profile.transaction() as txn:
354-
if (
355-
not cred_ex_version
356-
or cred_ex_version == IssuerCredRevRecord.VERSION_1
357-
):
387+
cred_ex_id = cred_rev_record.cred_ex_id
388+
cred_ex_version = cred_rev_record.cred_ex_version
389+
known_record_type = _cred_ex_version_map.get(cred_ex_version)
390+
391+
# If we know the record type, use it, otherwise try both V1 and V2
392+
record_type_to_use = (
393+
[known_record_type]
394+
if known_record_type
395+
else [V10CredentialExchange, V20CredExRecord]
396+
)
397+
398+
for record_type in record_type_to_use:
358399
try:
359-
cred_ex_record = await V10CredentialExchange.retrieve_by_id(
400+
cred_ex_record = await record_type.retrieve_by_id(
360401
txn, cred_ex_id, for_update=True
361402
)
362-
cred_ex_record.state = (
363-
V10CredentialExchange.STATE_CREDENTIAL_REVOKED
364-
)
403+
404+
# Update cred ex record state to indicate revoked
405+
cred_ex_record.state = record_type.STATE_CREDENTIAL_REVOKED
365406
await cred_ex_record.save(txn, reason="revoke credential")
366-
await txn.commit()
367-
continue # skip 2.0 record check
368-
except StorageNotFoundError:
369-
pass
370407

371-
if (
372-
not cred_ex_version
373-
or cred_ex_version == IssuerCredRevRecord.VERSION_2
374-
):
375-
try:
376-
cred_ex_record = await V20CredExRecord.retrieve_by_id(
377-
txn, cred_ex_id, for_update=True
408+
self._logger.debug(
409+
"Updated %s state to REVOKED for cred_ex_id=%s",
410+
record_type.__name__,
411+
cred_ex_id,
378412
)
379-
cred_ex_record.state = V20CredExRecord.STATE_CREDENTIAL_REVOKED
380-
await cred_ex_record.save(txn, reason="revoke credential")
381413
await txn.commit()
414+
break # Record found, no need to check other record type
382415
except StorageNotFoundError:
383-
pass
416+
# Credential Exchange records may have been deleted, which is fine
417+
self._logger.debug(
418+
"%s not found for cred_ex_id=%s / cred_rev_id=%s.%s",
419+
record_type.__name__,
420+
cred_ex_id,
421+
cred_rev_record.cred_rev_id,
422+
" Checking next record type."
423+
if not known_record_type
424+
else "",
425+
)

‎acapy_agent/revocation_anoncreds/models/issuer_cred_rev_record.py‎

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
"""Issuer credential revocation information."""
22

3-
from typing import Any, Optional, Sequence
3+
import json
4+
from collections.abc import Sequence
5+
from typing import Any, List, Optional
46

57
from marshmallow import fields
68

79
from ...core.profile import ProfileSession
810
from ...messaging.models.base_record import BaseRecord, BaseRecordSchema
911
from ...messaging.valid import UUID4_EXAMPLE
12+
from ...storage.base import BaseStorage
1013

1114

1215
class IssuerCredRevRecord(BaseRecord):
@@ -90,18 +93,34 @@ async def retrieve_by_ids(
9093
cls,
9194
session: ProfileSession,
9295
rev_reg_id: str,
93-
cred_rev_id: str,
96+
cred_rev_id: str | List[str],
9497
*,
9598
for_update: bool = False,
96-
) -> "IssuerCredRevRecord":
97-
"""Retrieve an issuer cred rev record by rev reg id and cred rev id."""
98-
return await cls.retrieve_by_tag_filter(
99-
session,
100-
{"rev_reg_id": rev_reg_id},
101-
{"cred_rev_id": cred_rev_id},
102-
for_update=for_update,
99+
) -> Sequence["IssuerCredRevRecord"]:
100+
"""Retrieve a list of issuer cred rev records by rev reg id and cred rev ids."""
101+
cred_rev_ids = [cred_rev_id] if isinstance(cred_rev_id, str) else cred_rev_id
102+
103+
tag_query = {
104+
"rev_reg_id": rev_reg_id,
105+
"cred_rev_id": {"$in": cred_rev_ids},
106+
}
107+
108+
storage = session.inject(BaseStorage)
109+
storage_records = await storage.find_all_records(
110+
cls.RECORD_TYPE,
111+
tag_query,
112+
options={
113+
"for_update": for_update,
114+
},
103115
)
104116

117+
rev_reg_records = [
118+
cls.from_storage(record.id, json.loads(record.value))
119+
for record in storage_records
120+
]
121+
122+
return rev_reg_records
123+
105124
@classmethod
106125
async def retrieve_by_cred_ex_id(
107126
cls,

‎acapy_agent/revocation_anoncreds/models/tests/test_issuer_cred_rev_record.py‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
from unittest import IsolatedAsyncioTestCase
22

3-
from ....storage.base import StorageNotFoundError
43
from ....utils.testing import create_test_profile
54
from .. import issuer_cred_rev_record as test_module
65
from ..issuer_cred_rev_record import IssuerCredRevRecord
@@ -84,13 +83,30 @@ async def test_rec_ops(self):
8483
)
8584
)
8685

86+
await recs[1].set_state( # Save extra record
87+
session,
88+
IssuerCredRevRecord.STATE_REVOKED,
89+
)
90+
# Fetch cred rev id as string
91+
assert await IssuerCredRevRecord.retrieve_by_ids(
92+
session, rev_reg_id=REV_REG_ID, cred_rev_id="1"
93+
) == [recs[0]]
94+
95+
# Fetch cred rev id as list
96+
assert await IssuerCredRevRecord.retrieve_by_ids(
97+
session, rev_reg_id=REV_REG_ID, cred_rev_id=["2"]
98+
) == [recs[1]]
99+
100+
# Fetch both
87101
assert (
88102
await IssuerCredRevRecord.retrieve_by_ids(
89-
session, rev_reg_id=REV_REG_ID, cred_rev_id="1"
103+
session, rev_reg_id=REV_REG_ID, cred_rev_id=["1", "2"]
90104
)
91-
== recs[0]
92-
)
93-
with self.assertRaises(StorageNotFoundError):
105+
) == recs
106+
107+
# Fetch cred rev id that doesn't exist
108+
assert (
94109
await IssuerCredRevRecord.retrieve_by_ids(
95-
session, rev_reg_id=REV_REG_ID, cred_rev_id="2"
110+
session, rev_reg_id=REV_REG_ID, cred_rev_id=["3"]
96111
)
112+
) == []

‎acapy_agent/revocation_anoncreds/routes.py‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@
5757
IssuerRevRegRecord,
5858
IssuerRevRegRecordSchema,
5959
)
60-
from ..storage.error import StorageError, StorageNotFoundError
60+
from ..storage.error import StorageDuplicateError, StorageError, StorageNotFoundError
6161
from ..utils.profiles import is_not_anoncreds_profile_raise_web_exception
6262
from .manager import RevocationManager, RevocationManagerError
6363
from .models.issuer_cred_rev_record import (
@@ -961,9 +961,21 @@ async def get_cred_rev_record(request: web.BaseRequest):
961961
try:
962962
async with profile.session() as session:
963963
if rev_reg_id and cred_rev_id:
964-
rec = await IssuerCredRevRecord.retrieve_by_ids(
964+
recs = await IssuerCredRevRecord.retrieve_by_ids(
965965
session, rev_reg_id, cred_rev_id
966966
)
967+
if len(recs) == 1:
968+
rec = recs[0]
969+
elif len(recs) > 1:
970+
raise StorageDuplicateError(
971+
f"Multiple records found for rev_reg_id: {rev_reg_id} "
972+
f"and cred_rev_id: {cred_rev_id}"
973+
)
974+
else:
975+
raise StorageNotFoundError(
976+
f"No record found for rev_reg_id: {rev_reg_id} "
977+
f"and cred_rev_id: {cred_rev_id}"
978+
)
967979
else:
968980
rec = await IssuerCredRevRecord.retrieve_by_cred_ex_id(
969981
session, cred_ex_id

‎acapy_agent/revocation_anoncreds/tests/test_routes.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -348,9 +348,9 @@ async def test_get_cred_rev_record(self):
348348
test_module.web, "json_response", mock.Mock()
349349
) as mock_json_response,
350350
):
351-
mock_retrieve.return_value = mock.MagicMock(
352-
serialize=mock.MagicMock(return_value="dummy")
353-
)
351+
mock_retrieve.return_value = [
352+
mock.MagicMock(serialize=mock.MagicMock(return_value="dummy"))
353+
]
354354
result = await test_module.get_cred_rev_record(self.request)
355355

356356
mock_json_response.assert_called_once_with({"result": "dummy"})

0 commit comments

Comments
 (0)