Arvados

At e27463c

I think Workbench shouldn't know anything about the special UUID for the "anonymous" user. It needs a config setting; it needs to use that token instead of the usual session token when ((user is inactive) or (user is not logged in)) and (not in one of the user-oriented controllers like sessions and user_agreements); and it needs to act differently in some cases based on the knowledge that it's using its configured token instead of a regular session token. (E.g., if I give it a real user's token as anonymous_user_token, it should still refuse to do post/put operations.)

Remove non-essential config stuff from application.yml.example.

Set anonymous_user_token to false in default config, and just check "if anonymous_user_token" instead of "....size==50".

I don't really like having helper methods that are only used by tests, not by the application. Shouldn't we test this behavior by using fixtures to make a "shared with anonymous" project? (That way we can add API server test cases as well, e.g., to ensure even a badly-behaved client cannot use an "anonymous" token to modify anything.)

(to be continued)

Tom,
1. Regarding the helper methods: I implemented them with the idea that we will use those method when we implement the UI button to "share" resources. When the UI is implemented, the framework would be ready, and it just needs to call these methods to create the un/share links. I wanted to make sure these are tested, and hence added the calls in the tests.

2. Can you please clarify about "Workbench shouldn't know anything about the special UUID for the "anonymous" user. It needs a config setting; it needs to use that token instead of the usual session token when . . . ".

Thanks.
--Radhika

(continued)

Instructions in workbench application.default.yml should be a bit more specific, like "...by running `bundle exec ./script/get_anonymous_user_token.rb` in the directory where your API server is running."

The session/logout stuff seems a bit fragile (using the session this way can cause occasional weird behavior in other tabs in the same browser). I think the thread stuff will also need to change to make the "inactive user can act as anonymous user" behavior possible. Perhaps ApplicationController can have a "switch to anonymous user if necessary" method. Controllers that don't desire this (like sessions, api_tokens, user_agreements) can override it and make it a no-op. That method can also set the appropriate "anonymous / can't act" flag in current_user, rather than having can_act look for the magic uuid string...

BTW I haven't managed to see the new behavior yet -- Workbench doesn't notice my config, or something. Haven't figured out exactly what I'm doing wrong.

Radhika Chippada wrote:

Tom,
1. Regarding the helper methods: I implemented them with the idea that we will use those method when we implement the UI button to "share" resources. When the UI is implemented, the framework would be ready, and it just needs to call these methods to create the un/share links. I wanted to make sure these are tested, and hence added the calls in the tests.

OK. I think we'll use a more generic system with no special code path for the anonymous case, but this does at least start to show how share links should work. So, no harm leaving it in. Test fixtures still desirable too, for testing on the API server side.

2. Can you please clarify about "Workbench shouldn't know anything about the special UUID for the "anonymous" user. It needs a config setting; it needs to use that token instead of the usual session token when . . . ".

Workbench shouldn't rely on api server to always use "*-anonymouspublic" as the anonymous user's uuid. Rather, Workbench should behave differently according to whether Workbench itself is actually using its configured "anonymous token" right now.

Tom,
Please take another look at this branch. It is not 100% done, but the following are in and I want to make sure we both are on the same page with the flow:

- anonymous user is configured as inactive (earlier I had it as active user)

- all references are now checking if user is active instead of can_act? (removed the can_act? method)

- anonymous user check is performed by comparing token to that configured (no knowledge of uuid in workbench)

- instead of using session, I introduced a new Thread.current parameter to keep track of the token and let login take place

- for the time being, I suppressed inactive user / agreement check (this is yet to be implemented)

What is still pending:
- inactive user agreement check. Currently, inactive user page, agreement check is suppressed. This needs to be addressed next

- test fixtures are yet to be added. couple tests are still failing and I will look into them tomorrow

Thanks.

Notes

• session['arv-referrer'] still seems to be there
• one check for magic uuid still seems to be there

check_anonymous_token is confusing me a bit. It seems like use_anonymous_token_if_necessary could be simpler, like

• if anonymous config enabled, and current_user is nil/inactive
  • set Thread.current[:arvados_anonymous_api_token] to configured anonymous token
  • yield
  • unset Thread.current[:arvados_anonymous_api_token]
• else, yield

This around_filter can be skipped by UserAgreementsController.

Then, ArvadosApiClient can automatically use the anonymous token if it is set in Thread.current and method=='GET'.

(I think everything will be less confusing if we can arrange for current_user to always be the currently logged-in user, regardless of what's happening with the anonymous token.)

1. session['arv-referrer'] -- With anonymous configuration, when a user visits the home page, they will see shared content instead of a login page. If a user were to login by clicking the "Log in" link and then click the Logout button, without this they will again see the shared content instead of Login page. I think this would be confusing to users. But, if this is the behavior we want, I can remove it. Please confirm.

2. Thanks for catching the spurious magic anonymous uuid usage. Fixed it. Along with it, I was able to cleanup a bit more around determining if is_anonymous.

3. The around_filter use_anonymous_token_if_necessary alone does not work. The application_controller -> thread_with_mandatory_api_token method invokes thread_with_api_token method, which needs to check the validity of the token and set it. Simply setting it if anonymous config enabled, and current_user is nil/inactive does not work, because we need to address the case where an invalid anonymous token might be configured. So, I just made a method than doing inline.

4 The around_filter is skipped by UserAgreementsController, though it would result in invoking the check_anonymous_token method in earlier filters.

5. current_user to always be the currently logged-in user: This is what the case is. I am no longer juggling with session tokens and instead using Thread.current[:arvados_anonymous_api_token]

Sketch of possible way to arrange/name existing/new filters so they make a bit more sense:

class ApplicationController
  before_filter :thread_clear
  before_filter :permit_anonymous_browsing
  around_filter :thread_with_optional_api_token
  before_filter :require_api_token
  before_filter :require_active_user

  def permit_anonymous_browsing
    @permit_anonymous_browsing = true
  end

  def require_api_token
    if !@permit_anonymous_browsing and !Thread.current[:arvados_api_token]
      redirect_to_login
    end
  end
end

class UserAgreementsController
  skip_before_filter :permit_anonymous_browsing
  skip_before_filter :require_active_user
end

I think this (in app/models/arvados_base.rb) should just be current_user && current_user.is_active:

-    current_user
+    current_user && !current_user.is_anonymous

This (in app/models/arvados_api_client.rb) is the API server's URL that makes logout happen, not a Workbench URL. (I don't think we want to change it.)

-    arvados_login_url(params).sub('/login','/logout')
+    "/user_sessions/logged_out" 

In the views/layouts, change current_user.is_anonymous to !current_user.is_active.

Rename anonymous_login_enabled to anonymous_browsing_enabled (I think "anonymous login" is confusing when compared to how it's really implemented) and it cannot depend on Thread.current[:arvados_anonymous_api_token] because that depends on which action is running. For example, the layout shouldn't lose the "projects" dropdown just because you happen to be looking at the user_agreements page where anonymous token usage is disabled.

I don't quite see why the long permit_anonymous_browsing_if_no_thread_token and permit_anonymous_browsing_for_inactive_user methods are better than the one-line permit_anonymous_browsing above...

I think replacing the stuff like "render 'user_agreements/index'" with "redirect_to user_agreements_path" could make the anonymous behavior easier to accomplish. Worth a try?

The User.is_anonymous method should probably go away: It's hard or impossible to make it correct, and it shouldn't be necessary. (The current implementation actually tells you "would an API call use the anonymous token right now?" not "is this particular user model the anonymous user?".)

Edited from above:

ArvadosApiClient should use the anonymous token if it is set in Thread.current and method=='GET' and !current_user.is_active.

If we do that, we can avoid overloading current_user to mean "current user, or sometimes anonymous user, depending on the situation" -- and avoid overloading Thread.current[:arvados_api_token] to mean "user's token, or perhaps the anonymous token"...?

Tom,
Thanks for catching the issue with the logout url. I addressed most of these comments. Please feel free to take another look. Have a few questions / unsure of some of the other comments.

1. "don't quite see why the long permit_anonymous_browsing_if_no_thread_token and permit_anonymous_browsing_for_inactive_user methods are better than the one-line permit_anonymous_browsing above" : Your permit method does not do any validation against the configured anonymous token. Do we not want to make sure the configured anonymous token is valid? I now made the permit_anonymous_browsing_if_no_thread_token method a single liner by letting the next filter (set_thread_api_token) do the validation.

2. "In the views/layouts, change current_user.is_anonymous to !current_user.is_active" : The view is customizing the login button display when it is anonymous user. For example, if an anonymous user is the context, the top nav does not show the email and just shows the login button. Whereas, if it is an active or in_active user, instead of Login button, we see the user's email and the dropdown with Logout button (and others as needed) is displayed.

3. "ArvadosApiClient should use the anonymous token if it is set in Thread.current and method=='GET' and !current_user.is_active" : Please clarify this. I added this logic to use arvados_anonymous_api_token instead of arvados_api_token in these specific cases.

4. "I think replacing the stuff like "render 'user_agreements/index'" with "redirect_to user_agreements_path" could make the anonymous behavior easier to accomplish" : I prefer to wrap this story up and merge into master asap given the amount of time it has already taken. I can revisit this suggestion as part "Workbench UI improvements" story this sprint.

Thanks.
--Radhika

At 2659-anonymous-server-side 4d351f0

• It seems like making "aproject" anonymously accessible could detract from the effectiveness of other test cases that test permissions of "aproject". Better to just use the new "anonymously_accessible_project" for testing anonymous behavior?
• If needed, share_aproject_with_anonymous_group might be a clearer name.

In get_anonymous_user_token.rb, the get_existing block should verify that scopes is restricted as we expect. Checking expiry time (while not exactly a security fix) might be nice too. Perhaps like

if get_existing
  api_client_auth = ApiClientAuthorization.
    where('user_id=?', anonymous_user.id.to_i).
    where('expires_at > ?', Time.now).
    select { |auth| auth.scopes == ['GET /'] }.
    first
end

• Could fix indentation at group_perm = Link.create(...)
• I think it would be better to look up (and create if necessary) the "anon user can_read anon group" permission link in the if not $anonymous_user block, even if we don't create a new User record (i.e., after the if !$anonymous_user block). This mirrors the behavior for the User and Group records, and lets us recover automatically (instead of staying in a mysterious troubleshooting situation) if somehow the database ends up with an anonymous_user but no permission link.
• Use create!, instead of create which can fail silently.

2659-anonymous-server-side @ 60ea080 - looks good to merge, thanks!

There is now an active customer request around this. Customer would like to create a project to share with his team. Customer would like to be able to create a project with a pipeline and some data and some analysis. Then wants to be able to create a shareable link that can be sent around. Anyone with the link can log in and view the project (including provenance, etc) for "free." Of course, running the analysis requires logging in, etc.

See me for details. I can set up a meeting between engineering and customer to refine the story if necessary.

• ApplicationController#check_user_agreements should return true if current_user.nil?
• ApplicationController#check_user_profile should return true if current_user.nil?
• ProjectsController should skip_before_filter :require_thread_api_token, only: :show
• ArvadosApiClient should use the configured anonymous token if Thread.current[:arvados_api_token].nil?
• Presumably, various parts of the view will need to be wrapped in if current_user

• "Create/remove sharing link" shouldn't be there (but the existing sharing link, if any, could be shown).
• Downloading and viewing files should work.

Hiding the jobs/pipelines/other tabs on projects#show might be an acceptable short term measure (as an alternative to making the pipeline/job #show pages work well in the anonymous case).

Branch '2659-anonymous-share-projects' implemented workbench support for anonymous users. Some details:

• Updated arvados_api_client to use the anonymous token when no current user is found. This would now make it possible to see the objects shared with anonymous users.

• For a shared project, it would be desirable that pipeline inputs selections are from within this project’s collection set. Similarly, is is desirable that we “run” pipelines in this project as opposed to move them from elsewhere so that the pipeline outputs are also in the shared projects.

• It appears that the job log is saved in the user’s Home project rather than the project in which they are run. Hence, suppressed the Log tab in job#show page for anonymous users.

• I updated link_to_if_arvados_object to display raw text when it is a User object for inactive users. Are there other object types that we want to do this for anonymous user case? I only found Collection, Job, PipelineInstance, PipelineTemplate and Group types in the three allowed project tabs, which the anonymous user is allowed to access.

• Should we update the 404 page to display (1) 404 when the object actually does not exist, (2) permission denied when object cannot be displayed because of access control? I found this to be very confusing in the past and an anonymous user might run into it more often than other regular / non-admin users