[BACK-2780] Add new user profiles endpoint. by lostlevels · Pull Request #698 · tidepool-org/platform

lostlevels · 2024-02-12T18:24:01Z

Since a lot of this is copy paste from shoreline, for reviewers:

~~shoreline/user/user.go => platform/user/full_user.go - (because there's already a type called User).~~ platform/user.go user.User extended with shoreline user fields.
~~shoreline/user/hasher.go => platform/user/hasher.go~~
shoreline/user/storage.go => platform/user/user_accessor.go (somewhat like the interface of the Storage repository, but simplified).
shoreline/user/migrationStore.go => platform/user/keycloak/user_accessor.go (Takes the logic around User creation / manipulation and removes the fallback / mongodb stuff), implements platform/user.UserAccessor interface
shoreline/keycloak/client.go => platform/user/keycloak/client.go (Wrapper around gocloak).

toddkazakov

Overall it looks good. I found few issues:

There is a lot of cruft that was copied from shoreline. I believe the majority of those functions are not used. There's no reason to keep unused code in platform, because it only leads to confusion.
There is probably a gap in the requirements that I discovered when I read the seagull code to try and figure out why the profile endpoints require custodian permissions. In order to fully deprecate and migrate existing seagull profiles from Mongo to Keycloak, we have to provide an alternative implementation of this functionality. For each user sharing their data with a user with a given id seagull fetches the user object from shoreline and merges it with the user profile attributes stored in the profile object in mongo. The response is sanitized depending on the actual permissions of the requesting user. This deserves its own ticket, but please do some research what the linked code does and then write the requirements. The alternative implementation should be much simpler, because the user account and profiles will be stored in a single service (Keycloak).

toddkazakov

Looks ok, but needs to be thoroughly tested. @lostlevels please open a new ticket for reimplementing the missing seagull endpoint I mentioned in my previous review.

toddkazakov · 2024-07-17T08:30:07Z

+	return slices.Contains(TRUES, value)
+}
+
+func parseUsersQuery(query url.Values) *usersProfileFilter {


My impression is that this query is not used at all. Can we check blip and uploader?

On checking blip and uploader it seems they are indeed no query string parameters used.

blip
platform-client
uploader
uploader

Can you confirm that no query string parameters are passed for getAssociatedUsersDetails @krystophv @gniezen @jh-bate ?

The blip and uploader calls are all piped through the platform-client function, so I believe you're correct - no query string params are being sent.

toddkazakov · 2024-07-18T16:35:56Z

+				profile = profile.ClearPatientInfo()
+			} else {
+				if trustorPerms.HasAny(permission.Custodian, permission.Read, permission.Write) {
+					// TODO: need to read seagull.value.settings - confirm this is actually used


Settings are not part of the profile object so probably there's no need to do anything?

I see the settings and preferences returned in the old seagull code, but don't know if they are actually used by clients. @krystophv @clintonium-119 @gniezen Do you know if the returned result of paltform-client getAssociatedUsersDetails ever uses the returned users' settings or preferences in blip or uploader?

toddkazakov · 2024-07-18T16:36:08Z

+					// TODO: need to read seagull.value.settings - confirm this is actually used
+				}
+				if trustorPerms.Has(permission.Custodian) {
+					// TODO: need to read seagull.value.preferences - confirm this is actually used


Preferences are not part of the profile object so probably there's no need to do anything?

lostlevels · 2025-06-24T21:59:01Z

/deploy qa1 auth

tidebot · 2025-06-24T21:59:05Z

lostlevels updated values.yaml file in qa1

tidebot · 2025-06-24T21:59:06Z

lostlevels updated flux policies file in qa1

tidebot · 2025-06-24T21:59:07Z

lostlevels deployed platform jimmy/BACK-2780-new-profiles-endpoint branch to qa1 namespace

sending as `{"patient":{"about": "..."}}`.

filtering.

Permissions.

…ofiles had them.

Updates

null fields work properly w/o changes.

Handle 404 case where profile failed to migrate / migration in progress in which case should return seagull profile. Refactor / remove some role-specific logic out of api layer. Fix panics when unable to get admin token. Fix bugs found during testing.

… seagull.

Tests that differentiate b/t trustor permissions and user vs service.

toddkazakov

The biggest issue are the looser permissions in /v1/users/:userId/users and the lack of sanitization in /v1/users/:userId/profile and /v1/users/legacy/:userId/profile. In addition to that, I think the code can be further cleaned up.

toddkazakov · 2026-06-05T10:08:37Z

+		if err != nil {
+			return err
+		}
+	}


You're missing a break here

toddkazakov · 2026-06-05T10:41:21Z

+	"golang.org/x/oauth2"
+
+	"github.com/tidepool-org/platform/pointer"
+	userLib "github.com/tidepool-org/platform/user"


This doesn't follow platform import conventions

toddkazakov · 2026-06-05T10:42:19Z

+)
+
+const (
+	tokenPrefix            = "kc"


This is not used

toddkazakov · 2026-06-05T10:42:23Z

+
+const (
+	tokenPrefix            = "kc"
+	tokenPartsSeparator    = ":"


This is not used

toddkazakov · 2026-06-05T10:51:51Z

+
+// 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 {
+	ok, _ := regexp.MatchString(`^([a-fA-F0-9]{10})$|^([a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12})$`, id)


Why is the regexp duplicated with the one above?

toddkazakov · 2026-06-08T08:58:53Z

+}
+
+// ClearPatientInfo makes a copy of up, clearing out certain patient information - this is called usually due to lack of permissions to the patient information
+func (up *UserProfile) ClearPatientInfo() *UserProfile {


Should the profile implement the standard Sanitize similar to other models?

toddkazakov · 2026-06-08T09:03:22Z

 	return cfg, err
 }

+func LoadConfigPrefix(prefix string) (*Config, error) {


Is this used? If not, let's delete it.

toddkazakov · 2026-06-08T09:06:42Z

+	GroupsForUser(ctx context.Context, granteeUserID string) (Permissions, error)
+	// UsersInGroup returns permissions that the user with id sharerID has shared with others, keyed by user id. It includes the user themself.
+	UsersInGroup(ctx context.Context, sharerID string) (Permissions, error)
+	HasMembershipRelationship(ctx context.Context, granteeUserID, grantorUserID string) (has bool, err error)


What do you think about renaming this to UsersHaveSharingRelationship? The implementation checks bi-directionally so grantee and grantor are incorrectly used

toddkazakov · 2026-06-08T09:10:27Z

 	GetUserPermissions(ctx context.Context, requestUserID string, targetUserID string) (Permissions, error)
 	UpdateUserPermissions(ctx context.Context, requestUserID string, targetUserID string, permissions Permissions) error
+	// GroupsForUser returns permissions that have been shared with granteeUserID. It is keyed by the user that has shared something with granteeUserID. It includes the user themself.
+	GroupsForUser(ctx context.Context, granteeUserID string) (Permissions, error)


What do you think about renaming this PermissionsGrantedToUser?

toddkazakov · 2026-06-08T09:10:37Z

+	// GroupsForUser returns permissions that have been shared with granteeUserID. It is keyed by the user that has shared something with granteeUserID. It includes the user themself.
+	GroupsForUser(ctx context.Context, granteeUserID string) (Permissions, error)
+	// UsersInGroup returns permissions that the user with id sharerID has shared with others, keyed by user id. It includes the user themself.
+	UsersInGroup(ctx context.Context, sharerID string) (Permissions, error)


What do you think about renaming this PermissionsGrantedByUser?

lostlevels force-pushed the jimmy/BACK-2780-new-profiles-endpoint branch from 5abaa66 to 7d32881 Compare March 27, 2024 20:54

toddkazakov reviewed Apr 19, 2024

View reviewed changes

Comment thread user/profile.go Outdated

toddkazakov reviewed Apr 19, 2024

View reviewed changes

Comment thread user/profile.go Outdated

toddkazakov requested changes Apr 25, 2024

View reviewed changes

toddkazakov reviewed Apr 25, 2024

View reviewed changes

Comment thread user/profile.go

toddkazakov previously approved these changes Jun 4, 2024

View reviewed changes

lostlevels dismissed toddkazakov’s stale review via d3f829a June 5, 2024 17:28

toddkazakov reviewed Jul 17, 2024

View reviewed changes

Comment thread auth/service/api/v1/profile.go Outdated

toddkazakov reviewed Jul 17, 2024

View reviewed changes

toddkazakov reviewed Jul 18, 2024

View reviewed changes

lostlevels force-pushed the jimmy/BACK-2780-new-profiles-endpoint branch from 6d1b1c7 to 7d53e78 Compare August 13, 2025 20:40

lostlevels force-pushed the jimmy/BACK-2780-new-profiles-endpoint branch from 8337700 to 6fd7649 Compare April 14, 2026 09:10

lostlevels added 13 commits April 16, 2026 07:03

Refactoring and adding shoreline models to platform/user for auth.

81964c7

Cleanup.

6cfa1b2

Add /v1/profiles/:userId route.

5e4e602

Renaming.

6135115

Add the route.

d159e8c

Fix build.

ee311c4

Fix test.

962ebc1

Use permissions like in seagull.

a746294

Validate the profile.

de33718

HasWritePermissions.

52fb052

Rename profile routes to be consistent w/ existing ones.

51706a3

Use snakecase attributes for now but don't flatten yet as blip is still

93222a3

sending as `{"patient":{"about": "..."}}`.

Have a LegacyUserProfile to support seagull requests.

1c6eba9

lostlevels added 25 commits April 16, 2026 07:56

Add clinic profile fields.

742c06a

Add normalizer methods for profiles.

8378b34

Account for empty profile fullName.

ad31dcd

[BACK-3046] Create initial shared users with profiles path w/o

3265f74

filtering.

Start metadata/users/:userid/users filter params.

3fca9bb

Parse users profiles query filter.

9a27e5b

Update users route to properly filter out users. Document Permission /

1ceb4b9

Permissions.

Remove unused query filter on users profiles.

62db0a4

Handle email and emails in legacy seagull profiles.

d58642c

Read raw value as map from seagull value.

e1de168

Allow setting of profile on seagull document's value field.

9fc1750

Migrate diagnosisType.

94d0a00

Remove email field from clinic as confirmed only a few fake clinic pr…

a79c2cd

…ofiles had them.

Use correct FullName in case of fake children.

20e6964

Handle certain incorrect types in legacy seagull profile.

374d117

Fix some logic and tests for profiles.

9eb119e

Updates

Set max profile field length to equal keycloak < 24

d643c07

Make some profile values pointers so that some legacy migration profiles

8737671

null fields work properly w/o changes.

Export MaxProfileFieldLen

32682d6

Remove unused field, add tests, synchronize keycloak access.

d11dc0a

Use existing UsersArray type, add update tests.

871f707

Make TargetTimezone a value.

8c775e5

Handle 404 case where profile failed to migrate / migration in progress in which case should return seagull profile. Refactor / remove some role-specific logic out of api layer. Fix panics when unable to get admin token. Fix bugs found during testing.

Remove unused files, remove unneded checks.

bc4b082

Fix tests broken during rebase.

e425e62

Regenerate.

97daee8

lostlevels force-pushed the jimmy/BACK-2780-new-profiles-endpoint branch from 6fd7649 to 97daee8 Compare April 20, 2026 14:15

lostlevels added 3 commits April 29, 2026 08:21

Add same named routes as seagull for clients that connect directly to…

8dbf5a7

… seagull.

Fix shadowed variable profile overwriting update.

f91d213

Add /v1/users/:userId/profile route tests.

b4d70a9

Tests that differentiate b/t trustor permissions and user vs service.

toddkazakov requested changes Jun 8, 2026

View reviewed changes

Uh oh!

Conversation

lostlevels commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddkazakov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddkazakov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toddkazakov Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostlevels commented Jun 24, 2025

Uh oh!

tidebot commented Jun 24, 2025

Uh oh!

tidebot commented Jun 24, 2025

Uh oh!

tidebot commented Jun 24, 2025

Uh oh!

toddkazakov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lostlevels commented Feb 12, 2024 •

edited

Loading

toddkazakov Jul 17, 2024 •

edited

Loading