Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for edit configuration_script_payloads #1231

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Jun 20, 2023

Workflows have a credentials jsonb column which allows users to map credentials to values being requested by the workflow. This means we have to allow for a user to edit configuration_script_payloads to do this mapping.

PATCH /api/configuration_script_payloads/:id {"credentials":{"api_user":{"credential_ref": "manageiq_api", "credential_field": "userid"}, "api_password":{"credential_ref": "manageiq_api", "credential_field": "password"}}

Follow-up:

TODO:

  • Fail the request if any of the credentials can't be found
  • Add specs for authentications owned by other user/tenant

@agrare agrare requested a review from bdunne as a code owner June 20, 2023 18:42
@agrare agrare requested review from kbrock and Fryguy June 21, 2023 15:48
@Fryguy Fryguy self-assigned this Jul 5, 2023
@Fryguy
Copy link
Member

Fryguy commented Jul 5, 2023

Does this allow the user to change things other than the credentials? If it's coming from git, we probably shouldn't allow it, except those specific fields/associations.

@agrare agrare force-pushed the add_edit_support_configuration_script_payloads branch from b3d2542 to 158fd90 Compare July 7, 2023 14:24
@agrare
Copy link
Member Author

agrare commented Jul 7, 2023

@Fryguy updated to only allow updates to the name, payload, and payload_type if there is no linked configuration_script_source, and only updates to the description and credentials otherwise.

config/api.yml Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Aug 2, 2023

As discussed, I think having the API for credential mapping be a 2 step process should be simplified. That is right now a user must

  1. Give permission to the workflow for a specific credential
  2. Map fields from that credential into fields from the asl

Instead, in step 2 we already know, through the act of mapping, that they also want to give permission, so we can give permission implicitly under the covers.

So, given a payload of

{
  "credentials": {
    "api_user": "$.manageiq_api.userid",
    "api_password": "$.manageiq_api.password"
  }
}

we would just give permission to manageiq_api directly if not already given (and ensure this is a credential they have access to in the first place), then update the credentials field accordingly.


Separately we discussed the payload itself. A user could technically hardcode things like usernames, and then we wouldn't be able to tell apart references vs strings. Additionally parsing strings is a potential source of error. Instead a payload like the following would allow us to differentiate:

{
  "credentials": {
    "api_user": "root",
    "api_password": {
      "credential_ref": "manageiq_api",
      "credential_field": "password"
    }
  }
}

@agrare agrare changed the title Add support for edit configuration_script_payloads [WIP] Add support for edit configuration_script_payloads Aug 3, 2023
@miq-bot miq-bot added the wip label Aug 3, 2023
@agrare
Copy link
Member Author

agrare commented Aug 3, 2023

ManageIQ/manageiq-providers-workflows#40 changes the expected credential payload format

@agrare agrare force-pushed the add_edit_support_configuration_script_payloads branch 2 times, most recently from e339762 to deb22d6 Compare August 3, 2023 20:44
@agrare agrare force-pushed the add_edit_support_configuration_script_payloads branch from deb22d6 to 75bbc75 Compare August 3, 2023 21:00
@agrare agrare closed this Aug 7, 2023
@agrare agrare reopened this Aug 7, 2023
@agrare agrare force-pushed the add_edit_support_configuration_script_payloads branch from 4a1f00b to 75bbc75 Compare August 15, 2023 19:20
Automatically add authentications referenced in the credentials payload
to the authentications_configuration_script_payload join table
@agrare agrare force-pushed the add_edit_support_configuration_script_payloads branch from 75bbc75 to f5de1e7 Compare August 16, 2023 15:35
@agrare agrare changed the title [WIP] Add support for edit configuration_script_payloads Add support for edit configuration_script_payloads Aug 16, 2023
@miq-bot
Copy link
Member

miq-bot commented Aug 16, 2023

Checked commits agrare/manageiq-api@3c00159~...f5de1e7 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@miq-bot miq-bot removed the wip label Aug 16, 2023
@kbrock kbrock merged commit 9a77ef2 into ManageIQ:master Aug 16, 2023
3 checks passed
@kbrock kbrock self-assigned this Aug 16, 2023
@agrare agrare deleted the add_edit_support_configuration_script_payloads branch August 16, 2023 17:27
@bdunne
Copy link
Member

bdunne commented Aug 16, 2023

Backported to quinteros in commit cd280b1.

commit cd280b117ab9f08e7f381f55c36228f4a940d0aa
Author: Keenan Brock <[email protected]>
Date:   Wed Aug 16 13:21:59 2023 -0400

    Merge pull request #1231 from agrare/add_edit_support_configuration_script_payloads
    
    Add support for edit configuration_script_payloads
    
    (cherry picked from commit 9a77ef21ecb166e529a77a9b51f6e299ac56d034)

bdunne pushed a commit that referenced this pull request Aug 16, 2023
…cript_payloads

Add support for edit configuration_script_payloads

(cherry picked from commit 9a77ef2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants