Repository navigation
Use the standard OIDC picture claim for user avatars - #782
itsvshreyas wants to merge 27 commits into
Conversation
… URLs - Introduced and properties in OIC Security Realm. - Updated configuration files and documentation to reflect new avatar settings. - Implemented tests for avatar URL handling, including Microsoft Graph integration. Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
…plTest, OicSecurityRealmTest, and PluginTest Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
|
Hi @michael-doubez, This PR is completely ready for review. |
|
Hi @michael-doubez , Could you please review it? It will close the issue #709. |
|
Hi @michael-doubez , Could you please review this PR? It will close the issue #709. |
|
@itsvshreyas hello. I don t have access to a computer right now and my smartphone is not a good place for a code review I should be back to civilisation next week. |
Hi @michael-doubez , Thank you for your time! |
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
|
Hi @michael-doubez, As an update, I have also added commits to cover the remaining three lines for code coverage. Everything is now complete and ready for review. |
|
Hi @michael-doubez , I just wanted to check if you are back and would be possible to review this today? Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #782 +/- ##
============================================
+ Coverage 76.06% 76.46% +0.40%
- Complexity 325 335 +10
============================================
Files 33 33
Lines 1291 1313 +22
Branches 178 181 +3
============================================
+ Hits 982 1004 +22
Misses 227 227
Partials 82 82 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @olamy / @michael-doubez , Code Coverage tests have been passed. Could you please review and approve if everything is okay? Thanks! |
Hi @olamy / @michael-doubez / @timja / @jtnord , Code Coverage has been completed and all have passed. Could you please review and approve if everything is good? Thanks! |
|
Not a maintainer, if you want the Entra avatars to work you can take a look at this code: |
|
cc @mjmbischoff too |
Hi @timja, Thank you for that PR reference. I got it working in this PR for oic-auth. I made Microsoft Graph optional and it will be used only if required. |
The PR description doesn't seem to reflect that:
I would expect it to work out of the box without additional configuration or checkboxes. |
|
Noting: #523 (comment) No idea about Entra avatar. But if the picture claim URL require authentication, it will not work with current implementation. Curious, why not using azure-ad-plugin? A checkbox look silly I'm not a maintainer of this plugin |
|
Please avoid provider specific workarounds in this plugin, the OIDC spec has a spec so no specifics for any provider should not be needed. |
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
|
Thank you for all your feedback. I have got a working version which does not require any field selection. I will update the PR shortly (code/PR title and description) to reflect those changes. That will resolve broken image issue. |
Hi @jtnord, Thanks for pointing this out. I agree that provider-specific Microsoft Graph handling does not belong in the generic OIDC plugin, and that picture is already a standard OpenID Connect claim. The original change was intended to address providers that expose protected image URLs, but that should not be solved with Entra-specific logic here. I have simplified the implementation to use the standard picture claim directly and remove the provider-specific Graph handling and claim configuration. This PR will be updated shortly. |
…pertyAdd unit tests for Microsoft Graph avatar fetching and OIC avatar property Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
… handling Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
…hing Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
…ityRealm Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Hi @jtnord , Agreed. We initially tried relying only on the standard OIDC picture claim, but with Microsoft Entra ID the claim may be absent or may contain a Microsoft Graph photo URL that the browser cannot access without a bearer token. To support Entra deployments without making Microsoft-specific behavior the default, we added useMicrosoftGraphForAvatar, which is disabled by default. When enabled, the plugin fetches the photo server-side from Microsoft Graph using the access token; otherwise, it uses only the standard OIDC picture claim. Please review the latest changes and approve the PR if everything looks good now. |
So I am happy to support The problem is not this one implementation, it is that this opens the way for every implementation that does something to have provider specific code, and then the plugin config becomes a spaghetti mess of configuration options that mean nothing to most people (as well as the maintenance burden) |
| } | ||
|
|
||
| public void doImage(StaplerResponse2 response) throws IOException { | ||
| AvatarData data = avatarImage == null ? null : parseDataUrl(avatarImage.url); |
There was a problem hiding this comment.
just wondering how large these can be (and if they actually need to be in memory)?
5MB looking at MAX_SIZE. this is information that would be loaded/persisted in the User object and on a busy system with 100s of active users that could be quite large.
Could we save the image to disk in the users folder, and then use the optimized serveFile API?
There was a problem hiding this comment.
Hi @jtnord , I’ve updated the implementation so that data URL avatars are validated against the existing 5 MB limit, written to the user’s folder as oic-avatar, and represented in OicAvatarProperty only by their filename and content type rather than by the image data itself. Requests now stream the image from disk using Stapler’s optimized serveFile API, while regular external avatar URLs continue to work as before. I also added test coverage for persisting the image and serving it through serveFile; the focused avatar tests and full test suite pass.
Hi @jtnord , The solution in this PR keeps the standard OIDC behavior as the default. The plugin uses the standard picture claim when it contains a normal URL or a supported data: URL. Data URLs are stored and served by Jenkins, so the browser does not need direct access to the image. If the claim is absent or unusable, the plugin leaves the OIC avatar unset and Jenkins can use its normal default avatar. In our SAML setup, that default CloudBees avatar loads correctly; previously, the invalid avatar URL returned by the identity provider resulted in a broken image instead. In our Entra-based setup, the value returned for the avatar was either missing or a Microsoft Graph photo URL. Although that URL looks valid, the browser cannot load it because the request requires a bearer token. The opt-in useMicrosoftGraphForAvatar setting addresses that specific deployment: Jenkins fetches the photo server-side with the access token and stores it as a data: URL. It is disabled by default, so it does not add Microsoft-specific behavior for normal OIDC providers or alter existing configurations. This gives us a working avatar for Entra deployments while preserving the generic OIDC behavior and the normal Jenkins fallback for everyone else. The latest changes also validate supported image types and sizes before serving images locally. The opt-in path requires the appropriate Microsoft Graph permission, such as delegated User.Read. |
…nd retrieval Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
… test Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
…ptions more clearly Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
…pertyTest Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
|
Hi @jtnord , This is good for review now. All changes based on your feedback has been incorporated. Could you please review now? Thanks! |
If you are using CloudBees CI, I would recommend that you raise a support ticket with them about the broken behaviour you are seeing. |
Hi @jtnord , We had already brought this issue to attention and were informed that the root cause was associated with the plugin. Following that discussion, we decided to contribute a solution through this repository. We also observed similar reports from other users (e.g., #709), indicating that this is a recurring scenario rather than an isolated case. We believe having this solution in place would help others facing the same challenge. We would appreciate your review and approval of this proposed change. |
|
Hi @jtnord , Is there anything else that needs to be done as part of this PR? Happy to make if any further changes required. Thanks! |
Its still containing MS specifics and as I have said these should not be in this plugin. I have filed #793 that should fix the actual broken avatar (just set the As the Picture URL returned by O365 is the MS graph URL - all that is needed is an implementation that fetches the URL using the access token (and this has zero MS sepecific). NB I would start on the implementaion that fectes without authentication so that it is slightly easier to ensure that no security issues are introduced. |
- Added OicAvatarFetcher class to download images from OIDC providers using access tokens. - Updated OicAvatarProperty to store and serve avatars from Jenkins instead of relying on external URLs. - Refactored OicSecurityRealm to support configuration for serving avatars from Jenkins. - Removed MicrosoftGraphAvatarFetcher and related configurations, consolidating avatar fetching logic. - Updated configuration files and UI to reflect changes in avatar handling. - Added tests for OicAvatarFetcher and updated existing tests for OicAvatarProperty and OicSecurityRealm. Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
…sts for missing content type and avatar file absence Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Signed-off-by: Venkata Shreyas Kabekkodu <venkatashreyas.kabekkodu@fmr.com>
Hi @jtnord , Thanks, I've reworked this so there's nothing Microsoft-specific left. Could you please re-review the changes? Please let me know if further changes required. Thanks! |
|
Hi @jtnord , All mentioned changes have been completed. Could you please re-review this? Thanks! |
1 similar comment
|
Hi @jtnord , All mentioned changes have been completed. Could you please re-review this? Thanks! |
But you have not addressed that this is an inflexible boolean value, and ignored the referenced pull requests? FWIW, in CloudBees CI the avatars would still not be displayed in controllers when using single sign on. |
Use the standard OIDC
pictureclaim for user avatarspictureclaim as the default avatar claim for OIDC providers.avatarFieldNamesupport for providers exposing the avatar URL through another claim.User.Readscope automatically for Microsoft Entra issuers when it is not already configured./oic-avatar/imageendpoint.Fixes: #709
Testing done
AuthorizationandAcceptheaders.User.Readscope handling.pictureclaim.MicrosoftGraphAvatarFetcherTest: 9 tests, 0 failuresOicAvatarPropertyTest: 4 tests, 0 failuresOicSecurityRealmTest: 15 tests, 0 failuresPluginTest: 40 tests, 0 failures, 2 skippedMicrosoftGraphAvatarFetcher:mvn spotless:applycompleted successfully.Submitter checklist