From 37c81121bb846f354b10a3153b9bf8b5b74adbd6 Mon Sep 17 00:00:00 2001 From: Todd Kazakov Date: Fri, 7 Aug 2026 12:16:33 +0300 Subject: [PATCH] Address code review feedback from 2026-08-07 review - Fix profile attribute merge bug: clear existing profile attributes before applying the updated profile's attributes so cleared fields no longer retain stale values in keycloak (introduces RemoveProfileAttributes and attribute name constants as a single source of truth) - Fix terms_and_conditions round-trip: convert the terms accepted timestamp back to a unix timestamp string when updating a user, matching the format read by newUserFromGocloakUser - Remove unused User fields (Emails, PwHash, Hash, PasswordExists) and methods (IsClinic, IsClinician, IsCustodialAccount, AreTermsAccepted) - Remove bson tags from User since it is never stored in mongo - Call IsUnclaimedCustodialEmail in the keycloak client so unclaimed custodial placeholder emails are not exposed as the account username - Consolidate user id validation: user.ValidateID is the single source of truth, auth.ValidateUserID delegates to it - Slim UserAccessor: remove unused FindUser/FindUsersWithIds, replace FindUserById with the embedded user.Client interface (Get) - Move keycloak-specific types (TokenIntrospectionResult et al) and time helpers from the user package to user/keycloak - Move LegacySeagullProfileRepository from auth/store/mongo to user/store/mongo - Drop the unused username parameter of ProfileFromAttributes - Remove the userlib import alias in user/keycloak per import conventions --- auth/service/api/v1/profile.go | 4 +- auth/service/api/v1/router_test.go | 16 +-- auth/service/service/service.go | 3 +- auth/user.go | 7 +- user/keycloak/client.go | 121 ++++++++---------- user/keycloak/timeutil.go | 27 ++++ user/keycloak/token.go | 37 ++++++ user/keycloak/user_accessor.go | 59 +++------ user/profile.go | 109 +++++++++++----- user/profile_test.go | 55 ++++++++ .../legacy_seagull_profile_repository.go | 0 user/timeutil.go | 30 ----- user/user.go | 56 ++------ user/user_accessor.go | 47 +------ user/user_mock.go | 42 +----- user/user_test.go | 1 - 16 files changed, 301 insertions(+), 313 deletions(-) create mode 100644 user/keycloak/timeutil.go create mode 100644 user/keycloak/token.go rename {auth => user}/store/mongo/legacy_seagull_profile_repository.go (100%) delete mode 100644 user/timeutil.go diff --git a/auth/service/api/v1/profile.go b/auth/service/api/v1/profile.go index c1733b6e20..b8ad4779f9 100644 --- a/auth/service/api/v1/profile.go +++ b/auth/service/api/v1/profile.go @@ -95,7 +95,7 @@ func (r *Router) GetUsersWithProfiles(res rest.ResponseWriter, req *rest.Request responder := request.MustNewResponder(res, req) ctx := req.Context() targetUserID := req.PathParam("userId") - targetUser, err := r.UserAccessor().FindUserById(ctx, targetUserID) + targetUser, err := r.UserAccessor().Get(ctx, targetUserID) if err != nil { r.handleUserOrProfileErr(responder, err) return @@ -148,7 +148,7 @@ func (r *Router) GetUsersWithProfiles(res rest.ResponseWriter, req *rest.Request for userID, trustPerms := range mergedUserPerms { userID, trustPerms := userID, trustPerms group.Go(func() error { - sharedUser, err := r.UserAccessor().FindUserById(ctx, userID) + sharedUser, err := r.UserAccessor().Get(ctx, userID) if stdErrs.Is(err, user.ErrUserNotFound) || sharedUser == nil { // According to seagull code, "It's possible for a user profile to be deleted before the sharing permissions", so we can ignore if user or profile not found. return nil diff --git a/auth/service/api/v1/router_test.go b/auth/service/api/v1/router_test.go index 80f9b3575e..b9c17978dd 100644 --- a/auth/service/api/v1/router_test.go +++ b/auth/service/api/v1/router_test.go @@ -111,7 +111,7 @@ var _ = Describe("Router", func() { userRoles = []string{user.RolePatient} userAccessor.EXPECT(). - FindUserById(gomock.Any(), userID). + Get(gomock.Any(), userID). Return(userDetails, nil).AnyTimes() }) @@ -228,7 +228,7 @@ var _ = Describe("Router", func() { FindLegacyUserProfile(gomock.Any(), otherPersonID). Return(otherProfile, nil).AnyTimes() userAccessor.EXPECT(). - FindUserById(gomock.Any(), otherPersonID). + Get(gomock.Any(), otherPersonID). Return(otherDetails, nil).AnyTimes() handlerFunc(res, req) Expect(res.WriteHeaderInputs).To(Equal([]int{http.StatusOK})) @@ -247,7 +247,7 @@ var _ = Describe("Router", func() { FindLegacyUserProfile(gomock.Any(), otherPersonID). Return(otherProfile, nil).AnyTimes() userAccessor.EXPECT(). - FindUserById(gomock.Any(), otherPersonID). + Get(gomock.Any(), otherPersonID). Return(otherDetails, nil).AnyTimes() handlerFunc(res, req) Expect(res.WriteHeaderInputs).To(Equal([]int{http.StatusOK})) @@ -431,7 +431,7 @@ var _ = Describe("Router", func() { FindLegacyUserProfile(gomock.Any(), otherPersonID). Return(otherProfile.ToLegacyProfile(otherRoles), nil).AnyTimes() userAccessor.EXPECT(). - FindUserById(gomock.Any(), otherPersonID). + Get(gomock.Any(), otherPersonID). Return(otherDetails, nil).AnyTimes() handlerFunc(res, req) Expect(res.WriteHeaderInputs).To(Equal([]int{http.StatusOK})) @@ -450,7 +450,7 @@ var _ = Describe("Router", func() { FindLegacyUserProfile(gomock.Any(), otherPersonID). Return(otherProfile.ToLegacyProfile(otherRoles), nil).AnyTimes() userAccessor.EXPECT(). - FindUserById(gomock.Any(), otherPersonID). + Get(gomock.Any(), otherPersonID). Return(otherDetails, nil).AnyTimes() handlerFunc(res, req) Expect(res.WriteHeaderInputs).To(Equal([]int{http.StatusOK})) @@ -587,7 +587,6 @@ var _ = Describe("Router", func() { Username: pointer.FromString("dev@tidepool.org"), EmailVerified: pointer.FromBool(true), Roles: &userRoles, - Emails: []string{"dev@tidepool.org"}, Profile: &userProfile, } sanitizedUserDetails = &user.User{ @@ -595,7 +594,6 @@ var _ = Describe("Router", func() { Username: pointer.FromString("dev@tidepool.org"), EmailVerified: pointer.FromBool(true), Roles: &userRoles, - Emails: []string{"dev@tidepool.org"}, Profile: &userProfile, } @@ -615,7 +613,6 @@ var _ = Describe("Router", func() { Username: pointer.FromString("sharee@tidepool.org"), EmailVerified: pointer.FromBool(true), Roles: &shareeRoles, - Emails: []string{"sharee@tidepool.org"}, Profile: &shareeProfile, } limitedShareeDetails = &user.User{ @@ -623,13 +620,12 @@ var _ = Describe("Router", func() { Username: pointer.FromString("sharee@tidepool.org"), EmailVerified: pointer.FromBool(true), Roles: &shareeRoles, - Emails: []string{"sharee@tidepool.org"}, Profile: &limitedShareeProfile, } var s string userAccessor.EXPECT(). - FindUserById(gomock.Any(), gomock.AssignableToTypeOf(s)). + Get(gomock.Any(), gomock.AssignableToTypeOf(s)). DoAndReturn( func(ctx context.Context, id string) (*user.User, error) { switch id { diff --git a/auth/service/service/service.go b/auth/service/service/service.go index 824c67526b..225db6f489 100644 --- a/auth/service/service/service.go +++ b/auth/service/service/service.go @@ -13,6 +13,7 @@ import ( "github.com/tidepool-org/platform/user" "github.com/tidepool-org/platform/user/keycloak" + userStoreMongo "github.com/tidepool-org/platform/user/store/mongo" eventsCommon "github.com/tidepool-org/go-common/events" confirmationClient "github.com/tidepool-org/hydrophone/client" @@ -741,7 +742,7 @@ func (s *Service) initializeUserProfileAccessor(userAccessor user.UserAccessor) s.Logger().Debug("creating legacy seagull profile accessor") - repo, err := authStoreMongo.NewLegacySeagullProfileRepository(cfg) + repo, err := userStoreMongo.NewLegacySeagullProfileRepository(cfg) if err != nil { return errors.Wrap(err, "unable to create fallback user profile repository") } diff --git a/auth/user.go b/auth/user.go index 7f90a955b1..0db0bd0896 100644 --- a/auth/user.go +++ b/auth/user.go @@ -1,12 +1,11 @@ package auth import ( - "regexp" - "github.com/tidepool-org/platform/errors" "github.com/tidepool-org/platform/id" "github.com/tidepool-org/platform/structure" structureValidator "github.com/tidepool-org/platform/structure/validator" + "github.com/tidepool-org/platform/user" ) func NewUserID() string { @@ -24,7 +23,7 @@ func UserIDValidator(value string, errorReporter structure.ErrorReporter) { func ValidateUserID(value string) error { if value == "" { return structureValidator.ErrorValueEmpty() - } else if !idExpression.MatchString(value) { + } else if !user.IsValidID(value) { return ErrorValueStringAsUserIDNotValid(value) } return nil @@ -33,5 +32,3 @@ func ValidateUserID(value string) error { func ErrorValueStringAsUserIDNotValid(value string) error { return errors.Preparedf(structureValidator.ErrorCodeValueNotValid, "value is not valid", "value %q is not valid as user id", value) } - -var idExpression = regexp.MustCompile("\\A(?:[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|[0-9a-f]{10})\\z") diff --git a/user/keycloak/client.go b/user/keycloak/client.go index 994fb244f8..35e1cf890d 100644 --- a/user/keycloak/client.go +++ b/user/keycloak/client.go @@ -16,7 +16,7 @@ import ( "golang.org/x/oauth2" "github.com/tidepool-org/platform/pointer" - userlib "github.com/tidepool-org/platform/user" + "github.com/tidepool-org/platform/user" ) const ( @@ -129,7 +129,7 @@ func (c *keycloakClient) RefreshToken(ctx context.Context, token oauth2.Token) ( return c.jwtToAccessToken(jwt), nil } -func (c *keycloakClient) GetUserById(ctx context.Context, id string) (*userlib.User, error) { +func (c *keycloakClient) GetUserById(ctx context.Context, id string) (*user.User, error) { if id == "" { return nil, nil } @@ -142,73 +142,62 @@ func (c *keycloakClient) GetUserById(ctx context.Context, id string) (*userlib.U return users[0], nil } -func (c *keycloakClient) GetUserByEmail(ctx context.Context, email string) (*userlib.User, error) { - if email == "" { - return nil, nil - } - token, err := c.getAdminToken(ctx) - if err != nil { - return nil, err - } - - users, err := c.keycloak.GetUsers(ctx, token.AccessToken, c.cfg.Realm, gocloak.GetUsersParams{ - Email: &email, - Exact: gocloak.BoolP(true), - }) - if err != nil || len(users) == 0 { - return nil, err - } - - return c.GetUserById(ctx, *users[0].ID) -} - -func (c *keycloakClient) UpdateUser(ctx context.Context, user *userlib.User) error { +func (c *keycloakClient) UpdateUser(ctx context.Context, u *user.User) error { token, err := c.getAdminToken(ctx) if err != nil { return err } gocloakUser := gocloak.User{ - ID: user.UserID, - Username: user.Username, - Enabled: &user.Enabled, - EmailVerified: user.EmailVerified, - Email: user.Username, + ID: u.UserID, + Username: u.Username, + Enabled: &u.Enabled, + EmailVerified: u.EmailVerified, + Email: u.Username, } attrs := map[string][]string{} - maps.Copy(attrs, user.Attributes) - if terms := pointer.ToString(user.TermsAccepted); terms != "" { - attrs[termsAcceptedAttribute] = []string{terms} + maps.Copy(attrs, u.Attributes) + if terms := pointer.ToString(u.TermsAccepted); terms != "" { + // The terms accepted attribute is stored as a unix timestamp string + // in keycloak. If the timestamp cannot be converted, keep the + // existing attribute value from u.Attributes instead. + if unix, err := timestampToUnixString(terms); err == nil { + attrs[termsAcceptedAttribute] = []string{unix} + } } - if user.Profile != nil { - maps.Copy(attrs, user.Profile.ToAttributes()) + if u.Profile != nil { + // Remove all existing profile attributes before applying the profile + // so that fields cleared in the updated profile don't retain their + // stale values. Attributes not managed by the profile are preserved. + user.RemoveProfileAttributes(attrs) + maps.Copy(attrs, u.Profile.ToAttributes()) } gocloakUser.Attributes = &attrs if err := c.keycloak.UpdateUser(ctx, token.AccessToken, c.cfg.Realm, gocloakUser); err != nil { return err } - if err := c.updateRolesForUser(ctx, user); err != nil { + if err := c.updateRolesForUser(ctx, u); err != nil { return err } return nil } -func (c *keycloakClient) UpdateUserProfile(ctx context.Context, id string, p *userlib.Profile) error { - user, err := c.GetUserById(ctx, id) +func (c *keycloakClient) UpdateUserProfile(ctx context.Context, id string, p *user.Profile) error { + u, err := c.GetUserById(ctx, id) if err != nil { return err } - if user == nil { - return userlib.ErrUserNotFound + if u == nil { + return user.ErrUserNotFound } - user.Profile = p - return c.UpdateUser(ctx, user) + u.Profile = p + return c.UpdateUser(ctx, u) } -func (c *keycloakClient) FindUsersWithIds(ctx context.Context, ids []string) (users []*userlib.User, err error) { +func (c *keycloakClient) FindUsersWithIds(ctx context.Context, ids []string) (users []*user.User, err error) { const errMessage = "could not retrieve users by ids" token, err := c.getAdminToken(ctx) @@ -231,7 +220,7 @@ func (c *keycloakClient) FindUsersWithIds(ctx context.Context, ids []string) (us return nil, err } - users = make([]*userlib.User, len(res)) + users = make([]*user.User, len(res)) for i, u := range res { users[i] = newUserFromGocloakUser(u) } @@ -239,7 +228,7 @@ func (c *keycloakClient) FindUsersWithIds(ctx context.Context, ids []string) (us return users, nil } -func (c *keycloakClient) IntrospectToken(ctx context.Context, token oauth2.Token) (*userlib.TokenIntrospectionResult, error) { +func (c *keycloakClient) IntrospectToken(ctx context.Context, token oauth2.Token) (*TokenIntrospectionResult, error) { clientId, clientSecret := c.getClientAndSecretFromToken(ctx, token) rtr, err := c.keycloak.RetrospectToken( @@ -253,11 +242,11 @@ func (c *keycloakClient) IntrospectToken(ctx context.Context, token oauth2.Token return nil, err } - result := &userlib.TokenIntrospectionResult{ + result := &TokenIntrospectionResult{ Active: pointer.ToBool(rtr.Active), } if result.Active { - customClaims := &userlib.AccessTokenCustomClaims{} + customClaims := &AccessTokenCustomClaims{} _, err := c.keycloak.DecodeAccessTokenCustomClaims( ctx, token.AccessToken, @@ -270,7 +259,7 @@ func (c *keycloakClient) IntrospectToken(ctx context.Context, token oauth2.Token result.Subject = customClaims.Subject result.EmailVerified = customClaims.EmailVerified result.ExpiresAt = customClaims.ExpiresAt.Unix() - result.RealmAccess = userlib.RealmAccess{ + result.RealmAccess = RealmAccess{ Roles: customClaims.RealmAccess.Roles, } result.IdentityProvider = customClaims.IdentityProvider @@ -341,12 +330,12 @@ func (c *keycloakClient) adminTokenIsExpired() bool { return c.adminToken == nil || time.Now().After(c.adminTokenRefreshExpires) } -func (c *keycloakClient) updateRolesForUser(ctx context.Context, user *userlib.User) error { +func (c *keycloakClient) updateRolesForUser(ctx context.Context, u *user.User) error { token, err := c.getAdminToken(ctx) if err != nil { return err } - userID := pointer.ToString(user.UserID) + userID := pointer.ToString(u.UserID) realmRoles, err := c.keycloak.GetRealmRoles(ctx, token.AccessToken, c.cfg.Realm, gocloak.GetRoleParams{ Max: gocloak.IntP(1000), @@ -363,8 +352,8 @@ func (c *keycloakClient) updateRolesForUser(ctx context.Context, user *userlib.U var rolesToDelete []gocloak.Role targetRoles := make(map[string]struct{}) - if user.Roles != nil && len(*user.Roles) > 0 { - for _, targetRoleName := range *user.Roles { + if u.Roles != nil && len(*u.Roles) > 0 { + for _, targetRoleName := range *u.Roles { targetRoles[targetRoleName] = struct{}{} } } @@ -384,7 +373,7 @@ func (c *keycloakClient) updateRolesForUser(ctx context.Context, user *userlib.U if _, ok := targetRoles[*currentRole.Name]; !ok { // Only remove roles managed by shoreline - if _, ok := userlib.ShorelineManagedRoles[*currentRole.Name]; ok { + if _, ok := user.ShorelineManagedRoles[*currentRole.Name]; ok { rolesToDelete = append(rolesToDelete, *currentRole) } } @@ -448,41 +437,37 @@ func (c *keycloakClient) getClientAndSecretFromToken(ctx context.Context, token return clientId, clientSecret } -func newUserFromGocloakUser(gocloakUser *gocloak.User) *userlib.User { - user := &userlib.User{ +func newUserFromGocloakUser(gocloakUser *gocloak.User) *user.User { + u := &user.User{ UserID: gocloakUser.ID, Username: gocloakUser.Username, - Emails: []string{}, Roles: gocloakUser.RealmRoles, EmailVerified: gocloakUser.EmailVerified, Enabled: pointer.ToBool(gocloakUser.Enabled), } + // Unclaimed custodial accounts have a placeholder email generated during + // account creation. Don't expose it as the account's username. + if user.IsUnclaimedCustodialEmail(pointer.ToString(u.Username)) { + u.Username = nil + } if gocloakUser.Attributes != nil { attrs := *gocloakUser.Attributes if termsAttrs, ok := attrs[termsAcceptedAttribute]; ok && len(termsAttrs) > 0 { - if ts, err := userlib.UnixStringToTimestamp(termsAttrs[0]); err == nil { - user.TermsAccepted = &ts + if ts, err := unixStringToTimestamp(termsAttrs[0]); err == nil { + u.TermsAccepted = &ts } } var roles []string if gocloakUser.RealmRoles != nil { roles = *gocloakUser.RealmRoles } - if profile := userlib.ProfileFromAttributes(pointer.ToString(gocloakUser.Username), attrs, roles); profile != nil { - user.Profile = profile + if profile := user.ProfileFromAttributes(attrs, roles); profile != nil { + u.Profile = profile } - user.Attributes = attrs - } - - // All non-custodial users have a password and it's important to set the hash to a non-empty value. - // When users are serialized by this service, the payload contains a flag `passwordExists` that - // is computed based on the presence of a password hash in the user struct. This flag is used by - // other services (e.g. hydrophone) to determine whether the user is custodial or not. - if !user.IsCustodialAccount() { - user.PwHash = "true" + u.Attributes = attrs } - return user + return u } func getRealmRoleByName(realmRoles []*gocloak.Role, name string) *gocloak.Role { diff --git a/user/keycloak/timeutil.go b/user/keycloak/timeutil.go new file mode 100644 index 0000000000..75212ffba7 --- /dev/null +++ b/user/keycloak/timeutil.go @@ -0,0 +1,27 @@ +package keycloak + +import ( + "fmt" + "strconv" + "time" +) + +// timestampFormat is the format used for the terms accepted timestamp of a +// user. +const timestampFormat = "2006-01-02T15:04:05-07:00" + +func timestampToUnixString(timestamp string) (string, error) { + parsed, err := time.Parse(timestampFormat, timestamp) + if err != nil { + return "", err + } + return fmt.Sprintf("%v", parsed.Unix()), nil +} + +func unixStringToTimestamp(unixString string) (string, error) { + i, err := strconv.ParseInt(unixString, 10, 64) + if err != nil { + return "", err + } + return time.Unix(i, 0).Format(timestampFormat), nil +} diff --git a/user/keycloak/token.go b/user/keycloak/token.go new file mode 100644 index 0000000000..446cddf137 --- /dev/null +++ b/user/keycloak/token.go @@ -0,0 +1,37 @@ +package keycloak + +import ( + "github.com/Nerzal/gocloak/v13/pkg/jwx" +) + +const serverRole = "backend_service" + +type TokenIntrospectionResult struct { + Active bool `json:"active"` + Subject string `json:"sub"` + EmailVerified bool `json:"email_verified"` + ExpiresAt int64 `json:"eat"` + RealmAccess RealmAccess `json:"realm_access"` + IdentityProvider string `json:"identityProvider"` +} + +type AccessTokenCustomClaims struct { + jwx.Claims + IdentityProvider string `json:"identity_provider,omitempty"` +} + +type RealmAccess struct { + Roles []string `json:"roles"` +} + +func (t *TokenIntrospectionResult) IsServerToken() bool { + if len(t.RealmAccess.Roles) > 0 { + for _, role := range t.RealmAccess.Roles { + if role == serverRole { + return true + } + } + } + + return false +} diff --git a/user/keycloak/user_accessor.go b/user/keycloak/user_accessor.go index bf3fd11ec3..f238681605 100644 --- a/user/keycloak/user_accessor.go +++ b/user/keycloak/user_accessor.go @@ -4,7 +4,7 @@ import ( "context" "github.com/tidepool-org/platform/pointer" - userlib "github.com/tidepool-org/platform/user" + "github.com/tidepool-org/platform/user" ) type keycloakUserAccessor struct { @@ -17,74 +17,47 @@ func NewKeycloakUserAccessor(config *KeycloakConfig) *keycloakUserAccessor { } } -func (m *keycloakUserAccessor) FindUser(ctx context.Context, user *userlib.User) (*userlib.User, error) { - var foundUser *userlib.User - var err error - - if userlib.IsValidUserID(pointer.ToString(user.UserID)) { - foundUser, err = m.keycloakClient.GetUserById(ctx, pointer.ToString(user.UserID)) - } else { - email := "" - if len(user.Emails) > 0 { - email = user.Emails[0] - } - foundUser, err = m.keycloakClient.GetUserByEmail(ctx, email) +func (m *keycloakUserAccessor) Get(ctx context.Context, id string) (*user.User, error) { + if !user.IsValidID(id) { + return nil, user.ErrUserNotFound } - if err != nil && err != userlib.ErrUserNotFound { - return nil, err - } else if err == nil && foundUser != nil { - return foundUser, nil - } - // All users should be migrated into keycloak by the time this code is released. - return nil, userlib.ErrUserNotMigrated -} - -func (m *keycloakUserAccessor) FindUserById(ctx context.Context, id string) (*userlib.User, error) { - if !userlib.IsValidUserID(id) { - return nil, userlib.ErrUserNotFound - } - - user, err := m.keycloakClient.GetUserById(ctx, id) + u, err := m.keycloakClient.GetUserById(ctx, id) if err != nil { return nil, err } - if user == nil { - return nil, userlib.ErrUserNotFound + if u == nil { + return nil, user.ErrUserNotFound } - return user, nil + return u, nil } -func (m *keycloakUserAccessor) FindLegacyUserProfile(ctx context.Context, id string) (*userlib.LegacyUserProfile, error) { - user, err := m.FindUserById(ctx, id) +func (m *keycloakUserAccessor) FindLegacyUserProfile(ctx context.Context, id string) (*user.LegacyUserProfile, error) { + u, err := m.Get(ctx, id) if err != nil { return nil, err } - if user == nil || user.Profile == nil { - return nil, userlib.ErrUserProfileNotFound + if u == nil || u.Profile == nil { + return nil, user.ErrUserProfileNotFound } - return user.Profile.ToLegacyProfile(pointer.ToStringArray(user.Roles)), nil + return u.Profile.ToLegacyProfile(pointer.ToStringArray(u.Roles)), nil } func (m *keycloakUserAccessor) Roles(ctx context.Context, userID string) ([]string, error) { return m.keycloakClient.GetRolesForUser(ctx, userID) } -func (m *keycloakUserAccessor) FindUsersWithIds(ctx context.Context, ids []string) (users []*userlib.User, err error) { - return m.keycloakClient.FindUsersWithIds(ctx, ids) -} - -func (m *keycloakUserAccessor) UpdateLegacyUserProfile(ctx context.Context, userID string, p *userlib.LegacyUserProfile) error { +func (m *keycloakUserAccessor) UpdateLegacyUserProfile(ctx context.Context, userID string, p *user.LegacyUserProfile) error { roles, err := m.Roles(ctx, userID) if err != nil { return err } - if !userlib.HasClinicOrClinicianRole(roles) && p.Clinic != nil { + if !user.HasClinicOrClinicianRole(roles) && p.Clinic != nil { p.Clinic = nil } return m.keycloakClient.UpdateUserProfile(ctx, userID, p.ToUserProfile()) } -func (m *keycloakUserAccessor) UpdateUserProfile(ctx context.Context, userID string, p *userlib.Profile) error { +func (m *keycloakUserAccessor) UpdateUserProfile(ctx context.Context, userID string, p *user.Profile) error { return m.keycloakClient.UpdateUserProfile(ctx, userID, p) } diff --git a/user/profile.go b/user/profile.go index 93be76d0b2..52a52697c4 100644 --- a/user/profile.go +++ b/user/profile.go @@ -259,53 +259,102 @@ func (b *jsonBool) UnmarshalJSON(data []byte) error { return nil } +// Keycloak user attribute names used to store profile fields. +const ( + fullNameAttribute = "full_name" + custodianFullNameAttribute = "custodian_full_name" + hasCustodianAttribute = "has_custodian" + birthdayAttribute = "birthday" + diagnosisDateAttribute = "diagnosis_date" + diagnosisTypeAttribute = "diagnosis_type" + targetDevicesAttribute = "target_devices" + targetTimezoneAttribute = "target_timezone" + aboutAttribute = "about" + mrnAttribute = "mrn" + biologicalSexAttribute = "biological_sex" + clinicNameAttribute = "clinic_name" + clinicRoleAttribute = "clinic_role" + clinicTelephoneAttribute = "clinic_telephone" + clinicNPIAttribute = "clinic_npi" +) + +// profileAttributes lists every attribute that [Profile.ToAttributes] may set +// on a keycloak user. +var profileAttributes = []string{ + fullNameAttribute, + custodianFullNameAttribute, + hasCustodianAttribute, + birthdayAttribute, + diagnosisDateAttribute, + diagnosisTypeAttribute, + targetDevicesAttribute, + targetTimezoneAttribute, + aboutAttribute, + mrnAttribute, + biologicalSexAttribute, + clinicNameAttribute, + clinicRoleAttribute, + clinicTelephoneAttribute, + clinicNPIAttribute, +} + +// RemoveProfileAttributes deletes all profile attributes from attrs. It is +// used to clear stale values before applying the attributes of an updated +// profile, so that fields removed from the profile don't retain their old +// values. +func RemoveProfileAttributes(attrs map[string][]string) { + for _, attr := range profileAttributes { + delete(attrs, attr) + } +} + func (up *Profile) ToAttributes() map[string][]string { attributes := map[string][]string{} if up.FullName != "" { - addAttribute(attributes, "full_name", up.FullName) + addAttribute(attributes, fullNameAttribute, up.FullName) } if up.Custodian != nil && up.Custodian.FullName != "" { - addAttribute(attributes, "custodian_full_name", up.Custodian.FullName) + addAttribute(attributes, custodianFullNameAttribute, up.Custodian.FullName) // The "has_custodian" attribute is only added so that filtering on users is simpler via the keycloak API - because // there is a way to filter by custom attribute values but not by the presence of one. - addAttribute(attributes, "has_custodian", "true") + addAttribute(attributes, hasCustodianAttribute, "true") } if string(up.Birthday) != "" { - addAttribute(attributes, "birthday", string(up.Birthday)) + addAttribute(attributes, birthdayAttribute, string(up.Birthday)) } if string(up.DiagnosisDate) != "" { - addAttribute(attributes, "diagnosis_date", string(up.DiagnosisDate)) + addAttribute(attributes, diagnosisDateAttribute, string(up.DiagnosisDate)) } if up.DiagnosisType != "" { - addAttribute(attributes, "diagnosis_type", up.DiagnosisType) + addAttribute(attributes, diagnosisTypeAttribute, up.DiagnosisType) } - addAttributes(attributes, "target_devices", up.TargetDevices...) + addAttributes(attributes, targetDevicesAttribute, up.TargetDevices...) if up.TargetTimezone != "" { - addAttribute(attributes, "target_timezone", up.TargetTimezone) + addAttribute(attributes, targetTimezoneAttribute, up.TargetTimezone) } if up.About != "" { - addAttribute(attributes, "about", up.About) + addAttribute(attributes, aboutAttribute, up.About) } if up.MRN != "" { - addAttribute(attributes, "mrn", up.MRN) + addAttribute(attributes, mrnAttribute, up.MRN) } if up.BiologicalSex != "" { - addAttribute(attributes, "biological_sex", up.BiologicalSex) + addAttribute(attributes, biologicalSexAttribute, up.BiologicalSex) } if up.Clinic != nil { if val := pointer.ToString(up.Clinic.Name); val != "" { - addAttribute(attributes, "clinic_name", val) + addAttribute(attributes, clinicNameAttribute, val) } if val := pointer.ToString(up.Clinic.Role); val != "" { - addAttribute(attributes, "clinic_role", val) + addAttribute(attributes, clinicRoleAttribute, val) } if val := pointer.ToString(up.Clinic.Telephone); val != "" { - addAttribute(attributes, "clinic_telephone", val) + addAttribute(attributes, clinicTelephoneAttribute, val) } if val := pointer.ToString(up.Clinic.NPI); val != "" { - addAttribute(attributes, "clinic_npi", val) + addAttribute(attributes, clinicNPIAttribute, val) } } @@ -314,48 +363,48 @@ func (up *Profile) ToAttributes() map[string][]string { // ProfileFromAttributes returns a [Profile] if there exists at least one // profile attribute in the supplied attributes. Otherwise it returns nil. -func ProfileFromAttributes(username string, attributes map[string][]string, roles []string) *Profile { +func ProfileFromAttributes(attributes map[string][]string, roles []string) *Profile { up := &Profile{} foundAnyProfileAttr := false - if val := getAttribute(attributes, "full_name"); val != "" { + if val := getAttribute(attributes, fullNameAttribute); val != "" { up.FullName = val foundAnyProfileAttr = true } - if val := getAttribute(attributes, "custodian_full_name"); val != "" { + if val := getAttribute(attributes, custodianFullNameAttribute); val != "" { up.Custodian = &Custodian{ FullName: val, } foundAnyProfileAttr = true } - if val := getAttribute(attributes, "birthday"); val != "" { + if val := getAttribute(attributes, birthdayAttribute); val != "" { up.Birthday = Date(val) foundAnyProfileAttr = true } - if val := getAttribute(attributes, "diagnosis_date"); val != "" { + if val := getAttribute(attributes, diagnosisDateAttribute); val != "" { up.DiagnosisDate = Date(val) foundAnyProfileAttr = true } - if val := getAttribute(attributes, "diagnosis_type"); val != "" { + if val := getAttribute(attributes, diagnosisTypeAttribute); val != "" { up.DiagnosisType = val foundAnyProfileAttr = true } - if vals := getAttributes(attributes, "target_devices"); len(vals) > 0 { + if vals := getAttributes(attributes, targetDevicesAttribute); len(vals) > 0 { up.TargetDevices = vals foundAnyProfileAttr = true } - if val := getAttribute(attributes, "target_timezone"); val != "" { + if val := getAttribute(attributes, targetTimezoneAttribute); val != "" { up.TargetTimezone = val foundAnyProfileAttr = true } - if val := getAttribute(attributes, "about"); val != "" { + if val := getAttribute(attributes, aboutAttribute); val != "" { up.About = val foundAnyProfileAttr = true } - if val := getAttribute(attributes, "mrn"); val != "" { + if val := getAttribute(attributes, mrnAttribute); val != "" { up.MRN = val foundAnyProfileAttr = true } - if val := getAttribute(attributes, "biological_sex"); val != "" { + if val := getAttribute(attributes, biologicalSexAttribute); val != "" { up.BiologicalSex = val foundAnyProfileAttr = true } @@ -365,19 +414,19 @@ func ProfileFromAttributes(username string, attributes map[string][]string, role // returned so check both the presence of the clinic / clinician role and // individual clinic properties - It may be enough to just check the roles hasClinicProfile := HasClinicOrClinicianRole(roles) - if val := getAttribute(attributes, "clinic_name"); val != "" { + if val := getAttribute(attributes, clinicNameAttribute); val != "" { clinicProfile.Name = pointer.FromString(val) hasClinicProfile = true } - if val := getAttribute(attributes, "clinic_role"); val != "" { + if val := getAttribute(attributes, clinicRoleAttribute); val != "" { clinicProfile.Role = pointer.FromString(val) hasClinicProfile = true } - if val := getAttribute(attributes, "clinic_telephone"); val != "" { + if val := getAttribute(attributes, clinicTelephoneAttribute); val != "" { clinicProfile.Telephone = pointer.FromString(val) hasClinicProfile = true } - if val := getAttribute(attributes, "clinic_npi"); val != "" { + if val := getAttribute(attributes, clinicNPIAttribute); val != "" { clinicProfile.NPI = pointer.FromString(val) hasClinicProfile = true } diff --git a/user/profile_test.go b/user/profile_test.go index 6f40c5ff1d..23ba6798a0 100644 --- a/user/profile_test.go +++ b/user/profile_test.go @@ -108,5 +108,60 @@ var _ = Describe("User", func() { []string{user.RoleClinician}, ), ) + + Context("RemoveProfileAttributes", func() { + It("removes stale profile attributes while preserving non profile attributes", func() { + existingProfile := &user.Profile{ + FullName: "Bob", + Birthday: "2000-02-03", + About: "About me", + MRN: "1112222", + BiologicalSex: "male", + } + attrs := existingProfile.ToAttributes() + attrs["terms_and_conditions"] = []string{"1234567890"} + attrs["unrelated_attribute"] = []string{"value"} + + // Simulate an update that clears all patient fields except the full name. + updatedProfile := &user.Profile{ + FullName: "Bob", + } + user.RemoveProfileAttributes(attrs) + for attribute, values := range updatedProfile.ToAttributes() { + attrs[attribute] = values + } + + Expect(attrs).To(Equal(map[string][]string{ + "full_name": {"Bob"}, + "terms_and_conditions": {"1234567890"}, + "unrelated_attribute": {"value"}, + })) + }) + + It("removes every attribute a fully populated profile can produce", func() { + fullProfile := &user.Profile{ + FullName: "Bob", + Birthday: "2000-02-03", + DiagnosisDate: "2001-03-05", + DiagnosisType: user.DiabetesTypeType1, + TargetDevices: []string{"SomeDevice900"}, + TargetTimezone: "UTC", + About: "About me", + MRN: "1112222", + BiologicalSex: "male", + Custodian: &user.Custodian{FullName: "Alice"}, + Clinic: &user.ClinicProfile{ + Name: pointer.FromString("Clinic Name"), + Role: pointer.FromString("Some Role"), + Telephone: pointer.FromString("123-123-3456"), + NPI: pointer.FromString("1234567890"), + }, + } + attrs := fullProfile.ToAttributes() + Expect(attrs).ToNot(BeEmpty()) + user.RemoveProfileAttributes(attrs) + Expect(attrs).To(BeEmpty()) + }) + }) }) }) diff --git a/auth/store/mongo/legacy_seagull_profile_repository.go b/user/store/mongo/legacy_seagull_profile_repository.go similarity index 100% rename from auth/store/mongo/legacy_seagull_profile_repository.go rename to user/store/mongo/legacy_seagull_profile_repository.go diff --git a/user/timeutil.go b/user/timeutil.go deleted file mode 100644 index a0e6cf5109..0000000000 --- a/user/timeutil.go +++ /dev/null @@ -1,30 +0,0 @@ -package user - -import ( - "fmt" - "strconv" - "time" -) - -func ParseTimestamp(timestamp string) (time.Time, error) { - return time.Parse(TimestampFormat, timestamp) -} - -func TimestampToUnixString(timestamp string) (unix string, err error) { - parsed, err := ParseTimestamp(timestamp) - if err != nil { - return - } - unix = fmt.Sprintf("%v", parsed.Unix()) - return -} - -func UnixStringToTimestamp(unixString string) (timestamp string, err error) { - i, err := strconv.ParseInt(unixString, 10, 64) - if err != nil { - return - } - t := time.Unix(i, 0) - timestamp = t.Format(TimestampFormat) - return -} diff --git a/user/user.go b/user/user.go index b6af230dac..71cae885a3 100644 --- a/user/user.go +++ b/user/user.go @@ -37,7 +37,11 @@ var ( RolePatient: struct{}{}, } - idExpression = regexp.MustCompile(`^([0-9a-f]{10}|[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12})$`) + // idExpression matches user ids in either the uuid hex 8-4-4-4-12 format + // or the legacy 10 character hex format. It is the single source of truth + // for user id validation - [ValidateID] in this package and + // auth.ValidateUserID are both based on it. + idExpression = regexp.MustCompile(`\A(?:[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|[0-9a-f]{10})\z`) custodialAccountRegexp = regexp.MustCompile(`(?i)unclaimed-custodial-automation\+\d+@tidepool\.org`) ) @@ -59,18 +63,14 @@ type Client interface { } type User struct { - UserID *string `json:"userid,omitempty" bson:"userid,omitempty"` - Username *string `json:"username,omitempty" bson:"username,omitempty"` - EmailVerified *bool `json:"emailVerified,omitempty" bson:"emailVerified,omitempty"` - TermsAccepted *string `json:"termsAccepted,omitempty" bson:"termsAccepted,omitempty"` - Roles *[]string `json:"roles,omitempty" bson:"roles,omitempty"` - Emails []string `json:"emails,omitempty" bson:"emails,omitempty"` - PwHash string `json:"-" bson:"pwhash,omitempty"` - Hash string `json:"-" bson:"userhash,omitempty"` - Enabled bool `json:"-" bson:"-"` - Profile *Profile `json:"profile,omitempty" bson:"-"` - PasswordExists *bool `json:"passwordExists,omitempty" bson:"-"` - Attributes map[string][]string `json:"-" bson:"-"` + UserID *string `json:"userid,omitempty"` + Username *string `json:"username,omitempty"` + EmailVerified *bool `json:"emailVerified,omitempty"` + TermsAccepted *string `json:"termsAccepted,omitempty"` + Roles *[]string `json:"roles,omitempty"` + Enabled bool `json:"-"` + Profile *Profile `json:"profile,omitempty"` + Attributes map[string][]string `json:"-"` } // TrustUser is the user object returned for the /v1/users/:userId/users route. @@ -133,7 +133,6 @@ func (u *User) Sanitize(details request.AuthDetails) error { u.EmailVerified = nil u.TermsAccepted = nil u.Roles = nil - u.PasswordExists = nil } return nil } @@ -147,8 +146,6 @@ func (u *User) Email() string { func (u *TrustUser) Sanitize(details request.AuthDetails) error { if details == nil || (!details.IsService() && details.UserID() != *u.UserID) { - // Note that a TrustUser includes some fields in the user that [User.Sanitize] wouldn't. - u.PasswordExists = nil if (u.TrustorPermissions == nil || len(*u.TrustorPermissions) == 0) && u.User.Profile != nil { u.User.Profile.Sanitize() } @@ -165,28 +162,6 @@ func (us TrustUserArray) Sanitize(details request.AuthDetails) error { return nil } -// IsClinic returns true if the user is legacy clinic Account -func (u *User) IsClinic() bool { - return u.HasRole(RoleClinic) -} - -func (u *User) IsCustodialAccount() bool { - return u.HasRole(RoleCustodialAccount) -} - -// IsClinician returns true if the user is a clinician -func (u *User) IsClinician() bool { - return u.HasRole(RoleClinician) -} - -func (u *User) AreTermsAccepted() bool { - if u.TermsAccepted == nil { - return false - } - _, err := TimestampToUnixString(*u.TermsAccepted) - return err == nil -} - type UserArray []*User func (u UserArray) Sanitize(details request.AuthDetails) error { @@ -218,8 +193,3 @@ func ValidateID(value string) error { } return nil } - -// IsValidUserID return true if the string is in a human readable uuid hex 8-4-4-4-12 format or legacy alphanumeric 10 characters -func IsValidUserID(id string) bool { - return idExpression.MatchString(id) -} diff --git a/user/user_accessor.go b/user/user_accessor.go index 067791e853..ce4d86a266 100644 --- a/user/user_accessor.go +++ b/user/user_accessor.go @@ -3,14 +3,6 @@ package user import ( "context" "errors" - - "github.com/Nerzal/gocloak/v13/pkg/jwx" -) - -const ( - serverRole = "backend_service" - - TimestampFormat = "2006-01-02T15:04:05-07:00" ) //go:generate mockgen -build_flags=--mod=mod -destination=./user_mock.go -package=user . ProfileAccessor,UserAccessor @@ -45,43 +37,10 @@ type RoleGetter interface { } // UserAccessor is the interface that can retrieve users. -// It is the equivalent of shoreline's shoreline's Storage -// interface, but for now will only retrieve user -// information. +// It is the equivalent of shoreline's Storage interface, +// but for now will only retrieve user information. type UserAccessor interface { ProfileAccessor RoleGetter - FindUser(ctx context.Context, user *User) (*User, error) - FindUserById(ctx context.Context, id string) (*User, error) - FindUsersWithIds(ctx context.Context, ids []string) ([]*User, error) -} - -type TokenIntrospectionResult struct { - Active bool `json:"active"` - Subject string `json:"sub"` - EmailVerified bool `json:"email_verified"` - ExpiresAt int64 `json:"eat"` - RealmAccess RealmAccess `json:"realm_access"` - IdentityProvider string `json:"identityProvider"` -} - -type AccessTokenCustomClaims struct { - jwx.Claims - IdentityProvider string `json:"identity_provider,omitempty"` -} - -type RealmAccess struct { - Roles []string `json:"roles"` -} - -func (t *TokenIntrospectionResult) IsServerToken() bool { - if len(t.RealmAccess.Roles) > 0 { - for _, role := range t.RealmAccess.Roles { - if role == serverRole { - return true - } - } - } - - return false + Client } diff --git a/user/user_mock.go b/user/user_mock.go index 66c45d47b8..6e98adb268 100644 --- a/user/user_mock.go +++ b/user/user_mock.go @@ -122,49 +122,19 @@ func (mr *MockUserAccessorMockRecorder) FindLegacyUserProfile(ctx, userID any) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindLegacyUserProfile", reflect.TypeOf((*MockUserAccessor)(nil).FindLegacyUserProfile), ctx, userID) } -// FindUser mocks base method. -func (m *MockUserAccessor) FindUser(ctx context.Context, user *User) (*User, error) { +// Get mocks base method. +func (m *MockUserAccessor) Get(ctx context.Context, id string) (*User, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "FindUser", ctx, user) + ret := m.ctrl.Call(m, "Get", ctx, id) ret0, _ := ret[0].(*User) ret1, _ := ret[1].(error) return ret0, ret1 } -// FindUser indicates an expected call of FindUser. -func (mr *MockUserAccessorMockRecorder) FindUser(ctx, user any) *gomock.Call { +// Get indicates an expected call of Get. +func (mr *MockUserAccessorMockRecorder) Get(ctx, id any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindUser", reflect.TypeOf((*MockUserAccessor)(nil).FindUser), ctx, user) -} - -// FindUserById mocks base method. -func (m *MockUserAccessor) FindUserById(ctx context.Context, id string) (*User, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "FindUserById", ctx, id) - ret0, _ := ret[0].(*User) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// FindUserById indicates an expected call of FindUserById. -func (mr *MockUserAccessorMockRecorder) FindUserById(ctx, id any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindUserById", reflect.TypeOf((*MockUserAccessor)(nil).FindUserById), ctx, id) -} - -// FindUsersWithIds mocks base method. -func (m *MockUserAccessor) FindUsersWithIds(ctx context.Context, ids []string) ([]*User, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "FindUsersWithIds", ctx, ids) - ret0, _ := ret[0].([]*User) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// FindUsersWithIds indicates an expected call of FindUsersWithIds. -func (mr *MockUserAccessorMockRecorder) FindUsersWithIds(ctx, ids any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindUsersWithIds", reflect.TypeOf((*MockUserAccessor)(nil).FindUsersWithIds), ctx, ids) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockUserAccessor)(nil).Get), ctx, id) } // Roles mocks base method. diff --git a/user/user_test.go b/user/user_test.go index ec067fb752..3e6bc7304c 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -42,7 +42,6 @@ var _ = Describe("User", func() { datum := userTest.RandomUser() mutator(datum) test.ExpectSerializedObjectJSON(datum, userTest.NewObjectFromUser(datum, test.ObjectFormatJSON)) - test.ExpectSerializedObjectBSON(datum, userTest.NewObjectFromUser(datum, test.ObjectFormatBSON)) }, Entry("succeeds", func(datum *user.User) {},