Pushed a branch with a test: 16736-expiring-tokens-limits

Updates at 386b60af1 - branch 16736-expiring-tokens-limits
Test run: https://ci.arvados.org/job/developer-run-tests/2078/

• Adds tests exposing the bug.
• Adds checks related to token's expires_at on create/update.
• Fixes fixture removing expiration dates that were set far in the future.

Lucas Di Pentima wrote:

Updates at 386b60af1 - branch 16736-expiring-tokens-limits
Test run: https://ci.arvados.org/job/developer-run-tests/2078/

• Adds tests exposing the bug.
• Adds checks related to token's expires_at on create/update.
• Fixes fixture removing expiration dates that were set far in the future.

  def permission_to_create
    current_user.andand.is_admin or
      ((current_user.andand.id == self.user_id) and
        (current_api_client_authorization.andand.expires_at.nil? or
          (self.expires_at and current_api_client_authorization.expires_at >= self.expires_at)))
  end

I don't think this is quite right, this will allow an admin to always create a token even if their token is about to expire. But that's not right. Admins can create tokens for other users, but the tokens are still capped by expires_at.

Also, instead of "current_api_client_authorization.andand", if it is possible for current_api_client_authorization to be nil I don't think we allow tokens to be manipulated (is there a special case, like tests?)

I think this should be:

  def permission_to_create
    (current_user.andand.is_admin ||
      current_user.andand.id == self.user_id) and
        !current_api_client_authorization.nil? and
        (current_api_client_authorization.expires_at.nil? ||
          (!self.expires_at.nil? && current_api_client_authorization.expires_at >= self.expires_at)))
  end

By the way, I looked this up recently, the difference between "and"/"or" and "&&"/"||" in Ruby has to do with operator precedence, "and"/"or" have lower precedence than "&&"/"||" so the convention is to use the and/or for the "outer" clauses and &&/|| for the "inner" clauses although when grouping with parenthesis it doesn't really matter.

Arvados::V1::ApiClientAuthorizationsController#find_objects_for_index should include a filter on expires_at when current_api_client_authorization is non-nil, and there should be a controller test for that.

update: I think you mentioned on gitter than current_api_client_authorization is nil in the case of the login process, which makes sense. We are creating a token, we don't have one yet.

However, if there was a bug that allowed some other way of calling token creation without setting current_api_client_authorization, that would be a serious security hole.

I wonder if there's a way to handle login as a special case. The value of current_api_client_authorization comes from Thread.current[:api_client_authorization], so the login code could assign a fake (not persisted to database) ApiClientAuthorization object.

When Login.TokenLifetime is set:

• Token from login uses "Login.TokenLifetime" expiration (existing behavior)
• (valid) token from login can be used to create new tokens
  • If no expiration time is given, it is the "maximum" expiration (new configuration option API.MaximumTokenLifetime)
  • SystemRootToken can create tokens with any expiration (or nil)
• Token created to run a container doesn't have a set expire time, because it will expire when the container ends
• Token used on a shell node is max lifetime, when about to expire it is rotated by the arvados-login-sync script
• "Show current token" dialog in workbench should generate a new token instead
  • Change the label to something like "Get new user token"
• Auto-logout should coordinate across browser tabs/windows (using the same token?) so that it only logs out when all of them have been idle.
  • Figure out which browser feature lets you do that