Arvados

22760-public-ports @ 9d99812d7ef0d0248b1f2811b781d3068a33390e -- developer-run-tests: #4751

Tom Clegg wrote in #note-6:

22760-public-ports @ 9d99812d7ef0d0248b1f2811b781d3068a33390e -- developer-run-tests: #4751

The one thing that had me scratching my head was why it requested the container record twice with ContainerGet, but now I think I understand why -- the first time, it uses the system root token to ensure it can get the container if it exists and see if the port is public. The second time, it uses the current user context, which means if the user doesn't have permission to see the container at all, it won't be able to fetch the container record and then it errors out. If they did fetch the container record, it goes on to check permission using checkContainerLoginPermission.

I noticed that checkContainerLoginPermission uses ModifiedByUserUUID instead of the container request instead of comparing with RuntimeUserUUID of the actual container, is there a reason for that? (Using modified_by_user_id for any kind of security check has always seemed a little weird because I believe at minimum anyone with write permission can edit the container request name and description while it is running).

I think this is all fine for the 1.0 version of this feature but the fact that it requires multiple calls to the Rails backend per request through the proxy seems like it could be a bit of overhead if used heavily.

Peter Amstutz wrote in #note-7:

The one thing that had me scratching my head was why it requested the container record twice with ContainerGet, but now I think I understand why -- the first time, it uses the system root token to ensure it can get the container if it exists and see if the port is public. The second time, it uses the current user context, which means if the user doesn't have permission to see the container at all, it won't be able to fetch the container record and then it errors out. If they did fetch the container record, it goes on to check permission using checkContainerLoginPermission.

Right. I've added some comments there to explain.

I noticed that checkContainerLoginPermission uses ModifiedByUserUUID instead of the container request instead of comparing with RuntimeUserUUID of the actual container, is there a reason for that? (Using modified_by_user_id for any kind of security check has always seemed a little weird because I believe at minimum anyone with write permission can edit the container request name and description while it is running).

(This is the same logic we've been using for SSH access.)

I think it's hard to say, in general, what the right choice would be when modified_by_user_uuid ≠ runtime_user_uuid.

But I think it seems more likely that modified_by_user_uuid will be the user who is logged in to workbench and clicking the "connect to port" button.

I think this is all fine for the 1.0 version of this feature but the fact that it requires multiple calls to the Rails backend per request through the proxy seems like it could be a bit of overhead if used heavily.

Yes, at some point we'll surely want to reduce the overhead. I figured we'd want to do that after unblocking the workbench side, and possibly considering other permission options.

22760-public-ports @ 929fa9ab331bde5747d57ede7f5c9bf64344c531

Tom Clegg wrote in #note-8:

22760-public-ports @ 929fa9ab331bde5747d57ede7f5c9bf64344c531

LGTM.