Arvados

21910-remove-api_client_id @ d36e94985953eabdaec5f75ad9f609f39c877738 -- developer-run-tests: #4343

21910-remove-api_client_id @ 451af2f0f399080133d225ee30572e38a7e9ec8a -- developer-run-tests: #4345 (same code as d36e94985, upgrade notes updated)

• All agreed upon points are implemented / addressed.
  • ✅ api_client_id fields removed, arvados/v1/api_clients endpoints removed
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • see below
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • tests updated, see below
• Documentation has been updated.
  • ✅ api_client_id fields and .../api_clients endpoints removed from docs
• Behaves appropriately at the intended scale (describe intended scale).
  • N/A
• Considered backwards and forwards compatibility issues between client and server.
  • To address compatibility with earlier workbench2 versions in a federation scenario (and other older clients that might have the same issue), we now silently drop modified_by_client_uuid from select params. To check whether the missing field makes workbench2 crash, I made a test branch that reverts services/workbench2 to main (except some test setup code): 21910-remove-api_client_id-oldworkbench @ 8c233c045333592b079b77830252e1bccdf0ed35 -- developer-run-tests-services-workbench2: #981
  • Should we also silently ignore the other removed fields (api_client_id)? Or is it enough that we already advertised them as deprecated in previous versions?
• Follows our coding standards and GUI style guidelines.
  • ✅

I removed a test ("saving an unchanged client still makes a log" in source:services/api/test/unit/log_test.rb) without fully understanding why it existed. It tested that saving an api_client record after a.is_trusted = a.is_trusted resulted in an "update" log entry but didn't update the modified_at timestamp -- but I don't know why that was true or desirable. I tried updating the test to use a collection (a.name = a.name) but that does not generate a log entry, which seems fine. Possibly Rails behavior changed since the test was first written, but the test continued to pass because by then is_trusted had become a computed field, and therefore behaved differently than regular attributes? The test was added in 54978ac2108a5b1913f641d480143356659a413a for #2375.

I'm trying to figure out how these changes affect the semantics of the IssueTrustedTokens configuration option.

Previously, it seems that if IssueTrustedTokens was true, and the api_client was trusted, then you could view other tokens.
If either IssueTrustedTokens was false or api_client was not trusted, then you could not view other tokens.

With this branch, the api_client check is eliminated, so IssueTrustedTokens is the only option determining whether API calls to create or query tokens are permitted or not. Unlike other options where admins have different behavior or the ability to opt out, this affects regular users and admins the same way.

The name IssueTrustedTokens and config documentation are misleading and should be revisited. Also, https://doc.arvados.org/v2.7/admin/token-expiration-policy.html refers to a non-existing TrustLoginTokens configuration option which I think got changed to IssueTrustedTokens for some reason -- but the behavior being described is not exactly the behavior actually enforced by setting IssueTrustedTokens: false.

The thing that we probably want to do is have is the power to create additional tokens to attach to the token itself, and then options like IssueTrustedTokens would affect how that flag is set in various situations. This requires a design conversation so let's make a follow-up ticket.

The obvious (but incorrect) assumption of the purpose of modified_by_client_uuid is that it would provide some kind of tracking information (IP address etc) about the client that did the operation. This is wrong, which is why the modified_by_client_uuid field is going away, but improving the auditing of who did what is something we should think about doing. This also requires a design conversation so let's make a follow-up ticket.

With the current branch one can no longer configure Arvados such that Workbench issue non-trusted tokens and the admin can still generate trusted tokens some other way.

Also, now that the trust flag is determined at time of use rather than time of creation, the original TrustLoginTokens name seems more apt than IssueTrustedTokens.

I think a per-token flag would make more sense.

Meanwhile, what should we do with this branch? It touches a lot of files and will probably turn into a mess of merge conflicts if we let it sit.

Maybe just update the documentation page to reflect the "time of use" difference, with the expectation that we'll rename the config and/or add different functionality in a follow-up before the next release?

Tom Clegg wrote in #note-7:

With the current branch one can no longer configure Arvados such that Workbench issue non-trusted tokens and the admin can still generate trusted tokens some other way.

Also, now that the trust flag is determined at time of use rather than time of creation, the original TrustLoginTokens name seems more apt than IssueTrustedTokens.

I think a per-token flag would make more sense.

Right, but I think it merits some discussion about what we are really trying to do before we implement it. As far as I know this option has never actually been used, we just put it in as a "just in case".

Meanwhile, what should we do with this branch? It touches a lot of files and will probably turn into a mess of merge conflicts if we let it sit.

I agree, we want to merge it.

Maybe just update the documentation page to reflect the "time of use" difference, with the expectation that we'll rename the config and/or add different functionality in a follow-up before the next release?

Yes, let's fix the documentation (that includes the config file comment) to reflect the actual behavior (and also refer to the correct config option on the token expiration policy page).

21910-remove-api_client_id @ 9f860466ee8137692af63b6692c4786320efeb7b

Merged main. Updated doc page and config.default.yml comment.

Tom Clegg wrote in #note-10:

21910-remove-api_client_id @ 9f860466ee8137692af63b6692c4786320efeb7b

Merged main. Updated doc page and config.default.yml comment.

1. The documentation update says "For users or service accounts that need to tokens with fewer restrictions, the admin can "create a token at the command line":user-management-cli.html#create-token using the SystemRootToken." but I'm not sure that is actually true, the hook in api_client_authorizations_controller is:

  before_action :check_issue_trusted_tokens, :except => [:current]

Which applies to all API calls and doesn't distinguish between admin tokens, system root token, etc (Unless controller intercepts this as does something different). So we need to either change the behavior or change the documentation.

2. This message was never very helpful, now it is even less so because 'API client' is not even a concept any more. Can we update the message?

    send_error('Forbidden: this API client cannot manipulate other clients\' access tokens.',
               status: 403)

21910-remove-api_client_id @ e31c49ff8b05131ae389ad6c89db8df576844480 -- developer-run-tests: #4382

...plus documentation update at e8c929ce89a206c5da74ea18a5e88fb3f26ad2fb

• system root token is always "trusted" regardless of IssueTrustedTokens config.
• error message updated, now refers to IssueTrustedTokens config.

Tom Clegg wrote in #note-12:

21910-remove-api_client_id @ e31c49ff8b05131ae389ad6c89db8df576844480 -- developer-run-tests: #4382

...plus documentation update at e8c929ce89a206c5da74ea18a5e88fb3f26ad2fb

• system root token is always "trusted" regardless of IssueTrustedTokens config.
• error message updated, now refers to IssueTrustedTokens config.

• api/test/integration/api_client_authorizations_api_test.rb has several other instances of using "OAuth2" instead of "Bearer" in the authorization header.

• I'm a little unclear how these tests work. It seems that it provides the API token associated with "system_user" from the fixtures, which is systemusertesttoken1234567890aoeuidhtnsqjkxbmwvzpy

The token trust check is current_api_client_authorization.andand.api_token == Rails.configuration.SystemRootToken.

It's not obvious how it ensures that SystemRootToken is the same as the token in system_user fixture. Or if something else is happening here.

Otherwise, this LGTM, I think this gives a minimal escape hatch that makes IssueTrustedTokens actually usable.

developer-run-tests: #4387

• Added comment to system_root fixture (tldr: that's where SystemRootToken comes from in tests)
• Updated all of the "Authorization: OAuth2 ..." instances I found in the code base to "Bearer ..." (except the compatibility test)

Tom Clegg wrote in #note-15:

21910-remove-api_client_id @ c4190bdf52343953cfbbfa6458fe93467e2a259e -- developer-run-tests: #4387

• Added comment to system_root fixture (tldr: that's where SystemRootToken comes from in tests)

Thanks, I suspected something like that was the case, but it is not at all intuitive that setting up the config file for the Ruby on Rails test suite would depend on running some Python code.

• Updated all of the "Authorization: OAuth2 ..." instances I found in the code base to "Bearer ..." (except the compatibility test)

Wow, that was lingering in a lot more places than I would have expected.

LGTM.