Arvados

From IRC:

tom
3:20 I think the test would be
3:20 1. enable anonymous config
3:20 2. take uuid of a publicly accessible object, like the GPLv3 pdf / user agreement
3:21 3. ensure {workbench}/actions?uuid={uuid-of-public-thing} redirects to the appropriate "show" page
3:23 I think currently the "show" page works for anonymous users, but the redirect feature doesn't. Oh, and the background is that curover.se/{uuid} redirects to workbench/actions?uuid={uuid}

brett
3:24 (The obstacle that I know exists is that the Workbench around_filter require_thread_api_token is not skipped for this route.)

tom
3:28 May I load this up slightly by asking for a test for the non-public case, too? (I haven't checked git-blame to be sure but I'm pretty sure it's my fault there's no controller test for this.)

Reviewing 80abb8a, and I just have a question: why do we only skip the filter for certain classes of API objects? I know the route supports all kinds of objects for logged in users. If anonymous users have access to a particular object, they'll be able to view it through Workbench whether or not this route exists, so I don't see what we might gain by limiting this way.

The reason I worry about it is that, even if there are underlying bugs in Workbench, having a hardcoded list here means we'll have another place we need to change code in order to fix the bug. I'm worried we'll forget about it.

Thanks.

Brett asked:

why do we only skip the filter for certain classes of API objects

Please look at projects_controller etc for those 5 types of objects. These are the only ones that are currently skipping require_thread_api_token for show action when anonymous browsing is enabled. Hence, I added this same list in actions_controller. If we want to omit this for all #show operations, then we must do it for all controllers. Thanks.

Radhika Chippada wrote:

Brett asked:

why do we only skip the filter for certain classes of API objects

Please look at projects_controller etc for those 5 types of objects. These are the only ones that are currently skipping require_thread_api_token for show action when anonymous browsing is enabled. Hence, I added this same list in actions_controller. If we want to omit this for all #show operations, then we must do it for all controllers. Thanks.

I'm sorry, but I'm not sure I follow this. If the condition is removed from ActionsController, it seems like the end result for the user is the same. They'll still be redirected to the Workbench login page—they'll just go through the ActionsController redirect first. If the restrictions already live in the corresponding controllers, then I'm worried duplicating it here just means it's another thing we might miss fixing when we lift the restriction later. (And having it in the individual controllers seems like a more natural place to express it, since any code that doesn't work with the anonymous user token presumably lives in the controller or its templates.)

I'm sorry if I'm being dense, I'm realizing I haven't spent good time in the Workbench code in a while. Is there still something I'm missing?

There is going to be subtle difference in UX.

If a user goes to, let's say, qr1hi/projects/<project_uuid> or qr1hi/actions?uuid=<project_uuid> and if it is not publicly accessible, the user will see the Page not found page. This is because projects_controller bypasses the require_thread_api_token filter. On the other hand, if it is accessible publicly, the user will see it with the Login button still intact in top nav.

On the other hand, if a user were to access qr1hi/humans/<uuid> (or any other object type other than those 5 types), the user will see the login page. If we bypass the filter altogether in actions_controller#show operation, the user will see the Page not found page instead of login page if he were to access /actions?uuid=<human_uuid>. Since objects other than those 5 types are not publicly accessible, in this case the user will always see the error page instead of the login / landing page.

I hence used the same logic as the other controllers to avoid showing an error page when it is not required to be shown.

Thanks.

Thanks for explaining. I think explaining that in code comments or a commit message as well would be helpful (I think people are more likely to find that than read all the comments on a ticket), but that's my only lingering thought. 5f708f62 is good to merge.