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.
| if m.profilePictureClaim != "" && oidc.NewSessionFlagFromContext(ctx) { | ||
| if err := m.syncProfilePicture(ctx, req, user, token, claims); err != nil { | ||
| m.logger.Warn().Err(err).Str("userid", user.GetId().GetOpaqueId()).Msg("Failed to sync profile picture from OIDC claim") | ||
| } | ||
| } |
|
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. |
| parsedURL, err := url.Parse(pictureURL) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid profile picture URL: %w", err) | ||
| } | ||
| if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { | ||
| return fmt.Errorf("unsupported profile picture URL scheme: %s", parsedURL.Scheme) | ||
| } | ||
| if parsedURL.Host == "" { | ||
| return fmt.Errorf("profile picture URL is missing a host") | ||
| } |
| issuerURL, err := url.Parse(m.oidcIssuer) | ||
| if err != nil || issuerURL.Host == "" { | ||
| return false | ||
| } | ||
| return strings.EqualFold(issuerURL.Host, pictureURL.Host) |
| backendHTTPClient := &http.Client{ | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: backendTLSConfig, | ||
| }, | ||
| Timeout: time.Second * 10, |
| if m.profilePictureClaim != "" && oidc.NewSessionFlagFromContext(ctx) { | ||
| if err := m.syncProfilePicture(ctx, req, user, token, claims); err != nil { | ||
| m.logger.Warn().Err(err).Str("userid", user.GetId().GetOpaqueId()).Msg("Failed to sync profile picture from OIDC claim") | ||
| } | ||
| } |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 53 |
| Duplication | 0 |
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.
|
|
||
| const ( | ||
| graphServiceName = "eu.opencloud.web.graph" | ||
| maxProfilePhotoBytes = 10 << 20 |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The 10MB limit for profile photos is excessive for a synchronous automated sync process. Consider reducing this to 1-2MB to prevent high memory usage and mitigate resource exhaustion risks during peak login times.
| } | ||
|
|
||
| if m.profilePictureClaim != "" && oidc.NewSessionFlagFromContext(ctx) { | ||
| if err := m.syncProfilePicture(ctx, req, user, token, claims); err != nil { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Syncing the profile picture synchronously blocks the request flow and increases latency for the initial session request. Consider running this in a background goroutine using a detached context (e.g., context.WithoutCancel).
| if len(data) > maxProfilePhotoBytes { | ||
| return nil, fmt.Errorf("profile picture exceeds %d bytes", maxProfilePhotoBytes) | ||
| } | ||
| contentType := http.DetectContentType(data) |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The content-type is detected twice for the same image data. Optimize this by returning the detected content-type from fetchProfilePicture and passing it to updateGraphProfilePhoto.
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
Description
This PR adds two settings:
PROXY_OIDC_PROFILE_PICTURE_CLAIM: Sets which oidc claim to use as the profile picture.PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGES: Stops the user from updating their profile pictures by disabling the graph endpoint.Related Issue
Motivation and Context
See linked issue.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: