advanced-cms / advanced-reviews

This is an Open Source add-on that improves the reviewing process and lets external users to view & review content items or whole projects without the need to access the Edit Mode. Created and maintained by Bartosz Sekuła and Grzegorz Wiecheć
Apache License 2.0
17 stars 15 forks source link

Avatar image not displayed correct using AD login #149

Closed MorJonSkan closed 4 years ago

MorJonSkan commented 4 years ago

The avatar images are not displayed. Maybe need some instructions of how to add avatar images.. avataricon-not-shown

barteksekula commented 4 years ago

@MorJonSkan What are the urls of those broken images? We have a custom implementation of ICustomAvatarResolver in the sample Alloy https://github.com/advanced-cms/advanced-reviews/blob/master/src/alloy/Business/AlloyReviews/AssetsCustomAvatarResolver.cs However, by default you should see a dummy identicon: https://github.com/advanced-cms/advanced-reviews/blob/master/src/approval-reviews/AvatarsService/ReviewAvatarsHandler.cs#L41

image

gregwiechec commented 4 years ago

@MorJonSkan Maybe AD does not return userName in RouteData and we have 404: https://github.com/advanced-cms/advanced-reviews/blob/0c477afb58abc9e3843b63367a6172eceda2ef03/src/approval-reviews/AvatarsService/ReviewAvatarsHandler.cs#L34

We don't have environment with AD. Is there any chance that you could help us to debug this?

barteksekula commented 4 years ago

The url should be something like: https://your.host.com/review-avatars/current_user_name.jpg

Please debug this line: image

Do you have the correct value in ApplicationSettings.userName?

MorJonSkan commented 4 years ago

@gregwiechec The url of the image will be /review-avatars/{domain}{username}.jpg

barteksekula commented 4 years ago

@MorJonSkan is it possible for you to debug ReviewAvatarsHandler.cs?

MorJonSkan commented 4 years ago

@barteksekula Yes I think I found the issue. The url becomes /review-avatars/{domain}/{useraccountname}.jpg, and the route in AvatarRouteExtensions.cs only "listens" for one segement of username. The route will not be executed or activated.. I haven't tried this with Azure AD but I suspect that there will be a similar issue there.

barteksekula commented 4 years ago

@MorJonSkan good catch, it turns out that using / and \ in the url's path is not a good idea :- ) Maybe you can verify https://github.com/advanced-cms/advanced-reviews/pull/157 It works fine for me. The default avatar provider will just render identicons but maybe you can take i further and create your own implementation that serves images from AD.

MorJonSkan commented 4 years ago

@barteksekula Yes the change looks ok :) Another approach I was think of was to remove the domain part in review-store.tsx and keep the username as before.. not 100% sure if that would work for all scenarios. So if the username contains any / or \ the last part will be used..