Add support for avatars #523

jonesbusy · 2025-02-05T06:09:20Z

Fix for #512 (more details here)

Avatar is an URL given when profile scope is requested (standard OpenID)
Added tests

Other known plugin that support avatar extension point

https://github.com/jenkinsci/avatar-plugin
https://github.com/jenkinsci/azure-ad-plugin
https://github.com/jenkinsci/gravatar-plugin

Testing done

There was some fixes on Jenkins 2.495 for avatar rendering. That's why it would look different

On 2.479.1 (this current plugin jenkins.version). This is was fixed by jenkinsci/jenkins#10180 which should be included on ~3 month LTS

On 2.495 this looks much better

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

jonesbusy · 2025-02-05T06:11:05Z

FYI @timja thanks for the help, jenkinsci/azure-ad-plugin#658 was a good inspiration for it

codecov · 2025-02-05T06:25:55Z

Codecov Report

Attention: Patch coverage is 83.67347% with 8 lines in your changes missing coverage. Please review.

Project coverage is 72.45%. Comparing base (7b42906) to head (91ca44a).
Report is 39 commits behind head on master.

Files with missing lines	Patch %	Lines
...a/org/jenkinsci/plugins/oic/OicAvatarProperty.java	85.00%	1 Missing and 2 partials ⚠️
...a/org/jenkinsci/plugins/oic/OicAvatarResolver.java	50.00%	1 Missing and 2 partials ⚠️
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java	91.30%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #523      +/-   ##
============================================
+ Coverage     72.04%   72.45%   +0.40%     
- Complexity      235      249      +14     
============================================
  Files            18       20       +2     
  Lines          1073     1118      +45     
  Branches        149      154       +5     
============================================
+ Hits            773      810      +37     
- Misses          207      212       +5     
- Partials         93       96       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jtnord · 2025-02-08T22:42:15Z

Thankyou for the contribution, however I have a concern about this.

The OIDC spec has the users picture as a URL (string) in the picture claim (part of the profile claim).
AFAICT this is adding support for a non standard field. If this is the case the. I don't think this should be here as we will end up with request to support all and every way some provider does it in a unique way.

If this is non standard then there should be an extension point that can be implemented and chosen, with (I would say) the only implementation in this plugin should be the standard picture claim, which can just store and return the URL.

jtnord · 2025-02-08T22:48:24Z

keycloak/keycloak#30428 (comment)

jonesbusy · 2025-02-09T05:32:29Z

Thanks for the review. If the URL is the standard, let's use the URL to display the avatar.

I will check then on my side how to get a URL based on the base64 data (maybe some keycloak extension to extract the thumbnail and upload it somewhere, then return the URL in the profile).

I will update the PR harcodingly

jonesbusy · 2025-02-10T06:48:27Z

I updated the PR to use URL instead. Ii work now the same but with URL instead of Base64 (I've temporarly tested with an hardcoded claim "picture"). The mapping is still configurable

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

jonesbusy · 2025-02-12T07:21:00Z

I've pushed 2 commits. One that remove the configuration for the picture field since it's standard on OpenId.

The other one that change to direct link for the avatar.

The URL is now stored as property

<avatarImage>
    <url>http://localhost:8083/realms/ci-local-dev/avatar/ab2a1093-db55-4b61-8711-08ac4c38252e</url>
</avatarImage>

I confirm it work when the image is hosted on different domain (jenkins 8080, keycloak 8083)

jonesbusy · 2025-02-18T08:08:48Z

I rebased the 4 commits

jonesbusy · 2025-02-19T05:42:28Z

Fixed conflicts due to Junit5 migration and squashed commits

Signed-off-by: Valentin Delaye <[email protected]>

jonesbusy · 2025-02-26T08:54:21Z

@jtnord Hi again, would you be able to take a look? It's now using standard picture claim

jtnord · 2025-02-12T09:14:19Z

src/main/java/org/jenkinsci/plugins/oic/OicAvatarProperty.java

+    }
+
+    public String getDisplayName() {
+        return "Openid Connect Avatar";


Suggested change

return "Openid Connect Avatar";

return "OpenID Connect Avatar";

jtnord · 2025-02-12T09:15:21Z

src/main/java/org/jenkinsci/plugins/oic/OicAvatarProperty.java

+        @Override
+        @NonNull
+        public String getDisplayName() {
+            return "Openid Connect Avatar";


it is OpenID not OpenId or Openid (the ID is for Identity Document not Id as in Identity).
(yes this plugin should have been called oidc-auth but that ship sailed, but the user facing stuff should be correct)

Suggested change

return "Openid Connect Avatar";

return "OpenID Connect Avatar";

I will fix for the new code. However there are multiple occurence on the plugin

For example

OicSecurityRealm.DisplayName = Se connecter avec Openid Connect OicSecurityRealm.ClientIdRequired = Le client id est requis. OicSecurityRealm.ClientSecretRequired = Le client secret est obligatoire. OicSecurityRealm.URLNotAOpenIdEnpoint = L'URL ne semble pas d\u00e9crire des terminaux OpenID Connect

Thanks, I thought I had fixed them, but obviously I hadn't. filed #542 so I do not forget.

jtnord · 2025-02-12T09:17:31Z

src/main/java/org/jenkinsci/plugins/oic/OicAvatarProperty.java

+import java.util.logging.Logger;
+
+public class OicAvatarProperty extends UserProperty {
+    private static final Logger LOGGER = Logger.getLogger(OicAvatarProperty.class.getName());


unused logger

Suggested change

private static final Logger LOGGER = Logger.getLogger(OicAvatarProperty.class.getName());

jtnord · 2025-02-17T22:42:38Z

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

+                OicAvatarProperty oicAvatarProperty = new OicAvatarProperty(avatarImage);
+                user.addProperty(oicAvatarProperty);
+            } else {
+                LOGGER.finest("No avatar URL found");


Would It be useful to know which user this was for?
Also should this not remove the property (e.g. if someone deletes there picture from the profile, we would continue to show it?)

jtnord · 2025-02-17T22:45:45Z

src/test/java/org/jenkinsci/plugins/oic/PluginTest.java

@@ -834,6 +858,14 @@ private static void assertTestUserEmail(User user) {
                user.getProperty(Mailer.UserProperty.class).getAddress());
    }

+    private static void assertTestAvatar(User user, WireMockRule wireMockRule) {
+        String expectedAvatarUrl = "http://localhost:%s/my-avatar.png".formatted(wireMockRule.port());


WiremockRule.baseUrl()?

src/test/java/org/jenkinsci/plugins/oic/PluginTest.java

jtnord · 2025-02-26T10:16:45Z

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

+        try {
+            String avatarUrl = determineStringField(avatarFieldExpr, idToken, userInfo);
+            if (avatarUrl != null) {
+                LOGGER.finest("Avatar url is: " + avatarUrl);


This does string concatenation and then in most cases throws the result away.
Better to use parameterized logging or a supplier.

Additionally, would it be useful to know which user this was for?

(untested)

Suggested change

LOGGER.finest("Avatar url is: " + avatarUrl);

LOGGER.finest(() -> {"Avatar for user `" + user.getId() + "` url is: " + avatarUrl });

jtnord · 2025-02-26T10:17:54Z

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

+                LOGGER.finest("No avatar URL found");
+            }
+
+        } catch (Exception e) {


is it still valid to have this in a try/catch now we are not downloading, / decrypting the picture?

jtnord · 2025-02-26T10:29:44Z

src/test/java/org/jenkinsci/plugins/oic/PluginTest.java

+        assertEquals(expectedAvatarUrl, avatarProperty.getAvatarUrl(), "Avatar url should be " + expectedAvatarUrl);
+        assertEquals("Openid Connect Avatar", avatarProperty.getDisplayName());
+        assertNull(avatarProperty.getIconFileName(), "Icon filename must be null");


should this not check the AvatarResolver also?

e.g. (untested)

Suggested change

assertEquals(expectedAvatarUrl, avatarProperty.getAvatarUrl(), "Avatar url should be " + expectedAvatarUrl);

assertEquals("Openid Connect Avatar", avatarProperty.getDisplayName());

assertNull(avatarProperty.getIconFileName(), "Icon filename must be null");

assertEquals(expectedAvatarUrl, avatarProperty.getAvatarUrl(), "Avatar url should be " + expectedAvatarUrl);

assertEquals("OpenID Connect Avatar", avatarProperty.getDisplayName());

assertNull(avatarProperty.getIconFileName(), "Icon filename must be null");

String urlViaAvatarResolver = UserAvatarResolver.resolve(user, "48x48");

assertEquals(expectedAvatarUrl, urlViaAvatarResolver, "Avatar url should be " + expectedAvatarUrl);

jtnord · 2025-02-26T10:34:37Z

@jtnord Hi again, would you be able to take a look? It's now using standard picture claim

thanks for the reminder, I had reviewed it but forgot to submit it :(

jonesbusy requested a review from a team as a code owner February 5, 2025 06:09

jonesbusy changed the title ~~Add support for avatar on user endpoint~~ Add support for avatars Feb 5, 2025

jonesbusy force-pushed the feature/avatar-support branch from 290dd59 to f5f41de Compare February 5, 2025 07:47

jtnord reviewed Feb 11, 2025

View reviewed changes

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Outdated Show resolved Hide resolved

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Outdated Show resolved Hide resolved

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java Outdated Show resolved Hide resolved

jonesbusy requested review from jtnord and daniel-beck February 12, 2025 07:59

jonesbusy force-pushed the feature/avatar-support branch 2 times, most recently from 6149b12 to e73813f Compare February 18, 2025 07:35

jonesbusy force-pushed the feature/avatar-support branch from e73813f to f8b3750 Compare February 19, 2025 05:41

Add support for avatar

91ca44a

Signed-off-by: Valentin Delaye <[email protected]>

jonesbusy force-pushed the feature/avatar-support branch from f8b3750 to 91ca44a Compare February 25, 2025 03:59

jtnord reviewed Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for avatars #523

Add support for avatars #523

jonesbusy commented Feb 5, 2025 •

edited

Loading

jonesbusy commented Feb 5, 2025

codecov bot commented Feb 5, 2025 •

edited

Loading

jtnord commented Feb 8, 2025 •

edited

Loading

jtnord commented Feb 8, 2025

jonesbusy commented Feb 9, 2025

jonesbusy commented Feb 10, 2025

jonesbusy commented Feb 12, 2025

jonesbusy commented Feb 18, 2025

jonesbusy commented Feb 19, 2025

jonesbusy commented Feb 26, 2025

jtnord Feb 12, 2025

jtnord Feb 12, 2025

jonesbusy Feb 26, 2025

jtnord Feb 26, 2025

jtnord Feb 12, 2025

jtnord Feb 17, 2025

jonesbusy Feb 26, 2025

jtnord Feb 17, 2025

jtnord Feb 26, 2025

jtnord Feb 26, 2025

jtnord Feb 26, 2025

jtnord commented Feb 26, 2025

	return "Openid Connect Avatar";
	return "OpenID Connect Avatar";

	LOGGER.finest("Avatar url is: " + avatarUrl);
	LOGGER.finest(() -> {"Avatar for user `" + user.getId() + "` url is: " + avatarUrl });

Add support for avatars #523

Are you sure you want to change the base?

Add support for avatars #523

Conversation

jonesbusy commented Feb 5, 2025 • edited Loading

Testing done

Submitter checklist

jonesbusy commented Feb 5, 2025

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

jtnord commented Feb 8, 2025 • edited Loading

jtnord commented Feb 8, 2025

jonesbusy commented Feb 9, 2025

jonesbusy commented Feb 10, 2025

jonesbusy commented Feb 12, 2025

jonesbusy commented Feb 18, 2025

jonesbusy commented Feb 19, 2025

jonesbusy commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord commented Feb 26, 2025

jonesbusy commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

jtnord commented Feb 8, 2025 •

edited

Loading