Fixes #13749 - Handle Base64 and binary LDAP avatars #5630
Conversation
|
Issues: #13749 |
| @@ -214,12 +214,21 @@ def avatar_path | |||
| end | |||
|
|
|||
| def store_avatar(avatar) | |||
| avatar = avatar.to_utf8 | |||
| avatar = avatar.instance_of?(Net::BER::BerIdentifiedString) ? avatar : | |||
There was a problem hiding this comment.
Style/MultilineTernaryOperator: Avoid multi-line ternary operators, use if or unless instead.
| @@ -214,12 +214,22 @@ def avatar_path | |||
| end | |||
|
|
|||
| def store_avatar(avatar) | |||
| avatar = avatar.to_utf8 | |||
| if !avatar.instance_of?(Net::BER::BerIdentifiedString) | |||
There was a problem hiding this comment.
this if/else is missing end, causing all Foreman tests failures :-)
| @@ -214,12 +214,22 @@ def avatar_path | |||
| end | |||
|
|
|||
| def store_avatar(avatar) | |||
| avatar = avatar.to_utf8 | |||
| if !avatar.instance_of?(Net::BER::BerIdentifiedString) | |||
There was a problem hiding this comment.
Style/NegatedIf: Favor unless over if for negative conditions.
|
[test upgrade] |
|
Hello folks, did the test, infos on the link below. https://bugzilla.redhat.com/show_bug.cgi?id=1573243#c8 @dLobatog works fine but was necessary change the folder location one more change on the code. Thank you all. |
There was a problem hiding this comment.
@waldirio Thanks, you're totally right.
app/helpers/application_helper.rb
373: image_tag("avatars/#{user.avatar_hash}.jpg", html_options)
image_tag looks into public/images so that should really be the path. I'll fix it asap. Thanks again!
Before this change, we would always try to Base64.decode absolutely any avatar. This resulted in broken images, as sometimes the avatars are automatically turned into binary by net/ldap. In such cases, we want to save that directly, no decoding needed. We also had a minor bug - when a user gets its LDAP avatar updated, the old one could not be deleted. This results in *broken* log in. The bug was quite minor, the variable where the file was supposed to be was simply undefined.
|
I don't think the Katello test error is related with this PR at all - https://ci.theforeman.org/job/test_develop_pr_katello/7390/database=postgresql,ruby=2.4,slave=fast/testReport/junit/(root)/Katello__DockerMetaTagTest/test_import_meta_tags/ |
|
Hello folks -removed-
Feel free to use this server
Ps.: Just internal server and used for test, nothing related to production or personal use, just lab. Ps.: Ldap, your account with your picture / avatar. Edit (lzap): Removed IP address, hostname and passwords. |
|
Folks, one additional info We can see the similar behaviour on Satellite 6.2, the point is, on 6.2 the avatar didn't show, on 6.3 we can see the long hash on the avatar place. Best |
|
@waldirio I was able to confirm with your box. 🥇 |
|
@waldirio you want to remove the hostname, IP and credentials from here? :-) |
|
Hey @lzap Yeap, feel free, just take a note if you need to future tests. Best |
Before this change, we would always try to Base64.decode absolutely any
avatar. This resulted in broken images, as sometimes the avatars are
automatically turned into binary by net/ldap. In such cases, we want to
save that directly, no decoding needed.
We also had a minor bug - when a user gets its LDAP avatar updated, the
old one could not be deleted. This results in broken log in. The bug
was quite minor, the variable where the file was supposed to be was
simply undefined.
@waldirio it'd be very helpful if you could test this one on your systems. Thanks!