Custom fields are not validated against the configured format

[Sxderp]

Edit: Small word choice modification to avoid confusion.

Please confirm you have done the following before posting your bug report:

• [x] I have enabled debug mode
• [x] I have read checked the Common Issues page
• [x] Can be reproduced on the demo instance.

Describe the bug Asset custom fields are not being validated.

To Reproduce Steps to reproduce the behavior:

1. Create a custom field with "ALPHA" or a regex as the format.
2. Create a field set with the custom field.
3. Apply the field set to a model.
4. Create a new model asset with the just created model.
5. Edit the model asset and put numbers in the field that is supposed to be "Alpha" (or an invalid text if a regex was used).
6. Hit save
7. Item was saved successfully.. ?

Expected behavior An error or otherwise not saving the asset.

Screenshots If applicable, add screenshots to help explain your problem.

Server (please complete the following information):

• Snipe-IT: 4.7.4
• OS: RedHat Enterprise 7
• Web Server: Apache
• PHP Version: 7.1.29

Desktop (please complete the following information):

• OS: RedHat Enterprise 7
• Browser: Firefox
• Version: 60.7esr

Error Messages None

Additional context

Is this a fresh install or an upgrade?

Upgrade

What OS and web server you're running Snipe-IT on

Redhat Enterprise 7; Apache

What method you used to install Snipe-IT (install.sh, manual installation, docker, etc)

Manual (tarball)

Include what you've done so far in the installation, and if you got any error messages along the way.

Setup users, groups, fieldsets, a few models, created some assets.

Indicate whether or not you've manually edited any data directly in the database

No

[Sxderp]

I may have figured out the issue. I know very little about laravel but this seems likely.

In this commit the laravel version was updated to 5.5. from 5.4.. I took a look at the upgrading docs and found an issue without how Snipe-It is using the failedValidation function in the requests.

https://laravel.com/docs/5.5/upgrade#upgrade-5.5.0 Scroll down to "A Note On Form Requests". It looks like snipe should be throwing an exception rather than returning something, as it currently is.

Upgrade Guide - Laravel - The PHP Framework For Web Artisans

Laravel - The PHP framework for web artisans.

[snipe]

Ah, thanks for that find - I think you may be right. We'll look into that ASAP.

[snipe]

I fear this might not be as straightforward as it seems. If I make those changes, it seems to work, but when testing on the GUI with incomplete fields, it just re-presents the same form with no error messages, and debugbar shows me no errorbag.

Plus if I try this method on the SaveUserRequest, it returns JSON from the GUI. :(

We have a lot of funky overrides in the Exceptions/Handler.php which I'm wondering might be interfering with the new Laravel 5.5 way of doing things, but it's going to take me a little more time to track it down. :(

Interestingly, if you generate a form request using the Laravel 5.5 command line method, it creates:

<?php

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;

class StoreBlogPost extends FormRequest
{
    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        return false;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        return [
            //
        ];
    }
}

which of course doesn't even mention failedValidation, but also seems to be extending FormRequest instead of Request, so... FML.

[Sxderp]

Again I prefix this with I know little about Laravel and am making plenty of assumptions.

For testing when I made 'AssetRequest.php' throw an error in failedValidation. Once the response comes in it looks like the page is forcibly refreshed. I'm guessing this is because it was assumed that response would (eventually) be called? I put some logging in the response function and it seems like it's never being called.

[snipe]

The conversion I'd expect to work for AssetRequest.php would be:

/**
     * Handle a failed validation attempt.
     *
     * public function json($data = [], $status = 200, array $headers = [], $options = 0)
     *
     * @param  \Illuminate\Contracts\Validation\Validator  $validator
     * @return void
     *
     * @throws \Illuminate\Http\Exceptions\HttpResponseException
     */
    protected function failedValidation(Validator $validator)
    {

        throw new HttpResponseException(response()->json([
            'message' => 'The given data is invalid',
            'errors' => $validator->errors()
        ], 422));
    }

Assets are also a unique animal because we treat them a different way than all of the other forms, as we have a javascript file API that allows folks to take a photo from their camera and upload it directly when they create assets, so there is one more layer of confusion to get through to figure out what's going on.

I've been doing most of my testing through the SaveUserRequest, since that's at least pretty straightforward. We don't do any weird file API stuff on those forms, and the API endpoint is easy enough to test. It's a vanilla form. Many of the less-complex forms don't use form requests and we just rely on model-level validation, but this one does, since we've got some conditional validation that's required there. I've confirmed that the logic that determines whether it's a POST/PUT/etc is still working, so I just need to figure out why it's returning JSON in the web GUI - which again, I suspect is related to the Exceptions/Handler.php stuff we do that was required for the API to work as expected in earlier versions of Laravel but may be getting in the way now.

[Sxderp]

I did some more testing and took a look at the Laravel commits. I found a solution that seems to be working (notice the call to response). Not sure if there's anything else that needs to be done. You also might be able to change the ValidationException for HttpResponseException.

    protected function failedValidation(Validator $validator)
    {
        throw new ValidationException($validator, $this->response(
            $validator->getMessageBag()->toArray()
        ));
    }

[snipe]

I'm not sure if you mean for SaveUserRequest or AssetRequest here, but in order to make the other form requests work, I have to meddle with the exception handler. I've got the SaveUserRequest working now by commenting out some areas of the Handler, but still need to test how that affects the other forms and the API responses, as we can't suddenly change the format of the API payloads, or everyone's custom integrations will break.

[snipe]

For example, if I comment out:

if ($e instanceof \Illuminate\Validation\ValidationException) {
                return response()->json(Helper::formatStandardApiResponse('error', null, $e->response['messages'], 400));
}

in the Exception Handler and remove the (added in testing) failedValidation method from that form request, everything (forms and API) seem to work for the SaveUserRequest, but the payload doesn't return the actual errors...

{
    "status": "error",
    "messages": "An Error has occured! The given data was invalid.",
    "payload": null
}

If I comment out the area where we try to be clever and format errors in debug in a slightly nicer way in that error handler:

 if (config('app.debug')) {
                return response()->json(Helper::formatStandardApiResponse('error', null, "An Error has occured! " . $e->getMessage()), 500);
            }

I get:

{
    "first_name": [
        "The first name field is required."
    ],
    "username": [
        "The username field is required unless ldap import is in 1."
    ],
    "password": [
        "The password must include at least one number.",
        "The password confirmation does not match."
    ]
}

which is fine, but it's not formatted the way we need responses to be. That formatStandardApiResponse helper is what standardizes the formatting across all of our API payloads, so commenting it out gives us the barebones validation but without the payload formatted the way we need.

[Sxderp]

I meant for AssetRequest, which already had the failedValidation function. I did not do an API test so I'm not sure if the update caused a change in the response format.

[snipe]

I think I've made some progress here, but I need to do more testing to make sure we don't break other things.

[snipe]

I've pushed out some changes to a test branch. The changes I've made fix the SaveUserRequest problem of not providing useful feedback via the API because of the form request changes in Laravel 5.5, but I'm still working on the AssetRequest. I'm certain the main issue is the way we handle that specific form, since (as you can see in the AssetController's store() method, we don't return a redirect or a view there, we return JSON, specifically to handle that file upload API stuff.

Still working on it, but wanted to give you an update.

[snipe]

Weirdly, I think the rabbit hole goes even deeper. Right now I'm just trying to get the correct response from the API, since that should hit the same validation points...

With some debug info throughout, keeping the AssetRequest failedValidation method as:

return response()->json([
            'message' => 'The given data is invalid',
            'errors' => $validator->errors()
        ], 422);

I see:

Which is mostly okay, and implies that the actual problem is that the errorbag isn't collecting all of the errors or something, but they're being correctly applied.

When I test the API, I get:

Which is a true validation failure, for sure (since that asset does already exist), but its strange that the debug info and the final error bag are not the same.

(I'm mostly just rubber ducking here - feel free to mute this thread if it's too noisy.)

[snipe]

Haha - just to make it more confusing, if I swap out:

return response()->json([
            'message' => 'The given data is invalid',
            'errors' => $validator->errors()
        ], 422);

with

throw new HttpResponseException(response()->json([
            'status' => 'error',
            'messages' => $validator->errors(),
            'payload' => null
        ], 422));

I still get errors, but now:

It's as if the message bag is fighting with the form request failedValidation method or something. One is stomping on the other, but neither actually work as expected, since I should be seeing TWO errors each time: one that the asset tag is not unique, and one that the regex is required.

To recap....

return response()->json([
            'message' => 'The given data is invalid',
            'errors' => $validator->errors()
        ], 422);

results in

{
    "status": "error",
    "messages": {
        "asset_tag": [
            "The asset tag must be unique."
        ]
    },
    "payload": null
}

where

throw new HttpResponseException(response()->json([
            'status' => 'error',
            'messages' => $validator->errors(),
            'payload' => null
        ], 422));

results in:

{
    "status": "error",
    "messages": {
        "_snipeit_regex_14": [
            "The  snipeit regex 14 field is required."
        ]
    },
    "payload": null
}

They're both kinda right, and both kinda wrong, since neither of them contain BOTH error messages.

What we SHOULD be seeing here is:

{
    "status": "error",
    "messages": {
        "asset_tag": [
            "The asset tag must be unique."
        ],
        "_snipeit_regex_14": [
            "The  snipeit regex 14 field is required."
        ]
    },
    "payload": null
}

While this is all very frustrating, I think this is actually getting us somewhere with respect to understanding what's happening.

[snipe]

It also seems the latter is correctly processing multiple custom field rules.

throw new HttpResponseException(response()->json([
            'status' => 'error',
            'messages' => $validator->errors(),
            'payload' => null
        ], 422));

gives me

{
    "status": "error",
    "messages": {
        "_snipeit_regex_14": [
            "The  snipeit regex 14 format is invalid."
        ],
        "_snipeit_imei_15": [
            "The  snipeit imei 15 field is required."
        ]
    },
    "payload": null
}

if I add another regex field and give it a value that doesn't match the pattern.

[snipe]

I bet it's an issue with the model level validation.... If I remove the ALL of the existing asset form request rules, leaving just $rules[] in that form request, the model level validation still kicks in.

{
    "status": "error",
    "messages": {
        "model_id": [
            "The model id must be an integer."
        ],
        "asset_tag": [
            "The asset tag must be unique."
        ]
    },
    "payload": null
}

So I think

return response()->json([
            'message' => 'The given data is invalid',
            'errors' => $validator->errors()
        ], 422);

falls back to model level validation, and

        throw new HttpResponseException(response()->json([
            'status' => 'error',
            'messages' => $validator->errors(),
            'payload' => null
        ], 422));

ONLY looks at the form request stuff, or at least lets the form request validation stomp on the model level validation - which makes sense, because it's a form validator, but that may be part of our issue.

[snipe]

I have a WIP PR fix for this, but still seeing some wonkiness on the GUI, where it only sometimes displays the error messages. Still working on it. https://github.com/snipe/snipe-it/pull/7272

[snipe]

(Side note: I may remove that file upload API feature, or at least rework it at some point, because it seriously over complicates what should be a very straightforward form. I think I actually already removed it from Snipe-IT v5. I don't know many folks who use it, and the extra work it causes isn't worth it.)

[snipe]

Can you check out that branch and test?

[snipe]

I merged this into master, if you want to test

[Sxderp]

I'll test it when I have time. Unfortunately that might not be until Sunday/Monday.