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

Fixes #36591 - Fix the parameter value masking #9778

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

girijaasoni
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link
Member

Issues: #36591

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

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

@girijaasoni Your changes also affect the common_parameters page. The value column for parameters with hidden_value shows true instead of asterisks.

@girijaasoni
Copy link
Contributor Author

@girijaasoni Your changes also affect the common_parameters page. The value column for parameters with hidden_value shows true instead of asterisks.

That was because i changed the parameters model, fixed it now :)

@nofaralfasi
Copy link
Contributor

It is working for me now. Just one small note, I think it would be nicer to hide the parameter's value if the Hide value option is enabled, as we do on the common_parameters page.

@girijaasoni
Copy link
Contributor Author

It is working for me now. Just one small note, I think it would be nicer to hide the parameter's value if the Hide value option is enabled, as we do on the common_parameters page.

as per the acceptance criteria in the bz: https://bugzilla.redhat.com/show_bug.cgi?id=2172394, the expected results are mentioned as "The value should be visible even after hide and unhide."

@nofaralfasi
Copy link
Contributor

It is working for me now. Just one small note, I think it would be nicer to hide the parameter's value if the Hide value option is enabled, as we do on the common_parameters page.

as per the acceptance criteria in the bz: https://bugzilla.redhat.com/show_bug.cgi?id=2172394, the expected results are mentioned as "The value should be visible even after hide and unhide."

I believe their intention was that when you hide and unhide the value, you would see the original value again instead of asterisks.

@girijaasoni
Copy link
Contributor Author

It is working for me now. Just one small note, I think it would be nicer to hide the parameter's value if the Hide value option is enabled, as we do on the common_parameters page.

as per the acceptance criteria in the bz: https://bugzilla.redhat.com/show_bug.cgi?id=2172394, the expected results are mentioned as "The value should be visible even after hide and unhide."

I believe their intention was that when you hide and unhide the value, you would see the original value again instead of asterisks.

Thanks @nofaralfasi , fixed it :)

Copy link
Contributor

@nofaralfasi nofaralfasi 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 hide a value and then proceed to input something, when I reveal it again, only the most recent character I typed is visible, and the original value is replaced with asterisks.

@nofaralfasi
Copy link
Contributor

screen-capture.3.webm

@girijaasoni
Copy link
Contributor Author

girijaasoni commented Jul 25, 2023

If I hide a value and then proceed to input something, when I reveal it again, only the most recent character I typed is visible, and the original value is replaced with asterisks.

fixed it but for all types of input except for the boolean type as it is a dropdown in that case and the component we have used is "Select" and it doesn't have a parameter for masking a value and IMHO it's not required as well.

@stejskalleos stejskalleos self-assigned this Jul 25, 2023
@stejskalleos
Copy link
Contributor

ok to test

Copy link
Contributor

@stejskalleos stejskalleos 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 understand it correctly, we are masking the value of the parameter while editing it, right?

My question is, what's the point of hiding value while editing it? If users have permission for editing parameters, then there is no reason to hide the value for them.
IMHO it's a security feature with low to no impact that can be removed, but please feel free to correct me if I miss something.

attributes :id, :name, :parameter_type, :associated_type, :hidden_value?

node do
partial("api/v2/common/show_hidden", :locals => { :value => :value }, :object => @object)
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason to remove this node? We should not change the API,
customers can have their own infra on the top pf our API and this is a breaking change

@@ -135,7 +134,7 @@ export const EditParametersTableRow = ({
<SelectOption value="false" />
</Select>
) : (
<TextArea
<TextInput
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good change. For short strings, yeah, but what if you'll have a parameter with let's say public SSH key, which is a common use case:

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDLLfFKVRh3XWCZBhFZ9VTHk1dnjDAYuyPpGxXUdyaMBEA0mBk7xNW1vP3zikdo4rdn4lg4q1sLeh2MkkmZnBiV+AcetaQ3iceNfRKjCL7hML4fAyKqIC2FuO7zAFWbe1jqHDYz7TiOEIqmfJFCPnHHw2XyqKhLiMjsRqjLuTPeGeDmOOo1aI7KwTM1iyrrJsjKtuBT4navog5TNaTyR+E9KC+KPcTWSG61nA0HyOE/UD2jSVI+8Q13E6H2RfbMmbRzIJPUrxuS5B8Io2LxEr56YZuKeQhsYWJBqtpmvhKQULQo4WdPu+1nyApInXbF25jx0FieBROJ1D+KLl/Uj4Tp4O9ihPAwiUo3F6qK0aJFt01YvxEERFmqFdyI7+z/bOXlj0yO1qnFDphBd0s0uk/3FHcGZRqLtKXywv9aDbprjdNJzDAW6/j93n8yn1aaaaaaaaaaaaaaaaAAAAAAAAAAAAAAAAAAAAMgGLeQIAsWvU6PLwfHq8polj6xntR9XDv3/9mkNBgU= user@localhost.localdomain

That's not something you want to have in the input field.

@adarshdubey-star
Copy link

If I understand it correctly, we are masking the value of the parameter while editing it, right?

My question is, what's the point of hiding value while editing it? If users have permission for editing parameters, then there is no reason to hide the value for them. IMHO it's a security feature with low to no impact that can be removed, but please feel free to correct me if I miss something.

So the BZ was ideally filed for the scenario when you add a parameter and click on the hide button and again unclick it, the value vanished, in the context of editing a parameter the user can surely unmask and change the value and again mask it if it's a critical parameter based on the scenario.

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

🍏 LGTM, fix the issue

@stejskalleos stejskalleos merged commit cc68b53 into theforeman:develop Aug 8, 2023
17 checks passed
@stejskalleos
Copy link
Contributor

Thanks @girijaasoni @nofaralfasi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants