Updating an image by adding 'visibility' returns a 200 response

[Luke Wollney]

Overview:
When a user attempts to update an image by adding 'visibility' as an image property, a 200 response is returned.

Steps to reproduce:
1) Create an image
2) Update the image via PATCH /images/<id> passing '[{"path": "/visibility", "value": "private", "op": "add"}]'
3) Notice that a 200 accepted response is returned

Expected:
A 403 response should be returned

Actual:
A 200 response is returned

[Yongfeng Du (dolpherdu)]

Using the latest devstack, I did some tests on this senario.

1. user demo create an image, the image by default is 'private'

2. issue command to set private, success. (no changes)
curl -i -X PATCH -H 'Content-Type: application/openstack-images-v2.1-json-patch' -H 'X-Auth-Token: e1b9454607e54f74b0439931e469cf68' -d '[{"op": "add", "path": "/visibility", "value": "private"}]' http://devstack:9292/v2/images/3ce7864a-ebb7-426d-afc0-a7d756529a16

3. issue command to set 'public', failed with 403. (No permission to set public)

4. swtich to user admin, change the image property to 'public', success

5. switch back to user demo, change image property to 'private', success.

This behavior is reasonable to me, since demo is the owner of the image, he should have the permission of setting it to 'private'.

Could you clarify your test?
1). the detailed steps and setup of test
2). Why do you think this is a bug? maybe we have misunderstanding of the V2 PATCH method?

[Luke Wollney (luke-wollney)]

Thank you for taking the time to look into this issue. Please let me explain a little more here:

To me, the update image request accepts three different operations for a reason. We should use the 'add' operation to add new image properties that do not currently exist on the image. We should use the 'replace' operation to change an existing image property's value. We should use the 'remove' operation to remove an existing image property.

The scenario that I am attempting here is to update the image by using the add operation to add an image property that already exists. Because the property already exists (the visibility property exists on all images by default), I would expect some sort of error response to be returned. I mention that I expect a 403, but a 409 would probably also be acceptable.

Please let me know if you have further questions. Thank you again.

[Yongfeng Du (dolpherdu)]

OK, I understand your point now.

This behavior depends on how the API defines it. It seems that the glance API v2.0 doesn't explicitly state this situation,
http://specs.openstack.org/openstack/glance-specs/specs/api/v2/http-patch-image-api-v2.html#operations

But on RFC6902 (https://tools.ietf.org/html/rfc6902#section-4.1), it clearly defined the "add" operation's behavior:
"If the member already exists, it is replaced by the specified value.
"
So I think the glance behavior is proper, and this bug should be closed as "Not a Bug".

Any opinions?

Regards,
Dolpher

[Luke Wollney (luke-wollney)]

Thank you again for responding with some further details and for making those references.

It just seems a little odd to me because we have a 'replace' operation for the purpose of replacing property values. When using the add operation, passing visibility and the response being successful, it almost seems like a new visibility property is added even though it is only being replaced.

If that is the agreed upon standard, "if the member already exists, it is replaced by the specified value" for the add operation, then I guess I can be okay with it, but I just want to make sure that this is the standard that is used throughout. Can we get some other folks' input on this to make sure everyone is on the same page and okay with the way this works?

Thank you again!

[Yongfeng Du (dolpherdu)]

No problem Luke.
And since there are different understandings, I think it's better to submit a change request to the API doc to clarify this.

Will let you know when the patch is ready for review.

[Yongfeng Du (dolpherdu)]

Hi Luke,

review submitted, feel free to comment, or involve more people to review.
https://review.openstack.org/#/c/178491/

[Luke Wollney (luke-wollney)]

Thank you for opening that. I will post my comments and see what others say. Thank you again.