-
Notifications
You must be signed in to change notification settings - Fork 210
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,13 +25,14 @@ type Config struct { | |
| TokenManager *TokenManager `yaml:"token_manager"` | ||
| GRPCClientTLS *shared.GRPCClientTLS `yaml:"grpc_client_tls"` | ||
|
|
||
| Application Application `yaml:"application"` | ||
| Spaces Spaces `yaml:"spaces"` | ||
| Identity Identity `yaml:"identity"` | ||
| IncludeOCMSharees bool `yaml:"include_ocm_sharees" env:"OC_ENABLE_OCM;GRAPH_INCLUDE_OCM_SHAREES" desc:"Include OCM sharees when listing users." introductionVersion:"1.0.0"` | ||
| Events Events `yaml:"events"` | ||
| UnifiedRoles UnifiedRoles `yaml:"unified_roles"` | ||
| MaxConcurrency int `yaml:"max_concurrency" env:"OC_MAX_CONCURRENCY;GRAPH_MAX_CONCURRENCY" desc:"The maximum number of concurrent requests the service will handle." introductionVersion:"1.0.0"` | ||
| Application Application `yaml:"application"` | ||
| Spaces Spaces `yaml:"spaces"` | ||
| Identity Identity `yaml:"identity"` | ||
| OIDCProfilePicture OIDCProfilePicture `yaml:"oidc_profile_picture"` | ||
| IncludeOCMSharees bool `yaml:"include_ocm_sharees" env:"OC_ENABLE_OCM;GRAPH_INCLUDE_OCM_SHAREES" desc:"Include OCM sharees when listing users." introductionVersion:"1.0.0"` | ||
| Events Events `yaml:"events"` | ||
| UnifiedRoles UnifiedRoles `yaml:"unified_roles"` | ||
| MaxConcurrency int `yaml:"max_concurrency" env:"OC_MAX_CONCURRENCY;GRAPH_MAX_CONCURRENCY" desc:"The maximum number of concurrent requests the service will handle." introductionVersion:"1.0.0"` | ||
|
|
||
| Keycloak Keycloak `yaml:"keycloak"` | ||
| ServiceAccount ServiceAccount `yaml:"service_account"` | ||
|
|
@@ -116,6 +117,11 @@ type Identity struct { | |
| LDAP LDAP `yaml:"ldap"` | ||
| } | ||
|
|
||
| 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"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be clear that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not only valid: but trusted urls - otherwise it's an SSRF attack vector |
||
| } | ||
|
|
||
| // API represents API configuration parameters. | ||
| type API struct { | ||
| GroupMembersPatchLimit int `yaml:"group_members_patch_limit" env:"GRAPH_GROUP_MEMBERS_PATCH_LIMIT" desc:"The amount of group members allowed to be added with a single patch request." introductionVersion:"1.0.0"` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ type Options struct { | |
| Context context.Context | ||
| Logger log.Logger | ||
| Config *config.Config | ||
| ProfilePictureHTTPClient HTTPClient | ||
| Middleware []func(http.Handler) http.Handler | ||
| RequireAdminMiddleware func(http.Handler) http.Handler | ||
| GatewaySelector pool.Selectable[gateway.GatewayAPIClient] | ||
|
|
@@ -47,6 +48,13 @@ type Options struct { | |
| NatsKeyValue jetstream.KeyValue | ||
| } | ||
|
|
||
| // ProfilePictureHTTPClient provides a function to set the HTTP client used for downloading OIDC profile pictures. | ||
| func ProfilePictureHTTPClient(val HTTPClient) Option { | ||
| return func(o *Options) { | ||
| o.ProfilePictureHTTPClient = val | ||
| } | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick but I think this belongs further down in the file |
||
| // newOptions initializes the available default options. | ||
| func newOptions(opts ...Option) Options { | ||
| opt := Options{} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,25 @@ | ||
| package svc | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "crypto/tls" | ||
| "crypto/x509" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "net/url" | ||
| "os" | ||
| "path" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/go-chi/chi/v5" | ||
| "github.com/go-chi/chi/v5/middleware" | ||
| ldapv3 "github.com/go-ldap/ldap/v3" | ||
| "github.com/gobwas/glob" | ||
| "github.com/jellydator/ttlcache/v3" | ||
| "github.com/opencloud-eu/opencloud/services/graph/pkg/identity/cache" | ||
| "github.com/riandyrn/otelchi" | ||
|
|
@@ -39,8 +45,9 @@ import ( | |
|
|
||
| const ( | ||
| // HeaderPurge defines the header name for the purge header. | ||
| HeaderPurge = "Purge" | ||
| displayNameAttr = "displayName" | ||
| HeaderPurge = "Purge" | ||
| displayNameAttr = "displayName" | ||
| maxProfilePhotoBytes = 10 << 20 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ) | ||
|
|
||
| // Service defines the service handlers. | ||
|
|
@@ -191,6 +198,7 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx | |
| BaseGraphService: baseGraphService, | ||
| mux: m, | ||
| specialDriveItemsCache: spacePropertiesCache, | ||
| userProfilePhotoService: options.UserProfilePhotoService, | ||
| eventsPublisher: options.EventsPublisher, | ||
| eventsConsumer: options.EventsConsumer, | ||
| searchService: options.SearchService, | ||
|
|
@@ -200,6 +208,10 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx | |
| traceProvider: options.TraceProvider, | ||
| valueService: options.ValueService, | ||
| natskv: options.NatsKeyValue, | ||
| profilePictureHTTPClient: options.ProfilePictureHTTPClient, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not keep this as a member in the photo service? |
||
| } | ||
| if svc.profilePictureHTTPClient == nil { | ||
| svc.profilePictureHTTPClient = http.DefaultClient | ||
| } | ||
|
|
||
| if err := setIdentityBackends(options, &svc); err != nil { | ||
|
|
@@ -578,6 +590,11 @@ func (g *Graph) StartListenForLogonEvents(ctx context.Context, l log.Logger) err | |
| if err := g.identityBackend.UpdateLastSignInDate(ctx, ev.Executant.OpaqueId, utils.TSToTime(ev.Timestamp)); err != nil { | ||
| l.Error().Err(err).Str("userid", ev.Executant.OpaqueId).Msg("Error updating last sign in date") | ||
| } | ||
| if ev.PictureURL != "" { | ||
| if err := g.syncProfilePictureFromURL(ctx, ev.Executant.GetOpaqueId(), ev.PictureURL); err != nil { | ||
| l.Warn().Err(err).Str("userid", ev.Executant.GetOpaqueId()).Msg("Failed to sync profile picture from OIDC claim") | ||
| } | ||
| } | ||
| } | ||
| case <-ctx.Done(): | ||
| l.Info().Msg("context cancelled") | ||
|
|
@@ -588,6 +605,125 @@ func (g *Graph) StartListenForLogonEvents(ctx context.Context, l log.Logger) err | |
| return nil | ||
| } | ||
|
|
||
| func (g *Graph) syncProfilePictureFromURL(ctx context.Context, userID, rawURL string) error { | ||
| if g.userProfilePhotoService == nil { | ||
| return errors.New("profile photo service not configured") | ||
| } | ||
| if userID == "" { | ||
| return errors.New("missing user id for profile picture sync") | ||
| } | ||
| if !g.isProfilePictureURLAllowed(rawURL) { | ||
| return fmt.Errorf("profile picture URL not allowed: %s", rawURL) | ||
| } | ||
|
|
||
| data, err := g.fetchProfilePicture(ctx, rawURL) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return g.userProfilePhotoService.UpdatePhoto(ctx, userID, bytes.NewReader(data)) | ||
| } | ||
|
|
||
| func (g *Graph) fetchProfilePicture(ctx context.Context, rawURL string) ([]byte, error) { | ||
| request, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| request.Header.Set("Accept", "image/*") | ||
|
|
||
| resp, err := g.profilePictureHTTPClient.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) | ||
| if !strings.HasPrefix(contentType, "image/") { | ||
| return nil, fmt.Errorf("unsupported profile picture content type: %s", contentType) | ||
| } | ||
|
|
||
| return data, nil | ||
| } | ||
|
|
||
| func (g *Graph) isProfilePictureURLAllowed(rawURL string) bool { | ||
| parsedURL, err := url.Parse(rawURL) | ||
| if err != nil || parsedURL.Host == "" { | ||
| return false | ||
| } | ||
| if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" { | ||
| return false | ||
| } | ||
|
|
||
| for _, pattern := range g.profilePictureAllowlistPatterns() { | ||
| if pattern == "*" { | ||
| return true | ||
| } | ||
| if urlPatternMatches(pattern, parsedURL) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func (g *Graph) profilePictureAllowlistPatterns() []string { | ||
| if len(g.config.OIDCProfilePicture.URLAllowlist) > 0 { | ||
| return g.config.OIDCProfilePicture.URLAllowlist | ||
| } | ||
| issuerURL, err := url.Parse(g.config.OIDCProfilePicture.OIDCIssuer) | ||
| if err != nil || issuerURL.Host == "" { | ||
| return nil | ||
| } | ||
| return []string{fmt.Sprintf("%s://%s", issuerURL.Scheme, issuerURL.Host)} | ||
| } | ||
|
|
||
| func urlPatternMatches(pattern string, target *url.URL) bool { | ||
| if target == nil { | ||
| return false | ||
| } | ||
| parsedPattern, err := url.Parse(pattern) | ||
| if err == nil && parsedPattern.Host != "" { | ||
| if parsedPattern.Scheme != "" && !strings.EqualFold(parsedPattern.Scheme, target.Scheme) { | ||
| return false | ||
| } | ||
| hostMatcher, err := glob.Compile(strings.ToLower(parsedPattern.Host)) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| if !hostMatcher.Match(strings.ToLower(target.Host)) { | ||
| return false | ||
| } | ||
| if parsedPattern.Path == "" || parsedPattern.Path == "/" { | ||
| return true | ||
| } | ||
| if strings.HasSuffix(parsedPattern.Path, "*") { | ||
| prefix := strings.TrimSuffix(parsedPattern.Path, "*") | ||
| return strings.HasPrefix(target.Path, prefix) | ||
| } | ||
| return path.Clean(parsedPattern.Path) == path.Clean(target.Path) | ||
| } | ||
|
|
||
| hostMatcher, err := glob.Compile(strings.ToLower(pattern)) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return hostMatcher.Match(strings.ToLower(target.Host)) | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My feeling is that |
||
| // parseHeaderPurge parses the 'Purge' header. | ||
| // '1', 't', 'T', 'TRUE', 'true', 'True' are parsed as true | ||
| // all other values are false. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -367,6 +367,7 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, | |
| middleware.UserOIDCClaim(cfg.UserOIDCClaim), | ||
| middleware.UserCS3Claim(cfg.UserCS3Claim), | ||
| middleware.TenantOIDCClaim(cfg.TenantOIDCClaim), | ||
| middleware.AutoProvisionClaims(cfg.AutoProvisionClaims), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you have to add it here? It was already added in another place (conditionally) |
||
| middleware.TenantIDMappingEnabled(cfg.TenantIDMappingEnabled), | ||
| middleware.ServiceAccount(cfg.ServiceAccount), | ||
| middleware.WithRevaGatewaySelector(gatewaySelector), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,7 @@ func DefaultConfig() *config.Config { | |
| Username: "preferred_username", | ||
| Email: "email", | ||
| DisplayName: "name", | ||
| ProfilePicture: "", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| Groups: "groups", | ||
| }, | ||
| EnableBasicAuth: false, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,7 @@ func AccountResolver(optionSetters ...Option) func(next http.Handler) http.Handl | |
| userOIDCClaim: options.UserOIDCClaim, | ||
| userCS3Claim: options.UserCS3Claim, | ||
| tenantOIDCClaim: options.TenantOIDCClaim, | ||
| autoProvisionClaims: options.AutoProvisionClaims, | ||
| tenantIDMappingEnabled: options.TenantIDMappingEnabled, | ||
| gatewaySelector: options.RevaGatewaySelector, | ||
| serviceAccount: options.ServiceAccount, | ||
|
|
@@ -82,6 +83,7 @@ type accountResolver struct { | |
| userOIDCClaim string | ||
| userCS3Claim string | ||
| tenantOIDCClaim string | ||
| autoProvisionClaims config.AutoProvisionClaims | ||
| // 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. | ||
|
|
@@ -126,6 +128,18 @@ func readStringClaim(path string, claims map[string]any) (string, error) { | |
| return value, fmt.Errorf("claim path '%s' not set or empty", path) | ||
| } | ||
|
|
||
| func (m accountResolver) readProfilePictureURL(claims map[string]any) string { | ||
| if m.autoProvisionClaims.ProfilePicture == "" { | ||
| return "" | ||
| } | ||
| 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") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the wording should be a more neutral? "missing" sounds like a misconfiguration, maybe "no profile picture claim found" would be more neutral. |
||
| return "" | ||
| } | ||
| return pictureURL | ||
| } | ||
|
|
||
| // 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) { | ||
| ctx, span := m.tracer.Start(req.Context(), fmt.Sprintf("%s %s", req.Method, req.URL.Path), trace.WithSpanKind(trace.SpanKindServer)) | ||
|
|
@@ -229,9 +243,11 @@ func (m accountResolver) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |
|
|
||
| // If this is a new session, publish user login event | ||
| if newSession := oidc.NewSessionFlagFromContext(ctx); newSession && m.eventsPublisher != nil { | ||
| pictureURL := m.readProfilePictureURL(claims) | ||
| event := events.UserSignedIn{ | ||
| Executant: user.Id, | ||
| Timestamp: utils.TimeToTS(time.Now()), | ||
| Executant: user.Id, | ||
| PictureURL: pictureURL, | ||
| Timestamp: utils.TimeToTS(time.Now()), | ||
| } | ||
| if err := events.Publish(req.Context(), m.eventsPublisher, event); err != nil { | ||
| m.logger.Error().Err(err).Msg("could not publish user signin event.") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit weird to have an explicit setting just to make it the default for another setting - not sure...