openservicebrokerapi / osb-checker

An automatic checker to verify an Open Service Broker API implementation against the specification
https://github.com/openservicebrokerapi/servicebroker/
Apache License 2.0
48 stars 40 forks source link

Parameters schema validation failed in Provision/Update/Bind request body #44

Closed leonwanghui closed 5 years ago

leonwanghui commented 6 years ago

According to what the spec (v2.13) indicates, parameters is JSON object that means Configuration options for the service instance. Brokers SHOULD ensure that the client has provided valid configuration parameters and values for the operation. So personally it provides no constraints for the format of content, and user can pass whatever they want into this JSON object.

But in osb-checker, there is a test case which is so compulsory:

            it ('should reject if parameters are not following schema', function(done){
                tempBody = JSON.parse(JSON.stringify(provision.body));
                tempBody.parameters = {
                    "can-not": "be-good"
                }
                preparedRequest()
                .put('/v2/service_instances/' + instance_id + "?accepts_incomplete=true")
                .set('X-Broker-API-Version', apiVersion)
                .auth(config.user, config.password)
                .send(tempBody)
                .expect(400, done)
            })

I know the error should indicate the request to the mock SB misses billing-account field, but for those service plans which don't need any parameter, it could fail to pass this test case because it's hard-coded.

Any thoughts?

fmui commented 6 years ago

I ran into this as well. It should be rather a warning than a failure.

leonwanghui commented 6 years ago

Thanks @fmui , we can discuss it next meeting : )

norshtein commented 5 years ago

Seems this test case is used to test malformed provision request. As said in spec, 400 Bad Request MUST be returned if the request is malformed or missing mandatory data. This test case adds key-value pair {"can-not": "be-good"} to parameters in the request body, and expect the service broker to return 400 Bad Request. Overall, it looks good to me...

petar-iv commented 5 years ago

To me it seems that the broker itself is not the component that should validate the provided properties against a given schema. Here is why:

Let me know if my understanding is not correct. If it is, then probably this test can be removed.

leonwanghui commented 5 years ago

Hi @petar-iv , firstly l think we can make the agreement that the validation between provided properties and a given schema is necessary, so the main issue is who should in charge of this work, platform or the broker side?

IMO the reason why indicating Brokers SHOULD ensure that the client has provided valid configuration parameters and values for the operation is that brokers provide those schema in catalog to platform, so it's more proper to let broker to validate them when broker receives parameters from provision/update/bind request.

leonwanghui commented 5 years ago

@norshtein I agree what you mentioned, but there would some errors if the parameter under catalog is empty. In that case, the test case should pass which it doesn't.

norshtein commented 5 years ago

@leonwanghui If the the broker says the parameter under catalog is empty, and then it receives a request with non-empty parameter, IMO, the request is invalid and the broker should be able to realize the request is invalid and return error code to the platform. Does that make sense?