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. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 37 |
| Duplication | -1 |
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
|
|
||
| type OIDCProfilePicture struct { | ||
| OIDCIssuer string `yaml:"oidc_issuer" env:"OC_URL;OC_OIDC_ISSUER;GRAPH_OIDC_ISSUER" desc:"URL of the OIDC issuer used to derive the default profile-picture URL allowlist when no explicit allowlist is configured." introductionVersion:"6.3.0"` | ||
| URLAllowlist []string `yaml:"url_allowlist" env:"GRAPH_OIDC_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. If empty, only the OIDC issuer host is allowed by default." introductionVersion:"6.3.0"` |
There was a problem hiding this comment.
It should be clear that '*' is dangerious - and it should only be used if the IdP is trusted to provide only valid urls
There was a problem hiding this comment.
not only valid: but trusted urls - otherwise it's an SSRF attack vector
| } | ||
|
|
||
| type OIDCProfilePicture struct { | ||
| OIDCIssuer string `yaml:"oidc_issuer" env:"OC_URL;OC_OIDC_ISSUER;GRAPH_OIDC_ISSUER" desc:"URL of the OIDC issuer used to derive the default profile-picture URL allowlist when no explicit allowlist is configured." introductionVersion:"6.3.0"` |
There was a problem hiding this comment.
It feels a bit weird to have an explicit setting just to make it the default for another setting - not sure...
| displayNameAttr = "displayName" | ||
| HeaderPurge = "Purge" | ||
| displayNameAttr = "displayName" | ||
| maxProfilePhotoBytes = 10 << 20 |
There was a problem hiding this comment.
const maxProfilePhotoBytes = 10 << 20 // 10 MiB
maybe?
| } | ||
| return hostMatcher.Match(strings.ToLower(target.Host)) | ||
| } | ||
|
|
There was a problem hiding this comment.
My feeling is that syncProfilePictureFromURL should be a function on the userProfilePhotoService and the rest of this block also belongs there. Yes, it needs a bit of wiring to pass configs down - but I feel it's worth it to have a clean separation of concerns
| o.ProfilePictureHTTPClient = val | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nitpick but I think this belongs further down in the file
| traceProvider: options.TraceProvider, | ||
| valueService: options.ValueService, | ||
| natskv: options.NatsKeyValue, | ||
| profilePictureHTTPClient: options.ProfilePictureHTTPClient, |
There was a problem hiding this comment.
Why not keep this as a member in the photo service?
| Username: "preferred_username", | ||
| Email: "email", | ||
| DisplayName: "name", | ||
| ProfilePicture: "", |
There was a problem hiding this comment.
If I understand the spec correctly, the default is picture (some IdPs seem to use avatar additionally?)
Let's use "picture" here - if someone doesnt want to use it although their IdP provides it, then they can still opt out by setting the value to empty explicitly?
| Timestamp *types.Timestamp | ||
| } | ||
|
|
||
| // Unmarshal to fulfill umarshaller interface |
There was a problem hiding this comment.
Unfortunately we can't do this - it would be lost on the next reva update.
We need to upstream it to the reva repo first. Maybe there's also the right place to have a discussion whether we really want to reuse this event or introduce a new one (or new ones: for picture updated e.g.)
| } | ||
| pictureURL, err := readStringClaim(m.autoProvisionClaims.ProfilePicture, claims) | ||
| if err != nil { | ||
| m.logger.Debug().Err(err).Str("claim", m.autoProvisionClaims.ProfilePicture).Msg("profile picture claim missing") |
There was a problem hiding this comment.
Maybe the wording should be a more neutral?
"missing" sounds like a misconfiguration, maybe "no profile picture claim found" would be more neutral.
And maaaybe we should add some user information to the log? like this it's really hard to debug or learn anything useful from the log message
| middleware.UserOIDCClaim(cfg.UserOIDCClaim), | ||
| middleware.UserCS3Claim(cfg.UserCS3Claim), | ||
| middleware.TenantOIDCClaim(cfg.TenantOIDCClaim), | ||
| middleware.AutoProvisionClaims(cfg.AutoProvisionClaims), |
There was a problem hiding this comment.
Why do you have to add it here?
It was already added in another place (conditionally)
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: