Arvados

This get_anonymous_user_token.rb script is very old and spits out a lot of warnings on Ruby 2.7 (see below).

I don't know that there is a reason for it to exist anymore; we should just have controller read the anonymous token from the config on startup and if it does not exist in the database, create the appropriate record. That way the installer does not need to know/do anything.

# RAILS_ENV=production bundle exec ./script/get_anonymous_user_token.rb 
Defaulting to memory cache, because /var/www/arvados-api/current/tmp/cache does not exist
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type.rb:27: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:9: warning: The called method `add_modifier' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activemodel-5.2.6/lib/active_model/type/integer.rb:13: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activemodel-5.2.6/lib/active_model/type/value.rb:8: warning: The called method `initialize' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/connection_adapters/postgresql/oid/specialized_string.rb:12: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activemodel-5.2.6/lib/active_model/type/value.rb:8: warning: The called method `initialize' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/actionpack-5.2.6/lib/action_dispatch/middleware/stack.rb:37: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/actionpack-5.2.6/lib/action_dispatch/middleware/ssl.rb:59: warning: The called method `initialize' is defined here
/var/www/arvados-api/current/app/models/container_request.rb:16: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/associations.rb:1653: warning: The called method `belongs_to' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:21: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:42: warning: The called method `matches?' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:21: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:109: warning: The called method `matches?' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activemodel-5.2.6/lib/active_model/type/registry.rb:20: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/type/adapter_specific_registry.rb:34: warning: The called method `call' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activesupport-5.2.6/lib/active_support/cache.rb:445: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activesupport-5.2.6/lib/active_support/cache.rb:716: warning: The called method `initialize' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/transactions.rb:212: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:260: warning: The called method `transaction' is defined here
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/connection_adapters/abstract/transaction.rb:171: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/var/www/arvados-api/shared/vendor_bundle/ruby/2.7.0/gems/activerecord-5.2.6/lib/active_record/connection_adapters/abstract/transaction.rb:97: warning: The called method `initialize' is defined here
v2/ac2it-gj3su-23wysa5b2pq3whf/uepahsheibai2aiqu0aeX0wiurevae3he1waucae6Gohv1phaeyey5xoiBuakeih

Ready for review at ad2851bce9be401f8feac6570b3958ce93732cfd on branch 18676-anon-token-improvement

Test run: which has the usual flakiness in the wb1 integration tests, re-running at

It looks like a v2 token would not be accepted with the current code. I don't know of a specific case where that would come up, but it seems like it could happen easily, and we could accommodate it easily since it's literally "v2/#{Rails.configuration.ClusterID}-gj3su-anonymouspublic/#{Rails.configuration.Users.AnonymousUserToken}"...

It looks like the configured token will be silently ignored if it's less than 50 chars, which could be confusing for someone who upgraded without noticing the upgrade note / checking their token length. Could we make this a config-check warning instead (maybe add a mandatory bool arg to (*Loader)checkToken() in source:lib/config/load.go) so RailsAPI only has to check whether length>0? And maybe the minimum recommended length should be 32 like the other tokens, and we get the "invalid chars" check for free? (Technically I think minimum length of 1 would be no less secure, but consistency/simplicity is worth something.)

Super nitpicky: although "and" is (was?) more Ruby-fashionable, "&&" has the precedence one expects ("a = b && c" means "a = (b && c) but "a = b and c" means "(a = b) and c") ... IMO it is a better habit to use "&&" in logical expressions. I also find it easier to read, fwiw.

Rest LGTM. Hurray for deleting setup steps, especially rake tasks!

Tom Clegg wrote:

It looks like a v2 token would not be accepted with the current code. I don't know of a specific case where that would come up, but it seems like it could happen easily, and we could accommodate it easily since it's literally "v2/#{Rails.configuration.ClusterID}-gj3su-anonymouspublic/#{Rails.configuration.Users.AnonymousUserToken}"...

True. I wonder if we need to make this change now? Putting an entire proper V2 token in the config file has never worked afaik.

It looks like the configured token will be silently ignored if it's less than 50 chars, which could be confusing for someone who upgraded without noticing the upgrade note / checking their token length. Could we make this a config-check warning instead (maybe add a mandatory bool arg to (*Loader)checkToken() in source:lib/config/load.go) so RailsAPI only has to check whether length>0? And maybe the minimum recommended length should be 32 like the other tokens, and we get the "invalid chars" check for free? (Technically I think minimum length of 1 would be no less secure, but consistency/simplicity is worth something.)

Good idea, I've made that change, thanks.

Super nitpicky: although "and" is (was?) more Ruby-fashionable, "&&" has the precedence one expects ("a = b && c" means "a = (b && c) but "a = b and c" means "(a = b) and c") ... IMO it is a better habit to use "&&" in logical expressions. I also find it easier to read, fwiw.

OK! Fixed.

Ready for another look at 053f74285455278bed87cd4b3dc0df2adffb3b9c

Tests at
, wb1 integration tests re-run at

The boolean expressions in checkToken() seem a bit obfuscated -- (mandatory || len(token) > 0) is just a long way to say len(token) > 0 because the first clause returns early if (mandatory && len(token) 0) -- maybe it would be clearer to put "if !mandatory && token "" { return nil } at the top?

True. I wonder if we need to make this change now? Putting an entire proper V2 token in the config file has never worked afaik.

I'm thinking either we make the change now, or we wait for someone/ourselves to fall into the trap, troubleshoot, and file a bug report. Seems like less trouble to fix it now.

To clarify, I meant we should preserve the existing behavior of accepting "Authorization: Bearer v2/.../{secret}" from API clients as a equivalent to "Authorization: Bearer {secret}" ... I wasn't thinking about having a v2 token in the config file.

Rest LGTM

Tom Clegg wrote:

The boolean expressions in checkToken() seem a bit obfuscated -- (mandatory || len(token) > 0) is just a long way to say len(token) > 0 because the first clause returns early if (mandatory && len(token) 0) -- maybe it would be clearer to put "if !mandatory && token "" { return nil } at the top?

Yes, of course, I've simplified it that way.

True. I wonder if we need to make this change now? Putting an entire proper V2 token in the config file has never worked afaik.

I'm thinking either we make the change now, or we wait for someone/ourselves to fall into the trap, troubleshoot, and file a bug report. Seems like less trouble to fix it now.

To clarify, I meant we should preserve the existing behavior of accepting "Authorization: Bearer v2/.../{secret}" from API clients as a equivalent to "Authorization: Bearer {secret}" ... I wasn't thinking about having a v2 token in the config file.

We had a chat about this - the code already handles V2 tokens but an additional check that the token uuid matches the anon token UUID is advised. I've made that change.

Rest LGTM

629cd91ffca67d6de5ad4dbe9854a064f9e26820 on branch 18676-anon-token-improvement

Tests at

LGTM, thanks!

• If the configured token happens to contain two "/" but doesn't start with "v2/", use the generic "bad chars" error instead of "token is a full V2 token"
• I suppose we should continue accepting the literal v2 token that was in the config file, even though the UUID is not the new predictable one (in case it has been copied & pasted into some client). Presumably we would do this by accepting any uuid with the right cluster id and infix?

Tom Clegg wrote:

Couple of suggestions

• If the configured token happens to contain two "/" but doesn't start with "v2/", use the generic "bad chars" error instead of "token is a full V2 token"

Sure, done.

• I suppose we should continue accepting the literal v2 token that was in the config file, even though the UUID is not the new predictable one (in case it has been copied & pasted into some client). Presumably we would do this by accepting any uuid with the right cluster id and infix?

I don't think that's needed - if a full v2 token is the config file on upgrade, a corresponding database record will exist (or, it never worked!), and that would still work, right? It just wouldn't use the new automatic anonymous token code path.

63645c871246a61a2148b259f10d2fedf30e8df8 on branch 18676-anon-token-support-v2-in-config

Tests at

if strings.Index(token, "v2/") == -1 should probably be if strings.Index(token, "v2/") != 0 ... or if !strings.HasPrefix(token, "v2/")

• I suppose we should continue accepting the literal v2 token that was in the config file, even though the UUID is not the new predictable one (in case it has been copied & pasted into some client). Presumably we would do this by accepting any uuid with the right cluster id and infix?

I don't think that's needed - if a full v2 token is the config file on upgrade, a corresponding database record will exist (or, it never worked!), and that would still work, right? It just wouldn't use the new automatic anonymous token code path.

Oh, that's right. I forgot the existing db records would still exist.

But that reminds me -- I think we need to add scopes: ["GET /"] to the new implementation.

Tom Clegg wrote:

if strings.Index(token, "v2/") == -1 should probably be if strings.Index(token, "v2/") != 0 ... or if !strings.HasPrefix(token, "v2/")

Ah, right, that's more precise. Fixed.

• I suppose we should continue accepting the literal v2 token that was in the config file, even though the UUID is not the new predictable one (in case it has been copied & pasted into some client). Presumably we would do this by accepting any uuid with the right cluster id and infix?

I don't think that's needed - if a full v2 token is the config file on upgrade, a corresponding database record will exist (or, it never worked!), and that would still work, right? It just wouldn't use the new automatic anonymous token code path.

Oh, that's right. I forgot the existing db records would still exist.

But that reminds me -- I think we need to add scopes: ["GET /"] to the new implementation.

Yes! Added.

608e8f79c3fb5cb7077fce4a0b497c5c93d6d6d0 on branch 18676-anon-token-support-v2-in-config

Tests at

re-running api server tests which had a git checkout problem:

LGTM, thanks!