Tenancy Error Message when Delivery Service Already Exists outside of a user's tenancy

ericholguin commented 5 years ago

I'm submitting a ...

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

Traffic Control components affected ...

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

Current behavior:

Delivery Service with xmlId: test2 exists, belonging to a user with tenancy: -root Login as another user with tenancy: --unassigned POST request to https://{{TO_BASE_URL}}/api/{{api_version}}/deliveryservices with xmlId: test2 in the body. Returns:

HTTP/1.1 403 Forbidden
Content-Type: application/json

{
    "alerts": [
        {
            "text": "not authorized on this tenant",
            "level": "error"
        }
    ]
}

Expected / new behavior:

Should Return

HTTP/1.1 400 Bad Request
Content-Type: application/json

{
    "alerts": [
        {
            "text": "deliveryservice xml_id 'test2' already exists.",
            "level": "error"
        }
    ]
}

Minimal reproduction of the problem with instructions:

Anything else:

mitchell852 commented 5 years ago

This is an interesting one. The first error is confusing to the user because in this case they were working inside their tenancy but got a "not authorized on this tenant" error when in fact the real error was that they picked an xml_id that is already in use (outside of their tenancy).

I like the 2nd error better as it is more accurate.....but it exposes the fact that test2 exists...

not sure of the right solution. @ocket8888 thoughts?

also, pretty sure this is not a regression...and probably not even a bug.

ocket8888 commented 5 years ago

No that's definitely a regression:

HTTP/1.1 400 Bad Request
Connection: keep-alive
Access-Control-Allow-Credentials: true
Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept
Set-Cookie: mojolicious=...; expires=Tue, 03 Dec 2019 23:25:47 GMT; path=/; HttpOnly
Whole-Content-SHA512: YuD9RBlIjNiqZZvJLeAZ1/pbrO0mucAkW2X2vE1akE+zUFjdUpDondWUskyZj/wLqJ1+roxJJgnWV4tjhydptg==
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Date: Tue, 03 Dec 2019 19:25:47 GMT
Access-Control-Allow-Origin: *
Content-Encoding: gzip
Vary: Accept-Encoding
Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE
Content-Length: 100
Content-Type: application/json
Server: Mojolicious (Perl)

{ "alerts": [
    {
        "level": "error",
        "text": "A deliveryservice with xmlId test already exists."
    }
]}

ocket8888 commented 5 years ago

Actually, never mind, you're right. I was on the wrong tenant when I did the request that got me the above response.

If we're returning that they have insufficient permissions to modify a DS, then we've already disclosed the DS's existence.

mitchell852 commented 5 years ago

If we're returning that they have insufficient permissions to modify a DS, then we've already disclosed the DS's existence.

So you're saying that either message discloses the existence of the DS, right?

"text": "not authorized on this tenant",

or

"text": "deliveryservice xml_id 'test2' already exists.",

in that case, i think the better error message is the 2nd one. would you agree @ocket8888 ? if so i would classify this as an improvement and not a bug, right?

ocket8888 commented 5 years ago

Yes, the second one is a much better UX.

apache / trafficcontrol