• 
      

    Fix up authentication issues with the HostingServiceAccount API.

    Review Request #14128 — Created Aug. 29, 2024 and submitted

    Information

    Review Board
    release-7.x

    Reviewers

    The HostingServiceAccountResource has always been primarily used in a
    read capacity, and the implementation for create wansn't able to
    create hosting service accounts for any kind of authenticated services.
    There are two big issues here:

    • HostingServiceAccountResource.create() was manually calling
      service.authorize, but with a legacy function signature that did not
      work for everything. We started the process of migrating
      HostingService subclasses to use a new credentials dict, but that
      still isn't implemented everywhere, and for services that do use it,
      the API endpoint was just completely broken.
    • Many HostingService instances have custom authentication forms which
      add or remove fields. These forms are what actually create the content
      of the credentials dict. In the case of the reported bug, GitLab's
      form uses a private_token field in credentials rather than
      password.

    This change makes it so the API resource instantiates the hosting
    service's auth form and then use that form to create the
    HostingServiceAccount. This makes it so we're always calling authorize
    with the expected kwargs for the given service, and any custom fields
    which are relied on will be properly validated and applied.

    This adds a few new unit tests that use the GitLab service, since that's
    an example of something that uses a custom auth form and uses
    non-standard keys in the credentials dict.

    Ran unit tests.

    Summary ID
    Fix up authentication issues with the HostingServiceAccount API.
    The `HostingServiceAccountResource` has always been primarily used in a read capacity, and the implementation for `create` wansn't able to create hosting service accounts for any kind of authenticated services. There are two big issues here: - `HostingServiceAccountResource.create()` was manually calling `service.authorize`, but with a legacy function signature that did not work for everything. We started the process of migrating HostingService subclasses to use a new `credentials` dict, but that still isn't implemented everywhere, and for services that do use it, the API endpoint was just completely broken. - Many `HostingService` instances have custom authentication forms which add or remove fields. These forms are what actually create the content of the `credentials` dict. In the case of the reported bug, GitLab's form uses a `private_token` field in credentials rather than `password`. This change makes it so the API resource instantiates the hosting service's auth form and then use that form to create the HostingServiceAccount. This makes it so we're always calling `authorize` with the expected kwargs for the given service, and any custom fields which are relied on will be properly validated and applied. This adds a few new unit tests that use the GitLab service, since that's an example of something that uses a custom auth form and uses non-standard keys in the credentials dict. Testing Done: Ran unit tests.
    ab33f0a379dd5e5611dcbeb00f8a2c87973a0ab9
    Description From Last Updated

    line too long (99 > 79 characters) Column: 80 Error code: E501

    reviewbotreviewbot

    whitespace after '{' Column: 22 Error code: E201

    reviewbotreviewbot

    whitespace before '}' Column: 49 Error code: E202

    reviewbotreviewbot

    Small nit, but let's alphabetize these keys.

    chipx86chipx86

    Feel free to discard: This could be simplified to a dictionary expression, and you'd get the typing for free.

    chipx86chipx86
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    david
    maubin
    1. Ship It!
    2. 
        
    david
    chipx86
    1. 
        
    2. Show all issues

      Small nit, but let's alphabetize these keys.

    3. reviewboard/webapi/resources/hosting_service_account.py (Diff revision 3)
       
       
       
       
       
       
       
       
      Show all issues

      Feel free to discard: This could be simplified to a dictionary expression, and you'd get the typing for free.

      1. I feel like nested comprehensions are... difficult to comprehend.

    4. 
        
    david
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-7.x (adbefff)