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

Remove encrypted from surveys #867

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

przemkalit
Copy link
Contributor

What does this PR do?

Recently, I've discovered that $encrypted$ is exported with the job template/workflow when a password is set in the survey field. During the import, this causes an issue, and the survey is not imported.

How should this be tested?

  1. Create the job template with survey, that has a password field, and set password in default choice field.
  2. Export the job template with filetree_create.
  3. There should not be any $encrypted$ in the export.

Is there a relevant Issue open for this?

N/A

Other Relevant info, PRs, etc

N/A

Copy link
Collaborator

@Tompage1994 Tompage1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correctly this will set the value to ''.

This doesn't feel like a good idea because this will mean we blank out the password. At least having $encrypted$ means people know where they need to replace values.

An alternate strategy here would be to raise a PR against the lookup plugin to add a flag to not return values with $encrypted$ and instead omit them from the results

@przemkalit
Copy link
Contributor Author

przemkalit commented Jul 15, 2024

Okay the error looks like this:
"Failed to update survey: $encrypted$ is a reserved keyword, may not be used for new default in position 13."
what if I just remove the dollars and leave only encrypted?

@Tompage1994
Copy link
Collaborator

Okay the error looks like this: "Failed to update survey: $encrypted$ is a reserved keyword, may not be used for new default in position 13." what if I just remove the dollars and leave only encrypted?

Ah I see. I hadn't read properly that this was within surveys so setting a default of being blank may be our best solution here as you've done. I'll approve for now but I'll request someone else takes a look

Copy link
Contributor

@ivarmu ivarmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed changes gives undesired results:

$ansible-playbook -i inventory.yaml example.yaml

PLAY [play all] *****************************************************************************************************************************************************************************************************************************

TASK [debug] ********************************************************************************************************************************************************************************************************************************
ok: [hosta] => {
    "msg": [
        "REDHAT_PASSWORD: \"$encrypted$\"\n",
        "REDHAT_PASSWORD: \"\\'\\'\"\n"
    ]
}

PLAY RECAP **********************************************************************************************************************************************************************************************************************************
hosta                      : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

Instead of replace("$encrypted$", "\'\'"), it should be replace("$encrypted$", "")

ansible-playbook -i inventory.yaml example.yaml

PLAY [play all] *****************************************************************************************************************************************************************************************************************************

TASK [debug] ********************************************************************************************************************************************************************************************************************************
ok: [hosta] => {
    "msg": [
        "REDHAT_PASSWORD: \"$encrypted$\"\n",
        "REDHAT_PASSWORD: \"\"\n"
    ]
}

PLAY RECAP **********************************************************************************************************************************************************************************************************************************
hosta                      : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

@przemkalit
Copy link
Contributor Author

Okay, I left only the empty double quotes.

Copy link
Contributor

@ivarmu ivarmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ivarmu ivarmu enabled auto-merge (squash) July 16, 2024 13:23
@ivarmu ivarmu merged commit ebd6f38 into redhat-cop:devel Jul 16, 2024
10 of 13 checks passed
@adonisgarciac
Copy link
Contributor

adonisgarciac commented Jul 16, 2024

I don't like this change at all @Tompage1994 @ivarmu. If you export a WF or a JT with any encrypted data either in a survey or in the WF/JT itself, that value should be put in place vaulted or it has to be omitted.

With this change, if someone exports a JT/WF, default password of survey will be replaced for nothing which could ends up in an unexpected situation.

I'd prefer to have to review things manually like passwords after export objects with filetree_create rather than leave filetree_create exports objects with false values of some parameters

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