Empty ServiceDecorator in OobRecord Response causes 422 Unprocessable Entity Error

[ff137]

The ACA swagger spec provides the following spec for OobRecord:

class OobRecord(BaseModel):
    """NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
...
    invi_msg_id: str
    invitation: InvitationMessage
    oob_id: str
    state: Literal["initial","prepare-response","await-response","reuse-not-accepted","reuse-accepted","done","deleted"]
    attach_thread_id: Optional[str] = None
    connection_id: Optional[str] = None
    created_at: Optional[str] = None
    our_recipient_key: Optional[str] = None
    role: Optional[Literal["sender", "receiver"]] = None
    their_service: Optional[ServiceDecorator] = None
    trace: Optional[bool] = None
    updated_at: Optional[str] = None

I draw your attention to: their_service: Optional[ServiceDecorator]

For ServiceDecorator we have the following:

class ServiceDecorator(BaseModel):
    """NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
...
    recipient_keys: List[str] = Field(..., alias="recipientKeys")
    service_endpoint: str = Field(..., alias="serviceEndpoint")
    routing_keys: Optional[List[str]] = Field(None, alias="routingKeys")

What you'll notice is that recipientKeys and serviceEndpoint are required when providing a ServiceDecorator.

However, when receiving an OobRecord response from receive_invitation, we've encountered 422 Unprocessable Entity due to the response providing an empty their_service field.

We could not yet intercept the body of the message before parsing into the response object, but the error message indicates:

b'{"detail":[{"loc":["their_service","recipientKeys"],"msg":"none is not an allowed value","type":"type_error.none.not_allowed"},{"loc":["their_service","serviceEndpoint"],"msg":"none is not an allowed value","type":"type_error.none.not_allowed"}]}'

In a nutshell: we're receiving a their_service, but with None for its required fields.

This occurs when creating a connectionless invitation in a multitenant environment, and so perhaps this particular case was missed.

A simple workaround for us is to manually make the ServiceDecorator fields optional:

    recipient_keys: Optional[List[str]] = Field(None, alias="recipientKeys")
    service_endpoint: Optional[str] = Field(None, alias="serviceEndpoint")
    routing_keys: Optional[List[str]] = Field(None, alias="routingKeys")

But of course, this is malforming the ServiceDecorator class. It would be more appropriate for their_service to be None, when its required fields are empty.

I'd be happy to look into making a PR for this if someone can direct me to the relevant code. Thanks!

[swcurran]

Thanks for the detail. I’m not sure where this change would be made. Perhaps @ianco or @jcourt562 could provide a pointer?

[ff137]

I've looked at this again, and I believe I've traced the source of the problem.

To recap, the ServiceDecorator has required fields for endpoint and recipient_keys.
I presume there is good reason for this. Because it's possible to just make those optional, but let's rather not.

I've looked at all the instances where a ServiceDecorator is referenced in the codebase, and I notice the following in _service_decorator_from_service, in the OutOfBand manager:

    async def _service_decorator_from_service(
        self, service: Union[Service, str]
    ) -> ServiceDecorator:
        if isinstance(service, str):
            (
                endpoint,
                recipient_keys,
                routing_keys,
            ) = await self.resolve_invitation(service)

            return ServiceDecorator(
                endpoint=endpoint,
                recipient_keys=recipient_keys,
                routing_keys=routing_keys,
            )
        else:
            # Create ~service decorator from the oob service
            recipient_keys = [
                DIDKey.from_did(did_key).public_key_b58
                for did_key in service.recipient_keys
            ]
            routing_keys = [
                DIDKey.from_did(did_key).public_key_b58
                for did_key in service.routing_keys
            ]

            return ServiceDecorator(
                endpoint=service.service_endpoint,
                recipient_keys=recipient_keys,
                routing_keys=routing_keys,
            )

The else block seems to have the problem.

The self.resolve_invitation(service) in the if block seems to be safe - it interacts with the Service definitions from pydid:

class Service(Resource):
    """Representation of DID Document Services."""

    id: DIDUrl
    type: str
    service_endpoint: Union[DIDUrl, AnyUrl, Literal[""], List[ServiceEndpoint]]

which will always return a string for service_endpoint - at worst an empty string "".

The else block, however, seems that it can potentially introduce nulls into the ServiceDecorator object, because service.service_endpoint can be None (that's the default value):

class Service(BaseModel):
...
    def __init__(
...
        service_endpoint: str = None,
    ):

As an aside, it's a bit of a code smell to cast None to a str ... it should actually be a Optional[str]. Because the compiler indicates it's a string, when its default is actually None.

My proposal is that the following should only be attempted if service.service_endpoint is not None:

            return ServiceDecorator(
                endpoint=service.service_endpoint,
                recipient_keys=recipient_keys,
                routing_keys=routing_keys,
            )

It seems reasonable to me that if the endpoint is None, then we should return None, instead of a ServiceDecorator. Because the only place the result is used, is to call handle_message in the OobMessageProcessor, which expects their_service to be an Optional ServiceDecorator:

    async def handle_message(
        self,
        profile: Profile,
        messages: List[Dict[str, Any]],
        oob_record: OobRecord,
        their_service: Optional[ServiceDecorator] = None,
    ):

If their_service has an empty endpoint, then a ServiceDecorator object shouldn't be necessary. I feel it should be the same if the endpoint is an empty string.

@dbluhm - it seems you helped with the Service implementation.
I know this looks like a lot of info to ask about, but it's just to show my working.
What do you think would be reasonable solution for how empty (none or "") endpoints should be handled?

Crux of the issue is that, at the moment, the OobRecord response made in OutOfBandManager.receive_invitation can provide a malformed response (trying to define a ServiceDecorator with a None for required fields). Seems to me like their_service should just be None in those cases.

[dbluhm]

As an aside, it's a bit of a code smell to cast None to a str ... it should actually be a Optional[str]. Because the compiler indicates it's a string, when its default is actually None.

Yes, unfortunately, this is all over ACA-Py. Before the type checking tools really matured to where they are today, it was a common convention (at least in ACA-Py) for Optional values to be expressed as my_value: str = None. Pyright/Pylance, Mypy, etc. now complain about this. I correct it as I come across these but it's still quite prevalent in the code base.

What do you think would be reasonable solution for how empty (none or "") endpoints should be handled?

The empty string serviceEndpoint was an early mechanism for indicating to the receiver that the sender did not have a publicly accessible endpoint. This should eventually be phased out in favor of more explicit mechanisms such as didcomm:transport/queue as recommended in the DIDComm v2 extension on return routing.

I'm curious, how is the connectionless invitation constructed that is resulting in these issues? If there isn't a valid service endpoint to message, it seems like it will ultimately result in an error still since the receiver wouldn't be able to respond to it.

I agree that the types in these methods needs to be tightened up and perhaps returning a None is the right answer when there is no valid service endpoint passed to that _service_decorator_from_service helper.

[ff137]

@dbluhm I've made some amendments to fix this in #2362 . Please let me know what you think