search

apache / trafficcontrol

Apache Traffic Control is an Open Source implementation of a Content Delivery Network
https://trafficcontrol.apache.org/
Apache License 2.0
1.04k stars 339 forks source link

TO golang API framework tenancy enhancements #2273

Open rawlinp opened 6 years ago

rawlinp commented 6 years ago

While working on the Origin API, I found a few things related to tenancy in the TO golang API framework that should be enhanced:

~1. The shared handler functions (e.g. UpdateHandler, CreateHandler, etc.) should check if tenancy is enabled before calling IsTenantAuthorized on a Tenantable type. This prevents each Delete, Update, etc. function from having to check if tenancy is actually enabled before checking further. Generally if tenancy is disabled there should be no checking of tenancy. This could be cached on startup so that it doesn't have to be checked on every single request.~ UPDATE: no longer relevant.

  1. If the isTenantAuthorized function had an added operation enum parameter added (i.e. when calling it from the CreateHandler, it would pass Operation.Create, from the DeleteHandler it would pass Operation.Delete, etc), you could add specialized tenancy logic to your isTenantAuthorized implementation for the type of operation being performed. For instance, Delete() doesn't take a request body, so you only need to check if the user has access to the existing tenant. However, for Update(), you need to check the tenancy on both the current and requested tenants. It would be better if all the tenancy logic was able to stay in isTenantAuthorized rather than specialized tenancy checks in each method.
DylanVolz commented 6 years ago

+1

dangogh commented 6 years ago

use_tenancy really needs to be eliminated. If, instead, we ensure tenant_id is set to the root tenant for all, it effectively is the same as setting use_tenancy to 0, but will simplify the tenancy code. See the discussion in the dev mailing list here: https://lists.apache.org/thread.html/64500aed4bd899e9b11ac43332084680b4a0f65d3366481fcb472088@%3Cdev.trafficcontrol.apache.org%3E

ocket8888 commented 3 years ago

I believe 1 is no longer relevant, and I think there's a handler for tenants that takes in multiple tenant keys to check for access, which would cover 2, right?

rawlinp commented 3 years ago

1 is no longer relevant, but 2 still is. I don't think you understood 2 correctly -- it's not about having a handler that checks multiple tenant keys. It's about keeping all the tenancy checks together in IsTenantAuthorized() # the Tenantable interface rather than having special tenant checks in each of the create, read, update, and delete methods.

ocket8888 commented 3 years ago

Oh, I see. I was confused because we have isTenantAuthorized and IsTenantAuthorized which are not the same thing. That makes sense