Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix OOB proof-request generation #444

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

esune
Copy link
Member

@esune esune commented Mar 22, 2024

Tweaks required after the previous PR setting the local DID for the agent

@esune esune requested review from loneil and Gavinok March 22, 2024 19:00
Gavinok
Gavinok previously approved these changes Mar 22, 2024
Copy link
Contributor

@Gavinok Gavinok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

docker/manage Outdated Show resolved Hide resolved
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we have an issue here now if USE_OOB_PRESENT_PROOF is False

I'm getting an error if running locally trying to use the existing connectionless if I set OOB to False

controller-1     | {"event": "Traceback (most recent call last):\n  File \"/usr/local/lib/python3.11/site-packages/starlette/middleware/base.py\", line 78, in call_next\n    message = await recv_stream.receive()\n              ^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/anyio/streams/memory.py\", line 97, in receive\n    return self.receive_nowait()\n           ^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/anyio/streams/memory.py\", line 90, in receive_nowait\n    raise EndOfStream\nanyio.EndOfStream\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n  File \"/app/api/main.py\", line 90, in logging_middleware\n    response: Response = await call_next(request)\n                         ^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/starlette/middleware/base.py\", line 84, in call_next\n    raise app_exc\n  File \"/usr/local/lib/python3.11/site-packages/starlette/middleware/base.py\", line 70, in coro\n    await self.app(scope, receive_or_disconnect, send_no_error)\n  File \"/usr/local/lib/python3.11/site-packages/starlette/middleware/cors.py\", line 83, in __call__\n    await self.app(scope, receive, send)\n  File \"/usr/local/lib/python3.11/site-packages/starlette/middleware/exceptions.py\", line 79, in __call__\n    raise exc\n  File \"/usr/local/lib/python3.11/site-packages/starlette/middleware/exceptions.py\", line 68, in __call__\n    await self.app(scope, receive, sender)\n  File \"/usr/local/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py\", line 21, in __call__\n    raise e\n  File \"/usr/local/lib/python3.11/site-packages/fastapi/middleware/asyncexitstack.py\", line 18, in __call__\n    await self.app(scope, receive, send)\n  File \"/usr/local/lib/python3.11/site-packages/starlette/routing.py\", line 718, in __call__\n    await route.handle(scope, receive, send)\n  File \"/usr/local/lib/python3.11/site-packages/starlette/routing.py\", line 276, in handle\n    await self.app(scope, receive, send)\n  File \"/usr/local/lib/python3.11/site-packages/starlette/routing.py\", line 66, in app\n    response = await func(request)\n               ^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/fastapi/routing.py\", line 237, in app\n    raw_response = await run_endpoint_function(\n                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/fastapi/routing.py\", line 163, in run_endpoint_function\n    return await dependant.call(**values)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/app/api/routers/oidc.py\", line 116, in get_authorize\n    response = client.create_presentation_request(ver_config.generate_proof_request())\n               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/app/api/core/acapy/client.py\", line 47, in create_presentation_request\n    resp_raw = requests.post(\n               ^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/requests/api.py\", line 115, in post\n    return request(\"post\", url, data=data, json=json, **kwargs)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/requests/api.py\", line 59, in request\n    return session.request(method=method, url=url, **kwargs)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/requests/sessions.py\", line 589, in request\n    resp = self.send(prep, **send_kwargs)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/requests/sessions.py\", line 703, in send\n    r = adapter.send(request, **kwargs)\n        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File \"/usr/local/lib/python3.11/site-packages/requests/adapters.py\", line 519, in send\n    raise ConnectionError(e, request=request)\nrequests.exceptions.ConnectionError: HTTPConnectionPool(host='aca-py', port=8077): Max retries exceeded with url: /present-proof/create-request (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f8c9c84ebd0>: Failed to establish a new connection: [Errno 111] Connection refused'))\n", "logger": "api.main", "level": "error", "timestamp": "2024-03-22T22:25:03.318406Z"}

Not sure if I've got something wrong locally... investigating, but putting block on merge for now.

IE running this on main

  export USE_OOB_PRESENT_PROOF="False"
  export USE_OOB_LOCAL_DID_SERVICE="False"

but on this branch I get the error trying to load the PR

@loneil
Copy link
Contributor

loneil commented Mar 22, 2024

Oh NM above, I had vars setting wrong.
So this is the case that breaks it

export USE_OOB_PRESENT_PROOF="False"
export USE_OOB_LOCAL_DID_SERVICE="True"

which may be expected?

But with the change in defaults if someone sets just USE_OOB_PRESENT_PROOF to False when running it appears to be breaking so not sure if we should take that var into account when setting up the USE_OOB_LOCAL_DID_SERVICE too?
Or just be understood to set both to false by the operator in that case.

@loneil loneil self-requested a review March 25, 2024 16:41
@esune
Copy link
Member Author

esune commented Mar 25, 2024

I thought I tested all combinations, but I didn't. trying to figure out a more real/permanent solution to this now...

@esune
Copy link
Member Author

esune commented Mar 25, 2024

Things should work now - other than actually being able to scan an OOB request in BC Wallet.

I cleaned-up a tiny bit how the DID is fetched from the wallet, and had to fix the default values for some of the flags: it turns out strtobool does not like capitalized True/False and was returning the opposite result, so fixing that actually makes the logic work as expected.

esune added 2 commits March 25, 2024 10:46
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
@esune esune force-pushed the fix/oob-proof-request branch from f356558 to 811ac68 Compare March 25, 2024 17:46
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think it's good.

Tried with export USE_OOB_PRESENT_PROOF as true and false, then running manage start and got expected behaviour locally

@esune esune merged commit 8fbdfea into openwallet-foundation:main Mar 25, 2024
5 checks passed
@esune esune deleted the fix/oob-proof-request branch March 25, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants