Feat: Pull users' Profile Picture from the OIDC claims#2704
Feat: Pull users' Profile Picture from the OIDC claims#2704Guibi1 wants to merge 1 commit intoopencloud-eu:mainfrom
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.
| 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") | ||
| } | ||
|
|
||
| authHeader := "" | ||
| if req != nil { | ||
| authHeader = req.Header.Get("Authorization") | ||
| } | ||
|
|
||
| photo, err := m.fetchProfilePicture(ctx, parsedURL, authHeader) |
| func (m accountResolver) shouldAttachOIDCToken(pictureURL *url.URL) bool { | ||
| if m.oidcIssuer == "" || pictureURL == nil { | ||
| return false | ||
| } | ||
| issuerURL, err := url.Parse(m.oidcIssuer) | ||
| if err != nil || issuerURL.Host == "" { | ||
| return false | ||
| } | ||
| return strings.EqualFold(issuerURL.Host, pictureURL.Host) |
| if m.disableLocalProfilePictureChange && claims != nil && m.isProfilePhotoMutation(req) { | ||
| m.logger.Debug().Str("path", req.URL.Path).Msg("profile photo updates disabled for OIDC users") | ||
| w.WriteHeader(http.StatusForbidden) | ||
| return | ||
| } |
| 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") | ||
| } | ||
| } |
| Claim string `yaml:"claim" env:"PROXY_OIDC_PROFILE_PICTURE_CLAIM" desc:"The name of the OIDC claim that holds a URL to the user's profile picture. When set, the profile picture will be synced on login."` | ||
| DisableLocalChanges bool `yaml:"disable_local_changes" env:"PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGES" desc:"When set, users authenticated via OIDC cannot change their profile picture locally (PUT/PATCH/DELETE on /graph/v1.0/me/photo/$value)."` |
| backendHTTPClient := &http.Client{ | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: backendTLSConfig, | ||
| }, | ||
| Timeout: time.Second * 10, | ||
| } |
|
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... |
Description
This PR adds two settings:
Related Issue
Motivation and Context
See linked issue.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: