Feat: Pull users' Profile Picture from the OIDC claims#2704
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support in the proxy service to (a) sync a user’s profile photo from a configurable OIDC claim on login, and (b) optionally forbid local profile-photo updates via the Graph “me/photo/$value” endpoint for OIDC-authenticated requests.
Changes:
- Add
PROXY_OIDC_PROFILE_PICTURE_CLAIMto fetch a profile-photo URL from OIDC claims and sync it on new sessions. - Add
PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGESto block PUT/PATCH/DELETE to/graph/v1.0/me/photo/$valuefor OIDC-authenticated requests. - Introduce proxy wiring for internal Graph calls (service discovery + separate backend HTTP client).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| services/proxy/pkg/middleware/options.go | Extends middleware options with backend HTTP client, internal service selector, and OIDC profile-picture config. |
| services/proxy/pkg/middleware/account_resolver.go | Implements profile-picture fetch + Graph update on new session; optionally blocks local photo mutations. |
| services/proxy/pkg/config/defaults/defaultconfig.go | Adds default values for the new oidc_profile_picture config section. |
| services/proxy/pkg/config/config.go | Introduces OIDCProfilePicture config and env vars for claim + local-change disable. |
| services/proxy/pkg/command/server.go | Wires new config into middleware and creates a dedicated backend HTTP client for internal calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
First of all: Thanks for the effort, @Guibi1! I think this might entangle two separate concerns too tightly:
I have a feeling the proxy is not the right place to handle the second concern. To me the route handling is crossing service domains in an unfortunate way. When moving this responsibility to the graph service, this leaves us with the question of how to update the profile picture from the proxy (handling it at all there seems to be the right call as the proxy deals with all claims anyway), as the Graph HTTP Api now rejects updates. Two possibilities from my view:
It might be easier to discuss separate concerns in separate PRs - unfortunately they are heavily influencing each other... |
|
Agreed! For simplicity (and because i really want this merged), I straight up removed the second concern. |
|
|
afaik there is none - personally I don't think it's too bad adding it, we're not coming up with a new pattern in this code base. But ultimately not up for me to decide. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 critical |
🟢 Metrics 22 complexity · -52 duplication
Metric Results Complexity 22 Duplication -52
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR cannot be merged in its current state due to high-severity logic and security issues.
Critical Findings
- Compilation Errors: The code references
options.AutoProvisionClaimsandcfg.OIDCProfilePicture, neither of which exists in the provided configuration or middleware structs. - Security Risk: The
fetchProfilePicturefunction lacks host validation, exposing the system to SSRF (Server-Side Request Forgery) by allowing requests to arbitrary URLs provided in OIDC claims. - Requirement Gaps: The setting
PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGESpromised in the description is not implemented in the code. - Performance Concerns: Synchronizing profile pictures is handled synchronously within the middleware, which will block the login flow and increase latency for users during their first request.
About this PR
- The account resolver middleware has high complexity and significant new logic for fetching and updating profile pictures, yet no unit tests or integration tests have been provided. This is particularly concerning given the identified logic errors.
- There is a significant naming mismatch between the PR description ('PROXY_OIDC_PROFILE_PICTURE_CLAIM') and the code ('PROXY_AUTOPROVISION_CLAIM_PROFILE_PICTURE'). Furthermore, the
OIDCProfilePicturetype used inoptions.goandserver.goappears to be undefined or incorrectly replaced by a string field in the configuration diff.
Test suggestions
- Missing: Successful profile picture sync on new session with valid claim and URL
- Missing: Skip profile picture sync if the session is not a new OIDC session
- Missing: Reject profile picture sync if the URL scheme is not http or https
- Missing: Reject profile picture if the response size exceeds 10MB limit
- Missing: Verify Authorization header is only attached to image fetch requests if the host matches the OIDC issuer
- Missing: Verify Graph API update request includes the correct CS3 token and content type
- Missing: Verify local changes are disabled when PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGES is true
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing: Successful profile picture sync on new session with valid claim and URL
2. Missing: Skip profile picture sync if the session is not a new OIDC session
3. Missing: Reject profile picture sync if the URL scheme is not http or https
4. Missing: Reject profile picture if the response size exceeds 10MB limit
5. Missing: Verify Authorization header is only attached to image fetch requests if the host matches the OIDC issuer
6. Missing: Verify Graph API update request includes the correct CS3 token and content type
7. Missing: Verify local changes are disabled when PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGES is true
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| client = http.DefaultClient | ||
| } | ||
|
|
||
| request, err := http.NewRequestWithContext(ctx, http.MethodGet, pictureURL.String(), nil) |
There was a problem hiding this comment.
🔴 HIGH RISK
Fetching the profile picture from an arbitrary URL provided in OIDC claims poses a SSRF risk. You should validate that the URL's host matches the OIDC issuer or an allowed list of trusted domains to prevent internal network scanning.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces OIDC-based profile picture synchronization and the ability to disable local profile updates. While the implementation aligns with the stated goals via new configuration flags, there is a total lack of automated test coverage for these features.
Codacy analysis indicates the PR is technically up to standards regarding code style; however, the account_resolver.go and options.go files have seen a large increase in complexity (101 and 61 respectively) without any new tests to mitigate regression risk. It is recommended to address the missing test scenarios before merging.
About this PR
- This PR lacks unit and acceptance tests for the core logic. Specifically, the mapping of the OIDC claim to the user profile and the logic to block graph API updates remain unverified. Automated tests are required to ensure the OIDC claim parsing handles various formats (URL vs. binary) correctly.
- Multiple files (account_resolver.go, options.go, server.go, defaultconfig.go) have undergone significant complexity increases without being covered by tests. This creates a high risk of long-term maintenance issues and hidden bugs in the proxy middleware.
Test suggestions
- Verify that the profile picture is correctly mapped from the OIDC claim specified in PROXY_OIDC_PROFILE_PICTURE_CLAIM.
- Verify that attempts to update the profile picture via the graph endpoint return an error or are blocked when PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGES is true.
- Unit tests for new logic in services/proxy/pkg/middleware/account_resolver.go (Complexity: 101, Coverage: 0%).
- Unit tests for new logic in services/proxy/pkg/middleware/options.go (Complexity: 61, Coverage: 0%).
- Unit tests for new logic in services/proxy/pkg/command/server.go (Complexity: 28, Coverage: 0%).
- Unit tests for new logic in services/proxy/pkg/config/defaults/defaultconfig.go (Complexity: 27, Coverage: 0%).
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that the profile picture is correctly mapped from the OIDC claim specified in PROXY_OIDC_PROFILE_PICTURE_CLAIM.
2. Verify that attempts to update the profile picture via the graph endpoint return an error or are blocked when PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGES is true.
3. Unit tests for new logic in services/proxy/pkg/middleware/account_resolver.go (Complexity: 101, Coverage: 0%).
4. Unit tests for new logic in services/proxy/pkg/middleware/options.go (Complexity: 61, Coverage: 0%).
5. Unit tests for new logic in services/proxy/pkg/command/server.go (Complexity: 28, Coverage: 0%).
6. Unit tests for new logic in services/proxy/pkg/config/defaults/defaultconfig.go (Complexity: 27, Coverage: 0%).
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
|
I've been thinking about this for a bit and came to a few conclusions: First of all: This feature is more demanding to get right than it might seem at first sight. Sorry you have to go through this in your first contribution :) Token forwardingI don't think we should forward the token to the photo source. In OIDC every token has an audience and the token we receive in the proxy is for OpenCloud - not for the image source, that's conceptually a weird thing to forward it and a security nightmare. ProfilePicture claimhttps://openid.net/specs/openid-connect-core-1_0.html#StandardClaims
Graph communicationI think the way you're setting up an http client for talking to the graph service is much too complicated. I still see two approaches:
You might have guessed: I strongly prefer the second approach. Http Client configurationIf you agree / the team agrees with me that graph should handle it through an event, we can get rid of the http client setup in the proxy - but the image fetcher needs the same kind of config for insecure mode etc. Not sure we have a useful pattern/example to copy. |
|
Thanks for all that, i'm really not familiar enough with the codebase to have thought about it that way. |
|
Maybe allow a glob pattern like |
dschmidt
left a comment
dschmidt
left a comment
There was a problem hiding this comment.
Did you forget to commit the actual photo service? I don't see it.
Also I don't see how you instantiate it and if you always just use the default http client.
We probably need to setup a configurable client like in the proxy for talking to the IdP:
oidcHTTPClient := &http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: cfg.OIDC.Insecure, //nolint:gosec
},
DisableKeepAlives: true,
},
Timeout: time.Second * 10,
}Anyhow I really like where this is going! Good job
Guibi1
force-pushed
the
oidc-profile-picture
branch
from
53fb6fb to
b5074d8
Compare
dschmidt
left a comment
dschmidt
left a comment
There was a problem hiding this comment.
Great, I'm pretty happy with this now :)
unfortunately it's ultimately not up to me to decide
| e := UserProfilePictureUpdated{} | ||
| err := json.Unmarshal(v, &e) | ||
| return e, err | ||
| } |
There was a problem hiding this comment.
minor nitpick: one event has User prefix, one has not - feels a bit inconsistent
There was a problem hiding this comment.
Looking at the changelog folder, it seems we're not doing changelog items there anymore - but I'm not sure about that
There was a problem hiding this comment.
that's correct. Changelog is being built from PR titles (linking to the PRs) by the ready-release-go. So having a meaningful PR title and description is all you need. This changelog item can be deleted.
rhafer
left a comment
rhafer
left a comment
There was a problem hiding this comment.
@Guibi1 Thanks a lot for you effort on this pull request.
I haven't looked through all of the code yet. Especially I still need to get through the changes in the proxy. But in general I like it. I've added some comments and questions on the parts that I already reviewed.
| Application Application `yaml:"application"` | ||
| Spaces Spaces `yaml:"spaces"` | ||
| Identity Identity `yaml:"identity"` | ||
| ProfilePictureURLAllowlist []string `yaml:"profile_picture_url_allowlist" env:"GRAPH_PROFILE_PICTURE_URL_ALLOWLIST" desc:"A comma separated allowlist of URL patterns accepted for profile-picture sync events. Patterns can be full URLs with glob support in the host (for example 'https://*.example.com') or '*' to allow all URLs (dangerous: only use if the IdP is fully trusted to provide safe URLs, otherwise this is an SSRF attack vector). If empty, the OpenCloud URL host is allowed by default." introductionVersion:"6.3.0"` |
There was a problem hiding this comment.
introductionVersion needs to be %%NEXT%% for all newly introduced config settings. It will be replaced with the correct version during the release process.
| oidcIssuer := "" | ||
| if options.Config.Commons != nil { | ||
| oidcIssuer = options.Config.Commons.OpenCloudURL | ||
| } |
There was a problem hiding this comment.
The variable name oidcIssuer is misleading here when you're defaulting the URL of the OpenCloud instance. oidcIssuer usually refers to the IDP.
| //nolint:gosec // We need the ability to run with "insecure" (dev/testing) | ||
| TLSClientConfig: &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| InsecureSkipVerify: options.Config.Events.TLSInsecure, |
There was a problem hiding this comment.
I am pretty sure you're using the wrong source for the Insecure flag here. options.Config.Events.TLSInsecure is only meant for the event listener and nothing else. You might need to introduce a separate insecure flag for the picture download client.
| e := UserProfilePictureUpdated{} | ||
| err := json.Unmarshal(v, &e) | ||
| return e, err | ||
| } |
| return path.Clean(parsedPattern.Path) == path.Clean(target.Path) | ||
| } | ||
|
|
||
| hostMatcher, err := glob.Compile(strings.ToLower(pattern)) |
There was a problem hiding this comment.
What is this good for? We only get here if url.Parse returns an error, or if the host part is empty.
I guess this is for the pattern == * case? If yes, I'd suggest to make that explicit. And do an if pattern == * with an early return right before trying to parse the pattern as an URL.
| prefix := strings.TrimSuffix(parsedPattern.Path, "*") | ||
| return strings.HasPrefix(target.Path, prefix) | ||
| } | ||
| return path.Clean(parsedPattern.Path) == path.Clean(target.Path) |
There was a problem hiding this comment.
Reading the code and documentation in config.go it is a bit unclear to me what kind of patterns you do want to support. E.g. the code seems to accept patterns with out a Scheme. The annotation in config.go only refers to globs in hostname part. The code also accepts a trailiing * on the path ...
| l.Error().Err(err).Str("userid", ev.Executant.OpaqueId).Msg("Error updating last sign in date") | ||
| } | ||
| case ocevents.ProfilePictureSyncRequested: | ||
| if err := g.userProfilePhotoService.SyncPhotoFromURL(ctx, ev.Executant.GetOpaqueId(), ev.PictureURL); err != nil { |
There was a problem hiding this comment.
Somehow I think we should trigger the photo download asynchronously in a go routine and not block the event loop here.
If for some reason the download + upload the metadata storage takes longer this can have ill effects on the event consumer.
I know, that the UploadLastSignInDate() doesn't do it correct either, but that shouldn't stop us from making it better here.
| Username string `yaml:"username" env:"PROXY_AUTOPROVISION_CLAIM_USERNAME" desc:"The name of the OIDC claim that holds the username." introductionVersion:"1.0.0"` | ||
| Email string `yaml:"email" env:"PROXY_AUTOPROVISION_CLAIM_EMAIL" desc:"The name of the OIDC claim that holds the email." introductionVersion:"1.0.0"` | ||
| DisplayName string `yaml:"display_name" env:"PROXY_AUTOPROVISION_CLAIM_DISPLAYNAME" desc:"The name of the OIDC claim that holds the display name." introductionVersion:"1.0.0"` | ||
| Picture string `yaml:"picture" env:"PROXY_AUTOPROVISION_CLAIM_PICTURE" desc:"The name of the OIDC claim that holds the profile picture URL. Defaults to 'picture' (the standard OIDC claim). Set to an empty string to disable profile picture sync." introductionVersion:"6.3.0"` |
There was a problem hiding this comment.
same as above "introductionVersion" should be %%NEXT%%
Also I am a bit unhappy with the PROXY_AUTOPROVISION_CLAIM_PICTURE name. As your feature is somewhat orthogonal with the rest of the PROXY_AUTOPROVISION_* settings which all are focussed around the creation and udpate of the user accounts and are only relevant when PROXY_AUTOPROVISION_ACCOUNTS is true. The new value is different. Should it even live in the AutoProvisionClaims struct?
Description
This PR adds one setting:
Related Issue
Motivation and Context
See linked issue.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: