feat: add validation for /groups endpoints

[babakks]

This PR adds validation for /groups endpoints to the ReBAC Admin UI library. This functionality is implemented as a decorated handler that wraps/decorates the actual handler and applies validation logic.

For now, this is only done for /groups endpoints. After this has landed, follow up PRs would be submitted for the rest of endpoints.

Note that the bulk of methods in rebac-admin-backend/v1/validation.go are there to implement the interface. I could've embedded the Unimplemented struct for this, but since these were already there, I didn't change it.

Fixes CSS-7621

[alesstimec]

LGTM with a few nitpicks

[shipperizer]

this looks really smart, but i m finding it really hard to understand

the use of generics could be dropped if u offload this to each implementation in here

while for validation_utils.go generics make a lot of sense, here i m not 100% sure, especially if we read the sections in https://go.dev/blog/when-generics on when not to use

[babakks]

I'm not sure if I'm getting your point correctly, but I don't have a strong opinion on using generics for this part. Of course, parsing/validation step can be done without generics. For example, without generics the handler methods in the handlerWithValidation struct would look change from this:

func (v handlerWithValidation) PostGroups(w http.ResponseWriter, r *http.Request) {
  setRequestBodyInContext[resources.Group](w, r, func(w http.ResponseWriter, r *http.Request, body *resources.Group) {
    if err := validateGroup(body); err != nil {
      writeErrorResponse(w, err)
      return
    }
    v.handler.PostGroups(w, r)
  })
}

to this:

func (v handlerWithValidation) PostGroups(w http.ResponseWriter, r *http.Request) {
  body := &resources.Group{}
  setRequestBodyInContext(body, w, r, func(w http.ResponseWriter, r *http.Request) {
    if err := validateGroup(body); err != nil {
      writeErrorResponse(w, err)
      return
    }
    v.handler.PostGroups(w, r)
  })
}

Is this what you're asking for?

[alesstimec]

i don't think generics are inherently bad.. i think this is a valid use case.. generics are a part of the language now.

[shipperizer]

the 2 bits of code above are basically the same, if we can achieve something without generics we should

it arguably makes the code harder to read, it doesn't speed it up and doesn't really bring anything to the plate apart from sugar syntax in this case

[babakks]

@alesstimec Yeah, I've seen generics used just for ensuring type safety and helping with type casting in other legit OSS projects (other languages).

[shipperizer]

this could be implemented on types generation, see oapi-codegen/oapi-codegen#227

basically using the extensions in the specific the x-oapi-codegen-extra-tags, this would add an extra yaml file, but you would have validation tags/annotations in the struct itself

[babakks]

Yeah, I checked the extensions, but what I don't like about them is that they clutter the spec with generator-specific tags.

[shipperizer]

that is fair, but at the same time right now we have types without effective embedded validation which doesn't sound ideal either

[babakks]

That's fair, too. But the solution would be to change the code generator instead of touching the spec.

[babakks]

@shipperizer In the end, we decided to go with this suggestion. There's this PR on the spec repository to add the extension tags:
canonical/openfga-admin-openapi-spec#33

[BarcoMasile]

question + suggestion: why copying all the handlers (even the ones that don't require a body)? Since no validation is performed on GET /groups (for example) we could just rely on an embedded struct and leverage that mechanism (same way the now removed Unimplemented struct worked). This way you wouldn't need to have all these locs that just pass values to the underlying handler.

Another thing would be to validate the absence of a request body for APIs that don't use it, but we the handler would just ignore it so we're still good to go in that case.

[babakks]

You're right about the embedded struct. In the next push, I'll embed the handler so we only implement the methods we need.

[BarcoMasile]

My issue with this approach is that it is not a real decorator (which would avoid repetition), but it is a full on proxy, since a new implementation of all handler methods is required to do it.
I think we could do validation in one single place, thanks to the fact that we already have specific structs for each request body types (see links for examples on my proposal).

Proposal

Leveraging validator library, and a single shared var validate *validator.Validate instance (since it is thread-safe, link), we could use RegisterStructValidation method to have our custom validation functions for all the typed request bodies (as you did), without the need to add field level tags, so no need to touch generated files), just by running validate.Struct(requestBody).

After this setup, a single middleware would then be able to grab the request body (if r.method != http.MethodGet for example) and run validator.Validate on the specific type, it would just need to choose the right type based on the URL and method.
If using a single middleware makes type guards too long in the code, then we could have one middleware per resource type, but still the whole solution would be much more stramlined and simple.

[babakks]

I actually went that way a bit (except for making it as a middleware, more on that later) and faced some issues. The most obvious one is validator module is not meant to be used like this. It depends on/works with tags, and lets you have your own custom tags to implement more complex validations. I tried to use it without tags and implemented a few struct-level validators but then I noticed there's no decent error reporting mechanism for un-tagged usages. For example, this is the official example:

func UserStructLevelValidation(sl validator.StructLevel) {

	user := sl.Current().Interface().(User)

	if len(user.FirstName) == 0 && len(user.LastName) == 0 {
		sl.ReportError(user.FirstName, "fname", "FirstName", "fnameorlname", "")
		sl.ReportError(user.LastName, "lname", "LastName", "fnameorlname", "")
	}

	// plus can do more, even with different tag than "fnameorlname"
}

Here, the sl.ReportError method has the following signature:

ReportError(field interface{}, fieldName, structFieldName string, tag, param string)

As you see, it expects a tag reference. I, anyway, made a custom validator for Group structs and passed the string "cannot-be-empty" as the tag (when Group.Name is empty). When I tested the error message was:

Key: 'Group.name' Error:Field validation for 'name' failed on the 'cannot-be-empty' tag

Which is not accurate (because there's no cannot-be-empty tag around) and also not that straightforward/clear for the end user. I'm not just complaining about the error message formatting here, but my point is the library is not meant to be used like this.

[babakks]

As for the middleware suggestion, I'm against it because that means we should manually cover all the endpoints and duplicate something like the auto-generated routing. I want to use the generated code as much as possible and avoid repeating it. That's why I had to go with a decorated handler here (which uses the routing done in the auto-generated code).

Regarding this not being a true decorator, I don't think that's the case. I mean, with using an embedded struct as you suggested, the unnecessary repetitions would fade out. But the thing is validation, in essence, should apply on every request. You already pointed out that for some endpoints we need to check the absence of a request body. This means, all endpoints are actually meant to be implemented. But, for now we can skip those.