Skip to content

Feat: Pull users' Profile Picture from the OIDC claims #2704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Guibi1 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Feat: Pull users' Profile Picture from the OIDC claims #2704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72f83ba
Feat: OIDC profile picture
Guibi1 May 2, 2026
1c13bc0
Removed code to block profile picture updates
Guibi1 May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions services/proxy/pkg/command/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package command
import (
"context"
"crypto/tls"
"crypto/x509"
"fmt"
"net/http"
"os"
"os/signal"
"time"

Expand Down Expand Up @@ -276,6 +278,28 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config,
Timeout: time.Second * 10,
}

backendTLSConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: cfg.InsecureBackends, //nolint:gosec
}
if cfg.BackendHTTPSCACert != "" {
certs := x509.NewCertPool()
pemData, err := os.ReadFile(cfg.BackendHTTPSCACert)
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ LOW RISK

This block for loading CA certificates from a file is duplicated from the OIDC certificate loading logic. Refactoring this into a shared helper function would improve maintainability.

See Clone in Codacy

if err != nil {
logger.Fatal().Err(err).Msg("Failed to read backend HTTPS CA certificate")
}
if !certs.AppendCertsFromPEM(pemData) {
logger.Fatal().Msg("Failed to append backend HTTPS CA certificate")
}
backendTLSConfig.RootCAs = certs
}
backendHTTPClient := &http.Client{
Transport: &http.Transport{
TLSClientConfig: backendTLSConfig,
},
Timeout: time.Second * 10,
Comment on lines +296 to +300
}
Comment on lines +296 to +301

var authenticators []middleware.Authenticator
if cfg.EnableBasicAuth {
logger.Warn().Msg("basic auth enabled, use only for testing or development")
Expand Down Expand Up @@ -363,6 +387,11 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config,
middleware.TraceProvider(traceProvider),
middleware.UserProvider(userProvider),
middleware.UserRoleAssigner(roleAssigner),
middleware.HTTPClient(oidcHTTPClient),
middleware.BackendHTTPClient(backendHTTPClient),
middleware.OIDCIss(cfg.OIDC.Issuer),
middleware.ServiceSelector(serviceSelector),
middleware.OIDCProfilePicture(cfg.OIDCProfilePicture),
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH RISK

Compilation error: cfg.OIDCProfilePicture is not defined. According to the changes in config.go, this should be cfg.AutoProvisionClaims.ProfilePicture.

middleware.SkipUserInfo(cfg.OIDC.SkipUserInfo),
middleware.UserOIDCClaim(cfg.UserOIDCClaim),
middleware.UserCS3Claim(cfg.UserCS3Claim),
Expand Down
1 change: 1 addition & 0 deletions services/proxy/pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ type AutoProvisionClaims struct {
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"`
ProfilePicture string `yaml:"profile_picture" env:"PROXY_AUTOPROVISION_CLAIM_PROFILE_PICTURE" desc:"The name of the OIDC claim that holds the profile picture URL. When set, the profile picture will be synced on login."`
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

The environment variable name contradicts the PR description. Please use 'PROXY_OIDC_PROFILE_PICTURE_CLAIM' for consistency with the documentation.

Groups string `yaml:"groups" env:"PROXY_AUTOPROVISION_CLAIM_GROUPS" desc:"The name of the OIDC claim that holds the groups." introductionVersion:"1.0.0"`
Comment on lines 165 to 169
}

Expand Down
1 change: 1 addition & 0 deletions services/proxy/pkg/config/defaults/defaultconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func DefaultConfig() *config.Config {
Username: "preferred_username",
Email: "email",
DisplayName: "name",
ProfilePicture: "",
Groups: "groups",
},
EnableBasicAuth: false,
Expand Down
246 changes: 217 additions & 29 deletions services/proxy/pkg/middleware/account_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package middleware

import (
"bytes"
"context"
"errors"
"fmt"
"io"
"net/http"
"net/url"
"strings"
"time"

gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
Expand All @@ -14,6 +18,7 @@
"github.com/opencloud-eu/opencloud/services/proxy/pkg/router"
"github.com/opencloud-eu/opencloud/services/proxy/pkg/user/backend"
"github.com/opencloud-eu/opencloud/services/proxy/pkg/userroles"
"go-micro.dev/v4/selector"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"

Expand All @@ -27,6 +32,11 @@
"github.com/opencloud-eu/reva/v2/pkg/utils"
)

const (
graphServiceName = "eu.opencloud.web.graph"
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH RISK

The requirement 'PROXY_OIDC_PROFILE_PICTURE_DISABLE_LOCAL_CHANGES' from the PR description is not implemented. Logic to intercept or disable the graph endpoint based on this setting is missing.

maxProfilePhotoBytes = 10 << 20
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

)

// AccountResolver provides a middleware which mints a jwt and adds it to the proxied request based
// on the oidc-claims
func AccountResolver(optionSetters ...Option) func(next http.Handler) http.Handler {
Expand All @@ -46,42 +56,61 @@
)
go tenantIDCache.Start()

httpClient := options.HTTPClient
if httpClient == nil {
httpClient = &http.Client{Timeout: 10 * time.Second}
}
backendHTTPClient := options.BackendHTTPClient
if backendHTTPClient == nil {
backendHTTPClient = &http.Client{Timeout: 10 * time.Second}
}

return func(next http.Handler) http.Handler {
return &accountResolver{
next: next,
logger: logger,
tracer: tracer,
userProvider: options.UserProvider,
userOIDCClaim: options.UserOIDCClaim,
userCS3Claim: options.UserCS3Claim,
tenantOIDCClaim: options.TenantOIDCClaim,
tenantIDMappingEnabled: options.TenantIDMappingEnabled,
gatewaySelector: options.RevaGatewaySelector,
serviceAccount: options.ServiceAccount,
userRoleAssigner: options.UserRoleAssigner,
autoProvisionAccounts: options.AutoprovisionAccounts,
multiTenantEnabled: options.MultiTenantEnabled,
lastGroupSyncCache: lastGroupSyncCache,
tenantIDCache: tenantIDCache,
eventsPublisher: options.EventsPublisher,
next: next,
logger: logger,
tracer: tracer,
userProvider: options.UserProvider,
userOIDCClaim: options.UserOIDCClaim,
userCS3Claim: options.UserCS3Claim,
tenantOIDCClaim: options.TenantOIDCClaim,
tenantIDMappingEnabled: options.TenantIDMappingEnabled,
gatewaySelector: options.RevaGatewaySelector,
serviceAccount: options.ServiceAccount,
userRoleAssigner: options.UserRoleAssigner,
autoProvisionAccounts: options.AutoprovisionAccounts,
multiTenantEnabled: options.MultiTenantEnabled,
lastGroupSyncCache: lastGroupSyncCache,
tenantIDCache: tenantIDCache,
eventsPublisher: options.EventsPublisher,
profilePictureClaim: options.AutoProvisionClaims.ProfilePicture,
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH RISK

Compilation error: The field 'AutoProvisionClaims' is not defined on the 'Options' struct. The middleware should likely use the 'OIDCProfilePicture' config field directly as defined in options.go.

httpClient: httpClient,
backendHTTPClient: backendHTTPClient,
oidcIssuer: options.OIDCIss,
serviceSelector: options.ServiceSelector,
Comment on lines +82 to +90
}
}
}

type accountResolver struct {
next http.Handler
logger log.Logger
tracer trace.Tracer
userProvider backend.UserBackend
userRoleAssigner userroles.UserRoleAssigner
autoProvisionAccounts bool
multiTenantEnabled bool
tenantIDMappingEnabled bool
gatewaySelector pool.Selectable[gateway.GatewayAPIClient]
serviceAccount config.ServiceAccount
userOIDCClaim string
userCS3Claim string
tenantOIDCClaim string
next http.Handler
logger log.Logger
tracer trace.Tracer
userProvider backend.UserBackend
userRoleAssigner userroles.UserRoleAssigner
autoProvisionAccounts bool
multiTenantEnabled bool
tenantIDMappingEnabled bool
gatewaySelector pool.Selectable[gateway.GatewayAPIClient]
serviceSelector selector.Selector
serviceAccount config.ServiceAccount
userOIDCClaim string
userCS3Claim string
tenantOIDCClaim string
profilePictureClaim string
oidcIssuer string
httpClient *http.Client
backendHTTPClient *http.Client
// lastGroupSyncCache is used to keep track of when the last sync of group
// memberships was done for a specific user. This is used to trigger a sync
// with every single request.
Expand Down Expand Up @@ -126,8 +155,161 @@
return value, fmt.Errorf("claim path '%s' not set or empty", path)
}

func (m accountResolver) syncProfilePicture(ctx context.Context, req *http.Request, user *cs3user.User, token string, claims map[string]any) error {
if user == nil {
return errors.New("missing user for profile photo sync")
}
if token == "" {
return errors.New("missing user token for profile photo sync")
}

pictureURL, err := readStringClaim(m.profilePictureClaim, claims)
if err != nil {
m.logger.Debug().Err(err).Str("claim", m.profilePictureClaim).Msg("profile picture claim missing")
return nil
}
if pictureURL == "" {
return nil
}

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")
}
Comment on lines +175 to +184

authHeader := ""
if req != nil {
authHeader = req.Header.Get("Authorization")
}

photo, err := m.fetchProfilePicture(ctx, parsedURL, authHeader)
Comment on lines +175 to +191
if err != nil {
return err
}

return m.updateGraphProfilePhoto(ctx, token, photo)
}

func (m accountResolver) fetchProfilePicture(ctx context.Context, pictureURL *url.URL, authHeader string) ([]byte, error) {
client := m.httpClient
if client == nil {
client = http.DefaultClient
}

request, err := http.NewRequestWithContext(ctx, http.MethodGet, pictureURL.String(), nil)
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

if err != nil {
return nil, err
}
request.Header.Set("Accept", "image/*")
if authHeader != "" && m.shouldAttachOIDCToken(pictureURL) {
request.Header.Set("Authorization", authHeader)
}

resp, err := client.Do(request)
if err != nil {
return nil, err
}
defer resp.Body.Close()

if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
return nil, fmt.Errorf("profile picture request returned %s", resp.Status)
}

limited := io.LimitReader(resp.Body, int64(maxProfilePhotoBytes)+1)
data, err := io.ReadAll(limited)
if err != nil {
return nil, err
}
if len(data) == 0 {
return nil, errors.New("profile picture response was empty")
}
if len(data) > maxProfilePhotoBytes {
return nil, fmt.Errorf("profile picture exceeds %d bytes", maxProfilePhotoBytes)
}
contentType := http.DetectContentType(data)
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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.

if !strings.HasPrefix(contentType, "image/") {
return nil, fmt.Errorf("unsupported profile picture content type: %s", contentType)
}

return data, nil
}

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)
Comment on lines +243 to +251
Comment on lines +247 to +251
}

func (m accountResolver) updateGraphProfilePhoto(ctx context.Context, token string, photo []byte) error {
if token == "" {
return errors.New("missing access token for graph profile photo update")
}
baseURL, err := m.graphBaseURL()
if err != nil {
return err
}

endpoint := baseURL + "/v1.0/me/photo/$value"
request, err := http.NewRequestWithContext(ctx, http.MethodPut, endpoint, bytes.NewReader(photo))
if err != nil {
return err
}
request.Header.Set(revactx.TokenHeader, token)
request.Header.Set("Content-Type", http.DetectContentType(photo))

client := m.backendHTTPClient
if client == nil {
client = http.DefaultClient
}
resp, err := client.Do(request)
if err != nil {
return err
}
defer resp.Body.Close()

if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
body, _ := io.ReadAll(io.LimitReader(resp.Body, 2048))
return fmt.Errorf("graph profile photo update failed: %s (%s)", resp.Status, strings.TrimSpace(string(body)))
}

return nil
}

func (m accountResolver) graphBaseURL() (string, error) {
if m.serviceSelector == nil {
return "", errors.New("service selector not configured")
}
selectNext, err := m.serviceSelector.Select(graphServiceName)
if err != nil {
return "", err
}
node, err := selectNext()
if err != nil {
return "", err
}
scheme := node.Metadata["protocol"]
if node.Metadata["use_tls"] == "true" {
scheme = "https"
}
if scheme == "" {
scheme = "http"
}
return fmt.Sprintf("%s://%s/graph", scheme, node.Address), nil
}

// TODO do not use the context to store values: https://medium.com/@cep21/how-to-correctly-use-context-context-in-go-1-7-8f2c0fafdf39
func (m accountResolver) ServeHTTP(w http.ResponseWriter, req *http.Request) {

Check failure on line 312 in services/proxy/pkg/middleware/account_resolver.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 66 to the 15 allowed. 


See more on https://sonarcloud.io/project/issues?id=opencloud-eu_opencloud&issues=AZ3qfNe5NwJKsMcWXSBA&open=AZ3qfNe5NwJKsMcWXSBA&pullRequest=2704
ctx, span := m.tracer.Start(req.Context(), fmt.Sprintf("%s %s", req.Method, req.URL.Path), trace.WithSpanKind(trace.SpanKindServer))
claims := oidc.FromContext(ctx)
user, ok := revactx.ContextGetUser(ctx)
Expand Down Expand Up @@ -219,6 +401,12 @@
}
}

if m.profilePictureClaim != "" && oidc.NewSessionFlagFromContext(ctx) {
if err := m.syncProfilePicture(ctx, req, user, token, claims); err != nil {
Copy link
Copy Markdown

@codacy-production codacy-production Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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).

m.logger.Warn().Err(err).Str("userid", user.GetId().GetOpaqueId()).Msg("Failed to sync profile picture from OIDC claim")
}
}
Comment on lines +404 to +408
Comment on lines +404 to +408

// resolve the user's roles
user, err = m.userRoleAssigner.UpdateUserRoleAssignment(ctx, user, claims)
if err != nil {
Expand Down
Loading