Arvados

23210-stricter-scope-validation @ 55d21a856d5a43df2f5564757db55b9754139da2

developer-run-tests: #4923

• ✅ All agreed upon points are implemented / addressed.
  • Credentials are validated on create and update according to the given flowchart.
  • There is a { credential_class: [regexp] } hash in the credential model that can be updated when we add support for more credential classes. Something like this can be moved to the discovery document (or somewhere else widely accessible) so that workbench and other components can use it in the future if we want.
• ✅ Anything not implemented (discovered or discussed during work) has a follow-up story.
  • #23230
• ✅ Code is tested and passing, both automated and manual, what manual testing was done is described
• ✅ The tested code incorporates recent main branch changes.
• n/a New or changed UX/UX and has gotten feedback from stakeholders.
• ✅ Documentation has been updated.
• ✅ Behaves appropriately at the intended scale (describe intended scale).
• ✅ Considered backwards and forwards compatibility issues between client and server.
• ✅ Follows our coding standards and GUI style guidelines.

Lisa Knox wrote in #note-8:

23210-stricter-scope-validation @ 55d21a856d5a43df2f5564757db55b9754139da2

Implementation-wise this is all good. These comments are all about maintainability and style.

The validation error message seem to be inconsistent describing the subject. Some use "Credential class" while others use "Credential_class". Scopes errors start with the repetitive "Scopes Scopes". Does Rails automatically add the field name to the start of the error message? If so, our provided message should not repeat it. If not, then we should discuss and agree how we want to stylize field names, and use that consistently. I am wondering if Tom established a style when he did the big validation branch.

The new tests cover a great set of cases. It would be nicer if the list of cases was at the top level, and then test was called inside that each loop. The advantage of doing it this way is all the test cases will run regardless of how many of them fail, and the information you get about which succeeded vs. which failed can help you hone in on the bug in any future development. For illustration, consider something like this:

 test_credential = {
    description: "the credential for test",
    external_id: "my_username",
    secret: "my_password",
    expires_at: Time.now + 2.weeks,
    credential_class: "arv:aws_access_key",
    scopes: nil,
  }

  [
    {
      body: {
        name: "with valid scopes",
        scopes: ["s3://my-bucket", "s3://*"],
      },
      error_msg: nil
    },
    {
      body: {
        name: "with no scopes",
      },
      error_msg: /Scopes Scopes cannot be blank for credential class arv:aws_access_key/
    },
    # … more test cases…
  ].each do |tc|
    test "validation for credential #{tc[:body][:name]}" do
      post "/arvados/v1/credentials",
           params: {:format => :json,
                    credential: test_credential.merge(tc[:body]),
                   },
           headers: auth(:active),
           as: :json
      if tc[:error_msg].nil?
        assert_response :success
      else
        assert_response 422
        assert_match(tc[:error_msg], json_response["errors"][0])
      end
    end
  end

Note that body can still override the fields in test_credential as needed, so you can test other credential_class values, etc.

validate_credential_class_and_scopes has inconsistent indentation, and validate_scopes_for_implemented_credential_class has a blank line with extra whitespace. Please fix both.

Thanks.

23210-stricter-scope-validation @ 1b994233a8df046411b9cb246d9a38de9933b05b

developer-run-tests: #4925

So Rails does add the field name to the start of the error message in certain cases, and in some cases it will detect if the error message starts with the field name and, if not, prepend it. I am not 100% clear on how to either enforce or disallow this behavior, but as doing either would affect all of our other tests, it's probably not worth pursuing. The rule that we appear to be following every where else is to not put the field name in the error message, so I've removed it in the tests I wrote. The "Credential class" string misalignment is therefore moot.

The new tests cover a great set of cases. It would be nicer if the list of cases was at the top level...

Tom actually mentioned this in his feedback for #23181 but it completely slipped my mind. I've made the adjustment as suggested.

validate_credential_class_and_scopes has inconsistent indentation, and validate_scopes_for_implemented_credential_class has a blank line

Fixed

Lisa Knox wrote in #note-10:

23210-stricter-scope-validation @ 1b994233a8df046411b9cb246d9a38de9933b05b

LGTM, thank you.