authz: protect VPC endpoints #743
Conversation
| @@ -2110,18 +2198,15 @@ impl DataStore { | |||
| let now = Utc::now(); | |||
| diesel::update(dsl::vpc) | |||
There was a problem hiding this comment.
At least some of the other deletion methods use the check_if_exists method to separately handle the case where there is no such object at all, and where there is one, but it's already been deleted. In the latter case, I think we want to not update time_deleted and return a success, to make the operation idempotent. If I understand correctly, this will return an object-not-found error in that case.
There was a problem hiding this comment.
You might be right, though I think we expect that DELETE of a resource that doesn't exist in the public API produces a 404, not a successful result. This seems worth testing more broadly, though this one seems hard to test because you will have just looked up the VPC successfully to get to this code path.
Anyway, I think this is unrelated here (in that the behavior you're describing exists on "main" and isn't related to the change here). I'd like to file a separate issue to cover this.
There was a problem hiding this comment.
I meant that: if there is no such record at all, return 404; if there is a record, but it's already deleted, return success. Or by "in the public API", did you mean one that isn't deleted? As an example, when you create a VPC (so you have the ID), then delete it twice, what should we return for that second call?
But that's a general question, I agree, and we should do it separately.
| @@ -2110,18 +2198,15 @@ impl DataStore { | |||
| let now = Utc::now(); | |||
| diesel::update(dsl::vpc) | |||
There was a problem hiding this comment.
You might be right, though I think we expect that DELETE of a resource that doesn't exist in the public API produces a 404, not a successful result. This seems worth testing more broadly, though this one seems hard to test because you will have just looked up the VPC successfully to get to this code path.
Anyway, I think this is unrelated here (in that the behavior you're describing exists on "main" and isn't related to the change here). I'd like to file a separate issue to cover this.
See #419 for context. This change adds authz protection for VPC create/delete/update/get and the "list VPCs" endpoint. This does not affect any of the resources underneath VPCs (subnets, routers, firewalls, etc.).
I ran into #742 and fixed that here as well.