search

TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
961 stars 303 forks source link

API Boundary: Validation and Cleaning #69

Closed johanstokking closed 5 years ago

johanstokking commented 5 years ago

Summary:

This is an umbrella issue that covers validation of requests in the API boundary, and the contracts with components beind the API boundary.

Why do we need this?

The idea, which came from this article, is that we introduce validators on each message, responsible for validating and cleaning incoming data.

What is already there? What do you see now?

  1. We have validation middleware that validates the requested field mask paths for each RPC (#167)
  2. We have generated validators (generated with mwitkow/go-proto-validators) that validate our protos against rules defined in our protos.
  3. We have the validation middleware that calls the generated Validate() error or ValidateContext(ctx) error

What is missing? What do you want to see?

  1. Use generated validators (generated with github.com/TheThingsIndustries/protoc-gen-validate) to validate specific fields in our protos against rules defined in our protos (and not the entire message if only few fields are masked).
  2. Implement Validate() error or ValidateContext(ctx) error on all request messages, and make that func call ValidateFields(...string) error with the appropriate fields. On Get-like requests this means the fields of the request itself. On Set-like request this means the fields of the entity being updated.
  3. On Set-like requests, unset any fields that are set in the entity in the request, but not specified in the field mask.
  4. Add assertions to ensure that all request messages implement these validators.

How do you propose to implement this?

  1. 194

  2. We could add this to #194 in the way described above
  3. Something like this: 
    cleaned := new(T)
cleaned.SetFields(req.T, req.FieldMask.Paths...)
req.T = cleaned
  4. We can just make the Validator middleware error if request messages don't implement them

Original issue: https://github.com/TheThingsIndustries/lorawan-stack/issues/1058 by @htdvisser

htdvisser commented 5 years ago

Here are the formalized responsibilities of the API Boundary and the individual components.

API Boundary: (A1) The API boundary validators are responsible for making sure that for each path specified in the request's field mask, the corresponding field should either have a valid value, or no value at all. (A2) The API boundary may unset any fields that are not specified in the field mask. (A3) The ids path is implicitly part of the field mask. (A4) As far as the API boundary is concerned, only the TTN identifiers are mandatory (so not the EUIs in end devices, also note that the JS's GetByEUI's are not in our gRPC API). (A5) For each RPC a list of field mask paths that are allowed to be in the request message can be registered. The API boundary will then validate the field mask paths in the request message. (#167)

Components: (C1) Are responsible for validating presence non-TTN identifiers if required. (C2) Are responsible for validating that the result of an operation results in a valid state (i.e. it does not leave mandatory fields unset, and it does not leave related fields in an invalid state).

htdvisser commented 5 years ago

Updated issue description

htdvisser commented 5 years ago

I don't think everything in this umbrella issue is actually done with #194 getting merged.

cc: @KrishnaIyer @rvolosatovs

rvolosatovs commented 5 years ago

Can you clarify what isn't?

htdvisser commented 5 years ago

I don't know, but if we can answer all questions below with "yes", then I guess we have everything.

With the numbers of What is missing? What do you want to see?:

2: Is Validate() error or ValidateContext(ctx) error really implemented on all request messages now? 3: Are field that aren't specified in the field mask now unset on Set-like operations? 4: Do we have these assertions?

From https://github.com/TheThingsNetwork/lorawan-stack/issues/69#issuecomment-465184422:

A1: Are all fields now validated with (generated) validators?

rvolosatovs commented 5 years ago
  1. We don't need Validate() error or ValidateContext(context.Context) error on all requests, since we just call ValidateFields() if they're not. All requests with fieldmasks have a ValidateContext(context.Context) error defined
  2. That's quite tricky and we may want to avoid doing this. The issue is the same as with the validators - there's no generic way to determine if this cleaning has to be performed on the field of a message and the field this cleaning has to be performed on. IMO it makes more sense to just do a req.Field.SetFields(req.FieldMask.Paths...) at the top of the RPC, which needs this. Otherwise, for consistency sake, we would need to come up with a new method signature and handle it accordingly, which seems to just be a waste of time.
  3. https://github.com/TheThingsNetwork/lorawan-stack/pull/299
johanstokking commented 5 years ago
  1. Isn't this what the allowed field mask paths validator is doing? Or am I missing something here?
rvolosatovs commented 5 years ago

@johanstokking no, it's about unsetting the fields in the message itself. E.g. keys.app_s_key in EndDevice if keys.app_s_key is not specified in the fieldmask.

htdvisser commented 5 years ago

Ok, so it looks like all we have left from this umbrella is point (3) from my comment above. I agree with @rvolosatovs that implementing this in the generic part of the API boundary would be quite a lot for work, while it would be only a few lines in RPCs.

With that, I think we can close this issue.