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

Request Validator Checks readOnly Properties #71

Closed
danielgtaylor opened this issue Mar 13, 2019 · 7 comments · Fixed by #246
Closed

Request Validator Checks readOnly Properties #71

danielgtaylor opened this issue Mar 13, 2019 · 7 comments · Fixed by #246
Labels

Comments

@danielgtaylor
Copy link
Collaborator

As reported in danielgtaylor/apisprout#30, when given a request schema with a required readOnly property the request validator mistakenly treats it as required. According to the spec:

readOnly boolean
Relevant only for Schema "properties" definitions. Declares the property as "read only". This means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request. If the property is marked as readOnly being true and is in the required list, the required will take effect on the response only. A property MUST NOT be marked as both readOnly and writeOnly being true. Default value is false.

Basically this means that request & response can share a schema but will see different properties as required for validation purposes.

@danielgtaylor danielgtaylor changed the title Request Validator Mistakenly Checks readOnly Properties Request Validator Checks readOnly Properties Mar 13, 2019
@danielgtaylor
Copy link
Collaborator Author

The addition of these lines before openapi3/schema.go:1112 solves it for the request, but I'm not sure how to differentiate between the request & response in this validation code. Any ideas how you would design this feature @fenollp?

if properties[k].Value != nil && properties[k].Value.ReadOnly {
	continue
}

@fenollp
Copy link
Collaborator

fenollp commented Mar 17, 2019

Hm well this may be the time to switch from passing around this fast bool param to using https://golang.org/pkg/context/#example_WithValue & add a second boolean isRequest to it.

I would also just remove the single arguments: func (schema *Schema) VisitJSONArray(value []interface{}) error
and just introduce a func failFast(ctx) ctx that'd set the fail boolean value of ctx to true before returning ctx.

If you're up to tackle this I'd see it in 2-3 commits: the 1st one being about this large context.Context change, the rest actually about the feature & tests :)

@ntoljic
Copy link

ntoljic commented Feb 2, 2020

I just ran into this as well. Is there any news on this issue?

@fenollp
Copy link
Collaborator

fenollp commented Jul 24, 2020

Can one of you provide a failing go-run-able snippet so I can fix this quickly?
It's way easier for me that way

@jormaechea
Copy link

@fenollp I think you can find an example here: danielgtaylor/apisprout#30

@alexferl
Copy link

@fenollp here's a snippet you can run:

package main

import (
	"bytes"
	"context"
	"encoding/json"
	"fmt"
	"net/http"

	"github.com/getkin/kin-openapi/openapi3"
	"github.com/getkin/kin-openapi/openapi3filter"
)

var schema = []byte(`{
  "openapi": "3.0.3",
  "info": {
    "version": "1.0.0",
    "title": "title",
    "description": "desc",
    "contact": {
      "email": "email"
    }
  },
  "paths": {
    "/accounts": {
      "post": {
        "description": "Create a new account",
        "requestBody": {
          "required": true,
          "content": {
            "application/json": {
              "schema": {
                "type": "object",
                "properties": {
                  "_id": {
                    "type": "string",
                    "description": "Unique identifier for this object.",
                    "pattern": "[0-9a-v]+$",
                    "minLength": 20,
                    "maxLength": 20,
                    "readOnly": true
                  }
                }
              }
            }
          }
        },
        "responses": {
          "201": {
            "description": "Successfully created a new account"
          },
          "400": {
            "description": "The server could not understand the request due to invalid syntax",
          }
        }
      }
    }
  }
}
`)

type Request struct {
	ID string `json:"_id"`
}

func main() {
	l , _:= openapi3.NewSwaggerLoader().LoadSwaggerFromData(schema)
	router := openapi3filter.NewRouter().WithSwagger(l)
	ctx := context.TODO()

	b, _ := json.Marshal(Request{ID: "bt6kdc3d0cvp6u8u3ft0"})

	httpReq, _ := http.NewRequest(http.MethodPost, "/accounts", bytes.NewReader(b))
	httpReq.Header.Add("Content-Type", "application/json")

	route, pathParams, _ := router.FindRoute(httpReq.Method, httpReq.URL)

	requestValidationInput := &openapi3filter.RequestValidationInput{
		Request:    httpReq,
		PathParams: pathParams,
		Route:      route,
	}
	if err := openapi3filter.ValidateRequest(ctx, requestValidationInput); err != nil {
		panic(err)
	}

	fmt.Println("Did not fail")
}

@fenollp
Copy link
Collaborator

fenollp commented Sep 2, 2020

Hi there,
please take a look at #246 I'm sure I got the logic wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants