Added ProfileImageFormat support#174
Conversation
| return ProfileImageFormat.Url; | ||
| } | ||
|
|
||
| throw new InvalidFormatException($"Image attribute was a valid URI but had an invalid Scheme"); |
There was a problem hiding this comment.
This is the part I am least confident in. We shouldn't support any url with non http/https schema given we are making a request via a HTTP client. If however there are other schemas that the HttpClient supports (ftp & co), this default mode will fail even if the request might have succeeded. I am content given that if they have a non http url they can just manually change the format to Url which ignores this logic altogether and just lets the HttpClient fail with whatever it got given but i don't know is that is behavior the maintainers want.
Some feedback from both maintainers and users would be greatly appreciated.
There was a problem hiding this comment.
I am content given that if they have a non http url they can just manually change the format to Url
I feel like that's honestly the best way to do it. There would also need to be a change in documentation to let the user's know that though. Maybe added to the footer of the input?
There was a problem hiding this comment.
Should give some context to what failed, this error message itself isn't very helpful
| { | ||
| _logger = logger; | ||
| _applicationHost = applicationHost; | ||
| _httpClient = new(() => httpClientFactory.CreateClient()); |
There was a problem hiding this comment.
Create the HttpClient when needed, not once. This class's lifetime is the Jellyfin server's lifetime.
There was a problem hiding this comment.
Oh so we want to create 1 client per login? I thought it smarter to preserve the same client throughout the plugin lifetime.
Happy to change just wanted to utilize the least resources required
| return ProfileImageFormat.Url; | ||
| } | ||
|
|
||
| throw new InvalidFormatException($"Image attribute was a valid URI but had an invalid Scheme"); |
There was a problem hiding this comment.
Should give some context to what failed, this error message itself isn't very helpful
|
Done @crobibero, Would you mind re-reviewing? |
| } | ||
|
|
||
| private HttpClient HttpClient => _httpClient.Value; | ||
| private HttpClient HttpClient => _httpClientFactory.CreateClient(); |
There was a problem hiding this comment.
| private HttpClient HttpClient => _httpClientFactory.CreateClient(); | |
| private HttpClient HttpClient => _httpClientFactory.CreateClient(NamedClient.Default); |
Might need a new using statement at the top
There was a problem hiding this comment.
Added it. Did need a using statement.
|
done and ready for re-review @crobibero. Sorry for the delay I didn't see the review until it was already time for bed |
|
Any guidance on how to configure this with an Authentik LDAP? Would that even work in the actual state? Tried a few things but nothing work. First I am not sure what I have to specify on LDAP Profile Image Attribute
And I really dont know if specific config should be done on Authentik LDAP configuration to deal with it. We are missing doc or I did not find it (and I searched!) Txs |
Do you have a discord? I can help you out since I currently use authentik :3 |
|
Sorry for bumping this PR, but I believe context can help. While checking the merged commits, I noticed that the synchronization tasked hasn't been updated. Tests on my end with a url demonstrated the lack of implementation (it saves a .jpg containing the url as text). |
I am not quite sure what you mean with this? Are you trying to say that when setting the mode to |
|
Yes, exactly. And I think it may be because the implementation of the synchronization task (there is a dedicated file and object) has not been updated (it does not adapt to the type, but treats everything as bytes) |
|
Very strange According to the implementation I wrote it should make a http request and write whatever it responds with to the image: GetByteArrayAsync docs say that it makes a GET request and returns the response body as a byte stream. This would indicate that your http url returns as its response body a url. Can you confirm that this is the case? I suspect it may be a N+1 problem where @epireyn can you confirm for me what the url returns? |
|
It returns the image in the body. Actually, the url is the image name (and probably relative path).
You implemented the URL for the authentification provider, when a new user is created in Jellyfin (because inexistent). My user already exists, and thus the only way to update the profile picture is through the dedicated scheduled task, defined here. As you can see in the following snippet, the code was not updated accordingly with your changes. |
|
Omg. You are so right. I knew that the code I had written was specifically for the first login but I hadn't realized that there was more. Let me fix that. My bad. |
5 participants
This PR adds a enum ProfileImageFormat allowing the admin to specify whichever format the profile image should be in.
It also has a default value which will try and automatically determine which format the ProfileImage is in to simplify setup and maximize "Just Works" factor
There is an existing PR #166 with similar albeit incomplete functionality which the author has neglected to update for quite some time. Prompting me to raise my own.