Merge pull request openwallet-foundation#1827 from andrewwhitehead/fix/upd-crd-rev

Fix IssuerCredRevRecord state update on revocation publish

Author: swcurran

aries_cloudagent/indy/sdk/issuer.py

@@ -283,7 +283,7 @@ async def revoke_credentials(
         tails_reader_handle = await create_tails_reader(tails_file_path)
 
         result_json = None
-        for cred_rev_id in cred_rev_ids:
+        for cred_rev_id in set(cred_rev_ids):
             with IndyErrorHandler(
                 "Exception when revoking credential", IndyIssuerError
             ):

aries_cloudagent/indy/sdk/tests/test_issuer.py

@@ -311,17 +311,20 @@ async def test_create_revoke_credentials_x(
             values = json.loads(call_values)
             assert "attr1" in values
 
-            mock_indy_revoke_credential.side_effect = [
-                json.dumps(TEST_RR_DELTA),
-                IndyError(
-                    error_code=ErrorCode.AnoncredsInvalidUserRevocId,
-                    error_details={"message": "already revoked"},
-                ),
-                IndyError(
+            def mock_revoke(_h, _t, _r, cred_rev_id):
+                if cred_rev_id == "42":
+                    return json.dumps(TEST_RR_DELTA)
+                if cred_rev_id == "54":
+                    raise IndyError(
+                        error_code=ErrorCode.AnoncredsInvalidUserRevocId,
+                        error_details={"message": "already revoked"},
+                    )
+                raise IndyError(
                     error_code=ErrorCode.UnknownCryptoTypeError,
                     error_details={"message": "truly an outlier"},
-                ),
-            ]
+                )
+
+            mock_indy_revoke_credential.side_effect = mock_revoke
             mock_indy_merge_rr_deltas.return_value = json.dumps(TEST_RR_DELTA)
             (result, failed) = await self.issuer.revoke_credentials(
                 REV_REG_ID, tails_file_path="dummy", cred_rev_ids=test_cred_rev_ids

aries_cloudagent/revocation/manager.py

@@ -133,7 +133,7 @@ async def revoke_credential(
             rev_reg = await revoc.get_ledger_registry(rev_reg_id)
             await rev_reg.get_or_fetch_local_tails_path()
             # pick up pending revocations on input revocation registry
-            crids = list(set(issuer_rr_rec.pending_pub + [cred_rev_id]))
+            crids = (issuer_rr_rec.pending_pub or []) + [cred_rev_id]
             (delta_json, _) = await issuer.revoke_credentials(
                 issuer_rr_rec.revoc_reg_id, issuer_rr_rec.tails_local_path, crids
             )
@@ -145,9 +145,9 @@ async def revoke_credential(
                     issuer_rr_upd.revoc_reg_entry = json.loads(delta_json)
                 await issuer_rr_upd.clear_pending(txn, crids)
                 await txn.commit()
+            await self.set_cred_revoked_state(rev_reg_id, crids)
             if delta_json:
                 await issuer_rr_upd.send_entry(self._profile)
-            await self.set_cred_revoked_state(rev_reg_id, [cred_rev_id])
             await notify_revocation_published_event(
                 self._profile, rev_reg_id, [cred_rev_id]
             )
@@ -215,11 +215,11 @@ async def publish_pending_revocations(
                         issuer_rr_upd.revoc_reg_entry = json.loads(delta_json)
                     await issuer_rr_upd.clear_pending(txn, crids)
                     await txn.commit()
+                await self.set_cred_revoked_state(issuer_rr_rec.revoc_reg_id, crids)
                 if delta_json:
                     await issuer_rr_upd.send_entry(self._profile)
                 published = sorted(crid for crid in crids if crid not in failed_crids)
                 result[issuer_rr_rec.revoc_reg_id] = published
-                await self.set_cred_revoked_state(issuer_rr_rec.revoc_reg_id, crids)
                 await notify_revocation_published_event(
                     self._profile, issuer_rr_rec.revoc_reg_id, crids
                 )
@@ -289,34 +289,40 @@ async def set_cred_revoked_state(
 
         """
         for cred_rev_id in cred_rev_ids:
+            cred_ex_id = None
+
+            try:
+                async with self._profile.transaction() as txn:
+                    rev_rec = await IssuerCredRevRecord.retrieve_by_ids(
+                        txn, rev_reg_id, cred_rev_id, for_update=True
+                    )
+                    cred_ex_id = rev_rec.cred_ex_id
+                    rev_rec.state = IssuerCredRevRecord.STATE_REVOKED
+                    await rev_rec.save(txn, reason="revoke credential")
+                    await txn.commit()
+            except StorageNotFoundError:
+                continue
+
             async with self._profile.transaction() as txn:
                 try:
-                    rev_rec = await IssuerCredRevRecord.retrieve_by_ids(
-                        txn, rev_reg_id, cred_rev_id
+                    cred_ex_record = await V10CredentialExchange.retrieve_by_id(
+                        txn, cred_ex_id, for_update=True
+                    )
+                    cred_ex_record.state = (
+                        V10CredentialExchange.STATE_CREDENTIAL_REVOKED
                     )
-                    try:
-                        cred_ex_record = await V10CredentialExchange.retrieve_by_id(
-                            txn, rev_rec.cred_ex_id, for_update=True
-                        )
-                        cred_ex_record.state = (
-                            V10CredentialExchange.STATE_CREDENTIAL_REVOKED
-                        )
-                        await cred_ex_record.save(txn, reason="revoke credential")
-                        await txn.commit()
-
-                    except StorageNotFoundError:
-                        try:
-                            cred_ex_record = await V20CredExRecord.retrieve_by_id(
-                                txn, rev_rec.cred_ex_id, for_update=True
-                            )
-                            cred_ex_record.state = (
-                                V20CredExRecord.STATE_CREDENTIAL_REVOKED
-                            )
-                            await cred_ex_record.save(txn, reason="revoke credential")
-                            await txn.commit()
-
-                        except StorageNotFoundError:
-                            pass
+                    await cred_ex_record.save(txn, reason="revoke credential")
+                    await txn.commit()
+                    continue  # skip 2.0 record check
+                except StorageNotFoundError:
+                    pass
 
+                try:
+                    cred_ex_record = await V20CredExRecord.retrieve_by_id(
+                        txn, cred_ex_id, for_update=True
+                    )
+                    cred_ex_record.state = V20CredExRecord.STATE_CREDENTIAL_REVOKED
+                    await cred_ex_record.save(txn, reason="revoke credential")
+                    await txn.commit()
                 except StorageNotFoundError:
                     pass

aries_cloudagent/revocation/models/issuer_cred_rev_record.py

@@ -90,10 +90,15 @@ async def retrieve_by_ids(
         session: ProfileSession,
         rev_reg_id: str,
         cred_rev_id: str,
+        *,
+        for_update: bool = False,
     ) -> "IssuerCredRevRecord":
         """Retrieve an issuer cred rev record by rev reg id and cred rev id."""
         return await cls.retrieve_by_tag_filter(
-            session, {"rev_reg_id": rev_reg_id}, {"cred_rev_id": cred_rev_id}
+            session,
+            {"rev_reg_id": rev_reg_id},
+            {"cred_rev_id": cred_rev_id},
+            for_update=for_update,
         )
 
     @classmethod

aries_cloudagent/revocation/tests/test_manager.py

@@ -4,6 +4,10 @@
 from asynctest import TestCase as AsyncTestCase
 from more_itertools import side_effect
 
+from aries_cloudagent.revocation.models.issuer_cred_rev_record import (
+    IssuerCredRevRecord,
+)
+
 from ...core.in_memory import InMemoryProfile
 from ...indy.issuer import IndyIssuer
 from ...protocols.issue_credential.v1_0.models.credential_exchange import (
@@ -38,7 +42,25 @@ async def test_revoke_credential_publish(self):
             tails_local_path=TAILS_LOCAL,
             send_entry=async_mock.CoroutineMock(),
             clear_pending=async_mock.CoroutineMock(),
+            pending_pub=["2"],
         )
+        issuer = async_mock.MagicMock(IndyIssuer, autospec=True)
+        issuer.revoke_credentials = async_mock.CoroutineMock(
+            return_value=(
+                json.dumps(
+                    {
+                        "ver": "1.0",
+                        "value": {
+                            "prevAccum": "1 ...",
+                            "accum": "21 ...",
+                            "issued": [1],
+                        },
+                    }
+                ),
+                [],
+            )
+        )
+        self.profile.context.injector.bind_instance(IndyIssuer, issuer)
 
         with async_mock.patch.object(
             test_module.IssuerCredRevRecord,
@@ -64,26 +86,14 @@ async def test_revoke_credential_publish(self):
                 return_value=mock_rev_reg
             )
 
-            issuer = async_mock.MagicMock(IndyIssuer, autospec=True)
-            issuer.revoke_credentials = async_mock.CoroutineMock(
-                return_value=(
-                    json.dumps(
-                        {
-                            "ver": "1.0",
-                            "value": {
-                                "prevAccum": "1 ...",
-                                "accum": "21 ...",
-                                "issued": [1],
-                            },
-                        }
-                    ),
-                    [],
-                )
-            )
-            self.profile.context.injector.bind_instance(IndyIssuer, issuer)
-
             await self.manager.revoke_credential_by_cred_ex_id(CRED_EX_ID, publish=True)
 
+        issuer.revoke_credentials.assert_awaited_once_with(
+            mock_issuer_rev_reg_record.revoc_reg_id,
+            mock_issuer_rev_reg_record.tails_local_path,
+            ["2", "1"],
+        )
+
     async def test_revoke_cred_by_cxid_not_found(self):
         CRED_EX_ID = "dummy-cxid"
 
@@ -128,6 +138,8 @@ async def test_revoke_credential_pend(self):
         mock_issuer_rev_reg_record = async_mock.MagicMock(
             mark_pending=async_mock.CoroutineMock()
         )
+        issuer = async_mock.MagicMock(IndyIssuer, autospec=True)
+        self.profile.context.injector.bind_instance(IndyIssuer, issuer)
 
         with async_mock.patch.object(
             test_module, "IndyRevocation", autospec=True
@@ -148,14 +160,13 @@ async def test_revoke_credential_pend(self):
                 return_value=mock_issuer_rev_reg_record
             )
 
-            issuer = async_mock.MagicMock(IndyIssuer, autospec=True)
-            self.profile.context.injector.bind_instance(IndyIssuer, issuer)
-
             await self.manager.revoke_credential(REV_REG_ID, CRED_REV_ID, False)
             mock_issuer_rev_reg_record.mark_pending.assert_called_once_with(
                 session.return_value, CRED_REV_ID
             )
 
+        issuer.revoke_credentials.assert_not_awaited()
+
     async def test_publish_pending_revocations_basic(self):
         deltas = [
             {
@@ -415,3 +426,43 @@ async def test_retrieve_records(self):
                 )
                 assert ret_ex.connection_id == str(index)
                 assert ret_ex.thread_id == str(1000 + index)
+
+    async def test_set_revoked_state(self):
+        CRED_REV_ID = "1"
+
+        async with self.profile.session() as session:
+            exchange_record = V10CredentialExchange(
+                connection_id="mark-revoked-cid",
+                thread_id="mark-revoked-tid",
+                initiator=V10CredentialExchange.INITIATOR_SELF,
+                revoc_reg_id=REV_REG_ID,
+                revocation_id=CRED_REV_ID,
+                role=V10CredentialExchange.ROLE_ISSUER,
+                state=V10CredentialExchange.STATE_ISSUED,
+            )
+            await exchange_record.save(session)
+
+            crev_record = IssuerCredRevRecord(
+                cred_ex_id=exchange_record.credential_exchange_id,
+                cred_def_id=CRED_DEF_ID,
+                rev_reg_id=REV_REG_ID,
+                cred_rev_id=CRED_REV_ID,
+                state=IssuerCredRevRecord.STATE_ISSUED,
+            )
+            await crev_record.save(session)
+
+        await self.manager.set_cred_revoked_state(REV_REG_ID, [CRED_REV_ID])
+
+        async with self.profile.session() as session:
+            check_exchange_record = await V10CredentialExchange.retrieve_by_id(
+                session, exchange_record.credential_exchange_id
+            )
+            assert (
+                check_exchange_record.state
+                == V10CredentialExchange.STATE_CREDENTIAL_REVOKED
+            )
+
+            check_crev_record = await IssuerCredRevRecord.retrieve_by_id(
+                session, crev_record.record_id
+            )
+            assert check_crev_record.state == IssuerCredRevRecord.STATE_REVOKED