Admin can assign User to a Federation with wrong PARAMETER

[dpham692]

I'm submitting a ...

• bug report
• new feature / enhancement request
• improvement request (usability, performance, tech debt, etc.)
• other

Traffic Control components affected ...

• CDN in a Box
• Documentation
• Grove
• Traffic Control Client
• Traffic Monitor
• Traffic Ops
• Traffic Ops ORT
• Traffic Portal
• Traffic Router
• Traffic Stats
• Traffic Vault
• unknown

Current behavior:

Login as ADMIN
POST Request: https://{{TO_BASE_URL}}/api/{{api_version}}/federations/{{federationsID}}/users.
With wrong json payload parameter like "userIdssss"

{
        "userIdssss": [2],
        "replaces": true
}

It will return

HTTP/1.1 200 OK
Content-Type: application/json; charset=utf-8

{
    "alerts": [
        {
            "text": "0 user(s) were Assign to the test.quest. federation",
            "level": "success"
        }
    ],
    "response": {
        "userIds": null,
        "replace": null
    }
}

Expected / new behavior:

Should return 400 Bad Request

Minimal reproduction of the problem with instructions:

[ocket8888]

This is because our API always ignores keys that aren't a part of the defined request structure, as that's what the Go parser does. Implementing it to do anything else would be a lot of effort for little return, so I don't see this improvement happening any time soon.

[rob05c]

Rejecting that would also be a violation of the Robustness Principle, "Be liberal in what you accept, conservative in what you send." (https://tools.ietf.org/html/rfc1122#section-1.2.2, https://en.wikipedia.org/wiki/Robustness_principle).

I know it's common for a lot of APIs to reject unknown things like that, but in general, it makes servers more robust, and makes integration and interoperability better and work more and fail less, when the Robustness Principle is followed.

TC mostly follows it, though there's not a hard rule and I'm sure there are places it doesn't. I know I've said this before, but I'll keep saying it: we should follow the Robustness Principle wherever possible. There are cases like this one ("userIdssss": [2],) that can be confusing, but for the vast majority of cases it makes TO and TC more robust and resilient to imperfect input, and easier for clients to integrate and work with.

[lbathina]

this is a bug. I have seen this working working for other endpoints. when the parameter in json body is not specified correctly it spits error saying that the parameter is required.

[ocket8888]

That's because Go's json parsing can't determine the difference between a "zero-valued" field and a non-existent field. To oversimplify: null is the same as [] is the same as undefined. So from the parser's perspective, not including an array key is the same as including it but it's empty. When an array field is "required" in the API, that also means it can't be empty. The converse is also true.

So it's not a bug. That's by design. However, there's no reason that parameter shouldn't be required, IMO.

[mitchell852]

POST Request: https://{{TO_BASE_URL}}/api/{{api_version}}/federations/{{federationsID}}/users.
With wrong json payload parameter like "userIdssss"

{
        "userIdssss": [2], <<-- this is ignored. should be userIds
        "replaces": true <<-- this is ignored. should be replace
}

Does seem like a bug however because something must be required, so why would a 200 be returned from:

{  }

Why not just make userIds required? and i'm not sure about replace.

[mitchell852]

and i'm not sure the title of this issue is correct

Admin can assign User to a Federation with wrong PARAMETER

as i don't think that's true. using the wrong parameter does nothing as far as i can tell.

[mhoppa]

My plan is to make userIds required - replace is a documented optional parameter

[lbathina]

This issue isn't fixed. get the response 200

{
    "response": [
        {
            "company": null,
            "email": null,
            "fullName": null,
            "id": 2,
            "role": "admin",
            "username": "admin"
        }
    ]
}

for request body

{
        "userIdsss2sss": [2],
        "replaces": true
}

[mhoppa]

Did you make sure your environment is latest?

[lbathina]

the environment shows the latest version. as per our discussion, it seems the build doesn't seem to have complete updates. checking on ciab with latest master

[lbathina]

it's working as expected.