[BACK-2780] Add new user profiles endpoint. - #698
Conversation
5abaa66 to
7d32881
Compare
toddkazakov
left a comment
toddkazakov
left a comment
There was a problem hiding this comment.
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
left a comment
toddkazakov
left a comment
There was a problem hiding this comment.
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.
| return slices.Contains(TRUES, value) | ||
| } | ||
|
|
||
| func parseUsersQuery(query url.Values) *usersProfileFilter { |
There was a problem hiding this comment.
My impression is that this query is not used at all. Can we check blip and uploader?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
Settings are not part of the profile object so probably there's no need to do anything?
There was a problem hiding this comment.
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?
| // 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 |
There was a problem hiding this comment.
Preferences are not part of the profile object so probably there's no need to do anything?
|
/deploy qa1 auth |
|
lostlevels updated values.yaml file in qa1 |
|
lostlevels updated flux policies file in qa1 |
|
lostlevels deployed platform jimmy/BACK-2780-new-profiles-endpoint branch to qa1 namespace |
6d1b1c7 to
7d53e78
Compare
8337700 to
6fd7649
Compare
sending as `{"patient":{"about": "..."}}`.
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.
6fd7649 to
97daee8
Compare
Tests that differentiate b/t trustor permissions and user vs service.
toddkazakov
left a comment
toddkazakov
left a comment
There was a problem hiding this comment.
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
left a comment
toddkazakov
left a comment
There was a problem hiding this comment.
There's a new bug when merging the existing user attributes with the updated profile attributes, which makes it impossible to remove an attribute from the user profile.
I feel that the code should be cleaned up further - there's a lot of unused cruft that has been copied over from shoreline.
| } | ||
|
|
||
| attrs := map[string][]string{} | ||
| maps.Copy(attrs, user.Attributes) |
There was a problem hiding this comment.
I know this was added as a result of my previous review where I flagged an issue that removes non-profile attributes from the keycloak user, but this introduces a new bug. Profile fields cannot be removed from the user profile because only non-empty attributes are added to the map in user.Profile.ToAttributes().
| @@ -0,0 +1,30 @@ | |||
| package user | |||
There was a problem hiding this comment.
Can this file be moved to the keycloak package?
| } | ||
| if up.Custodian != nil && up.Custodian.FullName != "" { | ||
| addAttribute(attributes, "custodian_full_name", up.Custodian.FullName) | ||
| // The "has_custodian" attribute is only added so that filtering on users is simpler via the keycloak API - because |
There was a problem hiding this comment.
Instead of using a boolean attribute can we just add the custodial role to the account instead?
| @@ -0,0 +1,114 @@ | |||
| package mongo | |||
There was a problem hiding this comment.
This file seems misplaced - should it be in the user package?
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds profile models and migration handling across Keycloak and Seagull, permission-based authorization, profile API routes, service wiring, and generated test mocks. ChangesProfile migration and authorization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProfileRouter
participant PermissionClient
participant FallbackLegacyUserAccessor
participant Keycloak
participant Seagull
Client->>ProfileRouter: Request profile
ProfileRouter->>PermissionClient: Check sharing or custodian access
PermissionClient-->>ProfileRouter: Permission result
ProfileRouter->>FallbackLegacyUserAccessor: Find or update profile
FallbackLegacyUserAccessor->>Seagull: Read legacy profile
Seagull-->>FallbackLegacyUserAccessor: Profile and migration status
FallbackLegacyUserAccessor->>Keycloak: Read or update migrated profile
Keycloak-->>FallbackLegacyUserAccessor: Profile result
FallbackLegacyUserAccessor-->>ProfileRouter: Profile result
ProfileRouter-->>Client: HTTP response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
user/test/user.go-74-81 (1)
74-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert
Profilein user test helpers.
RandomUserandNewObjectFromUseromitProfile, andMatchUserignores it. Tests using these helpers can pass when profile data is lost during parsing or client reads. Add profile fixtures, serialization, and an explicitProfilematcher.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/test/user.go` around lines 74 - 81, Update the RandomUser and NewObjectFromUser helpers to populate and serialize the Profile field, then extend MatchUser with an explicit Profile matcher alongside the existing fields. Ensure the fixture, object conversion, and comparison all preserve and validate profile data.auth/service/api/v1/profile.go-214-218 (1)
214-218: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReturn the persisted profile from both update handlers.
UpdateUserretains existing Keycloak attributes and overlays only non-empty request fields. Omitted fields remain stored, but both handlers return the request profile. Reload the profile before responding or return the persisted value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/service/api/v1/profile.go` around lines 214 - 218, Update both profile update handlers, including the one calling UpdateLegacyUserProfile and its counterpart, to respond with the persisted profile rather than the request object. Reload the profile after a successful update, or use the update operation’s persisted return value, and pass that value to responder.Data while preserving existing error handling.user/profile.go-414-421 (1)
414-421: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the
addAttributesreturn value and drop the unused helper.
addAttributesassignsokin the loop and then returns the literaltrue, so the result never reflects whether anything was added. golangci-lint reports the ineffectual assignment at line 417. golangci-lint also reportscontainsAnyAttributeKeysas unused at line 432.🐛 Proposed fix
func addAttributes(attributes map[string][]string, attribute string, values ...string) (ok bool) { for _, value := range values { if addAttribute(attributes, attribute, value) { ok = true } } - return true + return ok }Remove
containsAnyAttributeKeysif no caller exists:#!/bin/bash rg -n '\bcontainsAnyAttributeKeys\s*\(' --type=goAlso applies to: 432-439
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile.go` around lines 414 - 421, Update addAttributes to return the accumulated ok value after processing all values, preserving false when no call to addAttribute succeeds. Remove the unused containsAnyAttributeKeys helper if no callers exist.Source: Linters/SAST tools
user/timeutil.go-22-30 (1)
22-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFormat in UTC to make the output deterministic.
time.Unix(i, 0)returns atime.Timein the process local zone. The formatted offset therefore depends on the containerTZsetting, so the same input yields different stored strings across environments.🐛 Proposed fix
- t := time.Unix(i, 0) + t := time.Unix(i, 0).UTC() timestamp = t.Format(TimestampFormat)Also prefer
strconv.FormatInt(parsed.Unix(), 10)overfmt.Sprintf("%v", ...)at line 18, and use explicit returns instead of naked returns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/timeutil.go` around lines 22 - 30, Update UnixStringToTimestamp to format the time in UTC so identical Unix inputs produce deterministic output regardless of the process time zone; replace naked returns with explicit return values, and in the related timestamp conversion use strconv.FormatInt(parsed.Unix(), 10) instead of fmt.Sprintf.user/user_accessor.go-58-65 (1)
58-65: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winChange the
ExpiresAtJSON tag toexp. The current Keycloak path sets this field from decoded claims, so the typo does not affect the current flow. It still breaks JSON serialization and deserialization of this standard claim.Suggested change
- ExpiresAt int64 `json:"eat"` + ExpiresAt int64 `json:"exp"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/user_accessor.go` around lines 58 - 65, Update the ExpiresAt field in TokenIntrospectionResult to use the standard JSON tag exp instead of eat, preserving the existing field type and behavior.user/profile.go-441-447 (1)
441-447: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport date errors on the scoped reference.
v.String("date", ...)produces/birthday/date, but the JSON field is/birthday. Do not usev.String("", ...); it produces/birthday/. Parse the value directly and callv.ReportError(...)withstructureValidator.ErrorValueStringAsTimeNotValid(...)so the error stays on/birthday.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile.go` around lines 441 - 447, Update Date.Validate to parse the non-empty date value directly instead of calling v.String("date", ...), then report parsing failures with v.ReportError using structureValidator.ErrorValueStringAsTimeNotValid(...). Keep the error reference scoped to the Date field itself, avoiding both the "date" child path and an empty-name trailing slash.env.sh-77-83 (1)
77-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe new
SEAGULL_TIDEPOOL_STORE_*configuration is inconsistent across the environment scripts.Config.LoadPrefix("SEAGULL")introduces a second Mongo configuration set, but the two scripts define it differently:env.shsets credentials that the local Mongo instance probably does not require, andenv.test.shomits the block entirely.
env.sh#L77-L83: removeSEAGULL_TIDEPOOL_STORE_USERNAME,SEAGULL_TIDEPOOL_STORE_PASSWORD, and theauthSource=adminoption so the Seagull store matches the unauthenticated local Mongo setup at lines 3-5, or document the required local Mongo user.env.test.sh#L20-L31: add theSEAGULL_TIDEPOOL_STORE_*block with test values (for exampleSEAGULL_TIDEPOOL_STORE_DATABASE="seagull_test") so tests that build the legacy Seagull repository can load their configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@env.sh` around lines 77 - 83, The Seagull Tidepool store environment configuration is inconsistent between scripts. In env.sh lines 77-83, remove the username, password, and authSource settings so it matches the unauthenticated local Mongo configuration; in env.test.sh lines 20-31, add the complete SEAGULL_TIDEPOOL_STORE_* block with test values, including a test database such as seagull_test.user/keycloak/client.go-242-280 (1)
242-280: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
customClaims.ExpiresAtbefore callingUnix().jwt.RegisteredClaims.ExpiresAtis a pointer, andexpis optional during decoding. A token withoutexpleaves this field nil and can panic this request path. Return an error or define a value for missing expiration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/keycloak/client.go` around lines 242 - 280, In IntrospectToken, validate customClaims.ExpiresAt after DecodeAccessTokenCustomClaims succeeds and before calling Unix(). Handle a nil expiration explicitly by returning an error or applying the established missing-expiration value, while preserving the existing result mapping for tokens with exp.
🧹 Nitpick comments (18)
auth/service/api/v1/profile.go (1)
144-187: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe order of
resultsis nondeterministic.The goroutines append to
resultsunderlockin completion order. Two identical requests can return the same users in a different order. Clients that diff or cache the response see spurious changes, and tests with more than one shared user become order-dependent.Sort
resultsby user ID before responding.♻️ Proposed change
if err := group.Wait(); err != nil { r.handleUserOrProfileErr(responder, err) return } + slices.SortFunc(results, func(a, b *user.TrustUser) int { + return strings.Compare(pointer.ToString(a.UserID), pointer.ToString(b.UserID)) + }) + // type TrustUserArray implements Sanitize to hide any properties for non service requests responder.Data(http.StatusOK, results)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/service/api/v1/profile.go` around lines 144 - 187, Sort results by user ID after group.Wait succeeds and before responder.Data returns the response. Update the flow around the results accumulation in the errgroup block, preserving the existing concurrent collection and error handling while ensuring deterministic ordering for identical requests.auth/service/api/v1/permission.go (1)
20-47: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winInvert the guard and extract the shared authorization logic.
Two concerns in
requireCustodian, andrequireMembershipat Lines 57-84 repeats both.
The body is wrapped in
if handlerFunc != nil && res != nil && req != nil. If that condition is false, the middleware writes nothing and returns. go-json-rest then completes the request with an empty200 OK. An authorization middleware that returns success on a wiring mistake is a fail-open path. The current call sites inprofile.goalways pass a non-nil method value, so this is not reachable today. Guard against future wiring changes by failing loudly instead.The two functions are identical except for the permission call and the doc comment. Extract one helper that takes the permission check as a parameter.
Line 24 also creates
responder, then Line 28 creates a second responder for the same request. Reuseresponder.♻️ Proposed refactor
+type permissionCheck func(ctx context.Context, granteeUserID, grantorUserID string) (bool, error) + +func (r *Router) requireRelationship(targetParamUserID string, check permissionCheck, handlerFunc rest.HandlerFunc) rest.HandlerFunc { + return func(res rest.ResponseWriter, req *rest.Request) { + if handlerFunc == nil || res == nil || req == nil { + panic("auth middleware configured with nil handler, response writer, or request") + } + targetUserID := req.PathParam(targetParamUserID) + responder := request.MustNewResponder(res, req) + ctx := req.Context() + details := request.GetAuthDetails(ctx) + if details == nil { + responder.Error(http.StatusUnauthorized, request.ErrorUnauthenticated()) + return + } + if details.IsService() || details.UserID() == targetUserID { + handlerFunc(res, req) + return + } + hasPerms, err := check(ctx, details.UserID(), targetUserID) + if err != nil { + responder.InternalServerError(err) + return + } + if !hasPerms { + responder.Empty(http.StatusForbidden) + return + } + handlerFunc(res, req) + } +} + +func (r *Router) requireCustodian(targetParamUserID string, handlerFunc rest.HandlerFunc) rest.HandlerFunc { + return r.requireRelationship(targetParamUserID, r.PermissionsClient().HasCustodianPermissions, handlerFunc) +} + +func (r *Router) requireMembership(targetParamUserID string, handlerFunc rest.HandlerFunc) rest.HandlerFunc { + return r.requireRelationship(targetParamUserID, r.PermissionsClient().UsersHaveSharingRelationship, handlerFunc) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/service/api/v1/permission.go` around lines 20 - 47, Refactor requireCustodian and requireMembership to share one authorization middleware helper that accepts the appropriate permission-check function, preserving their existing authorization behavior. In the helper, invert the nil guard so invalid handlerFunc, res, or req inputs fail loudly rather than silently returning a successful response; reuse the responder created before the authentication check instead of constructing a second one.data/service/api/v1/mocks/mocks.go (1)
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe four new
permission.Clientmethods were added as fixed-value stubs in every hand-written test double. Each returnsnil, nilorfalse, nil. Tests that route authorization through these doubles always observe "no permissions" and "no relationship", so a test can pass for the wrong reason and no test can drive an error path.
data/service/api/v1/mocks/mocks.go#L89-L104: returnp.Errorandp.Defaultin the new methods, matchingGetUserPermissionsat Line 75, and rename the receiver fromctop.auth/test/client.go#L37-L51: record inputs and drain configurable outputs in the four new methods, matching the accessor pattern used elsewhere in the package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data/service/api/v1/mocks/mocks.go` around lines 89 - 104, The new permission.Client methods are fixed-value stubs in two hand-written test doubles. In data/service/api/v1/mocks/mocks.go lines 89-104, update PermissionsGrantedToUser, PermissionsGrantedByUser, UsersHaveSharingRelationship, and HasCustodianPermissions to use receiver p, returning p.Error and p.Default consistently with GetUserPermissions. In auth/test/client.go lines 37-51, update the same four methods to record their inputs and drain configurable outputs following the package’s existing accessor pattern.user/user_accessor.go (2)
76-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify with
slices.Contains.The length guard is redundant, and the package already uses
slices.Containsinuser.go.♻️ Proposed refactor
func (t *TokenIntrospectionResult) IsServerToken() bool { - if len(t.RealmAccess.Roles) > 0 { - for _, role := range t.RealmAccess.Roles { - if role == serverRole { - return true - } - } - } - - return false + return slices.Contains(t.RealmAccess.Roles, serverRole) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/user_accessor.go` around lines 76 - 86, Update TokenIntrospectionResult.IsServerToken to use slices.Contains directly on t.RealmAccess.Roles, removing the redundant length guard and manual loop while preserving the existing boolean result.
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the exported role constants in
ShorelineManagedRoles.The map uses raw literals while
user.godefinesRolePatient,RoleClinic,RoleClinician, andRoleCustodialAccount. The literals can drift from the constants.♻️ Proposed refactor
- ShorelineManagedRoles = map[string]struct{}{"patient": {}, "clinic": {}, "clinician": {}, "custodial_account": {}} + ShorelineManagedRoles = map[string]struct{}{ + RolePatient: {}, + RoleClinic: {}, + RoleClinician: {}, + RoleCustodialAccount: {}, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/user_accessor.go` around lines 18 - 19, Update ShorelineManagedRoles to use the exported constants RolePatient, RoleClinic, RoleClinician, and RoleCustodialAccount from user.go as its keys instead of raw role-name literals, preserving the existing managed-role set.user/profile.go (5)
151-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
Clinicassignment.Line 154 assigns the legacy pointer directly. Lines 180-187 then replace it with a deep clone whenever
p.Clinic != nil, and it is nil otherwise. Dropping line 154 removes the momentary aliasing of the legacy struct.♻️ Proposed refactor
up := &Profile{ FullName: p.FullName, - Clinic: p.Clinic, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile.go` around lines 151 - 155, Remove the direct Clinic field assignment from LegacyUserProfile.ToUserProfile. Leave the later deep-clone handling for p.Clinic != nil and its nil behavior unchanged, so the returned Profile never temporarily aliases the legacy clinic data.
141-147: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake the
Patientnon-nil invariant explicit.Lines 143 and 146 dereference
legacyProfile.Patient. That field is only set inside theIsPatientProfilebranch. The code is safe today only becausehasPatientFields()returns true whenCustodian != nil. If that helper changes, this panics. Allocate the struct locally instead of relying on the remote invariant.🛡️ Proposed defensive change
if up.Custodian != nil { + if legacyProfile.Patient == nil { + legacyProfile.Patient = &LegacyPatientProfile{} + } legacyProfile.Patient.IsOtherPerson = true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile.go` around lines 141 - 147, In the Custodian handling block of the profile conversion function, ensure legacyProfile.Patient is initialized locally before assigning IsOtherPerson and FullName, rather than relying on hasPatientFields() or IsPatientProfile to have created it. Preserve the existing FullName selection behavior.
423-430: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndex the map instead of scanning all keys.
containsAttributewalks every key to find one.addAttributecalls it for each value, so building attributes for a profile scans the map repeatedly.♻️ Proposed refactor
func containsAttribute(attributes map[string][]string, attribute, value string) bool { - for key, vals := range attributes { - if key == attribute && slices.Contains(vals, value) { - return true - } - } - return false + return slices.Contains(attributes[attribute], value) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile.go` around lines 423 - 430, Update containsAttribute to directly retrieve attributes[attribute] and check whether that value slice contains value, removing the loop over all map keys while preserving the existing boolean result for missing attributes.
471-484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Profile.NormalizeskipsTargetDevices.Every other string field is trimmed.
TargetDevicesentries pass through untrimmed and reach Keycloak attributes throughToAttributes. Trim them for consistency.♻️ Proposed refactor
up.BiologicalSex = strings.TrimSpace(up.BiologicalSex) + for i := range up.TargetDevices { + up.TargetDevices[i] = strings.TrimSpace(up.TargetDevices[i]) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile.go` around lines 471 - 484, Update Profile.Normalize to trim whitespace from every TargetDevices entry before ToAttributes consumes them, while preserving the existing normalization of the other profile fields and nested values.
317-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
usernameparameter.
ProfileFromAttributesnever readsusername. Removing it prevents callers from assuming the username affects the result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile.go` at line 317, Remove the unused username parameter from ProfileFromAttributes and update every call site to pass only attributes and roles, preserving the function’s existing result behavior.user/legacy_raw_seagull_profile.go (1)
122-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWiden
dstto match the function name.The doc comment describes a generic round-trip helper, but the signature only accepts
*LegacyUserProfile. Changedsttoany(or add a type parameter) so the name matches the behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/legacy_raw_seagull_profile.go` around lines 122 - 131, Update MarshalThenUnmarshal to accept dst as any instead of *LegacyUserProfile, while preserving its existing JSON marshal and unmarshal flow so it supports arbitrary destination types described by the function name.user/profile_test.go (2)
13-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a malformed existing value.
The spec covers the happy path only. Add a case where
seagullValueBeforeis not valid JSON, and a case where it is the empty string. Those inputs drive the branch inAddProfileToSeagullValuethat discards the existing content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile_test.go` around lines 13 - 38, Extend the AddProfileToSeagullValue test context with cases for malformed JSON and an empty seagullValueBefore string. Verify both inputs discard the existing content and produce the expected profile value without errors, covering the fallback branch while preserving the existing happy-path test.
42-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage to the reverse conversion and the attribute mapping.
The table covers
ToLegacyProfileonly. The migration contract also depends onToUserProfile,ToAttributes, andProfileFromAttributes. Add a round-trip case (Profile -> LegacyUserProfile -> ProfileandProfile -> attributes -> Profile) and a case for a clinician with no clinic fields, which must produce a non-nil emptyClinicobject per the comment inuser/profile.golines 119-126.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/profile_test.go` around lines 42 - 110, Extend the profile conversion tests beyond ToLegacyProfile by adding round-trip coverage through ToUserProfile and through ToAttributes/ProfileFromAttributes, using representative patient data and asserting the reconstructed profiles. Add a clinician case with no clinic fields and verify the resulting profile contains a non-nil empty Clinic object, as required by the Profile conversion contract.auth/store/mongo/legacy_seagull_profile_repository.go (1)
24-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe Mongo store has no shutdown path, and
EnsureIndexesdoes nothing.
NewLegacySeagullProfileRepositorycreates a store at line 29 and keeps only the repository. Nothing retains the store, so no caller can terminate the client and release its connection pool.
EnsureIndexesreturnsnil. TheuserIdlookups in this repository need an index, and the conditional-update fix noted above needs a unique index onuserId.Keep the store on the struct and expose a
Terminatemethod. Create theuserIdindex inEnsureIndexes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/store/mongo/legacy_seagull_profile_repository.go` around lines 24 - 40, Update LegacySeagullProfileRepository to retain the store returned by NewStore, add a Terminate method that shuts it down, and make EnsureIndexes create the required unique userId index for lookups and conditional updates.go.mod (1)
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resty/v2is marked indirect but the code imports it directly.
user/keycloak/client.goimportsgithub.com/go-resty/resty/v2at line 14. The module belongs in the direct require block without the// indirectcomment. Rungo mod tidyto correct the classification.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` at line 69, Move github.com/go-resty/resty/v2 from the indirect dependency block into the direct require block in go.mod, remove the // indirect annotation, and run go mod tidy to normalize the module requirements.user/fallback_user_accessor.go (2)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
roleGetteris never read.The struct stores
roleGetterat line 18 and the constructor sets it at line 25, but no method in this file uses it. Remove the field and the constructor parameter, or use it. Removal changes the constructor signature, whichauth/service/service/service.gocalls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/fallback_user_accessor.go` around lines 15 - 27, Remove the unused roleGetter field from FallbackLegacyUserAccessor and remove the corresponding parameter and assignment from NewFallbackLegacyUserAccessor; update the constructor call in service/service.go to match the reduced signature.
62-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe retry loop sleeps after the last attempt and ignores context cancellation.
Two problems exist:
- When the third attempt returns
ErrUserProfileMigrationInProgress, the loop sleeps for three seconds and then exits. That sleep adds latency to the request thread and changes nothing.time.Sleepdoes not observectx. When the client disconnects or the request deadline passes, this call still blocks for up to six seconds in total.♻️ Proposed fix
arbritraryRetryLimit := 3 var err error for i := range arbritraryRetryLimit { err = f.upsertLegacyUserProfile(ctx, id, profile) if errors.Is(err, ErrUserProfileMigrationInProgress) { + if i == arbritraryRetryLimit-1 { + break + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(time.Second * time.Duration(i+1)): + } continue } if err != nil { return err } break } return err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/fallback_user_accessor.go` around lines 62 - 75, Update the retry loop around upsertLegacyUserProfile so it only waits when another attempt remains, and replace time.Sleep with a context-aware wait using ctx. Preserve immediate returns for non-migration errors and return the final migration error after the retry limit is exhausted, while propagating context cancellation promptly.user/keycloak/user_accessor.go (1)
54-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
UpdateLegacyUserProfilemutates the caller's profile.Line 60 sets
p.Clinic = nilon the pointer that the caller owns. The caller keeps that modified value after the call and may reuse it for a response body or a retry.FallbackLegacyUserAccessor.UpdateLegacyUserProfileretries the same pointer up to three times, so the clinic data is already gone on later attempts.Copy the profile before you clear the clinic field.
♻️ Proposed refactor
if !user.HasClinicOrClinicianRole(roles) && p.Clinic != nil { - p.Clinic = nil + clone := *p + clone.Clinic = nil + p = &clone }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@user/keycloak/user_accessor.go` around lines 54 - 63, Update UpdateLegacyUserProfile to copy the supplied LegacyUserProfile before applying the role-based Clinic clearing, and pass the copy to ToUserProfile. Preserve the original p value so callers and retry logic retain the clinic data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/service/api/v1/profile.go`:
- Around line 152-165: In the shared-user and profile lookup flow, update the
error handling around UserAccessor().Get and r.getProfile so non-sentinel errors
are propagated before checking nil results. Preserve the user.ErrUserNotFound
and user.ErrUserProfileNotFound cases as successful missing-resource handling,
then apply the nil-result checks only after those errors have been handled.
In `@auth/service/api/v1/router_test.go`:
- Around line 592-598: Update the test fixture sanitizedUserDetails and the
limited user-info test around the sharee cases so sanitization is actually
validated: use a profile with Birthday, DiagnosisDate, and MRN redacted while
preserving the expected non-sensitive fields and permissions. Keep the full
user-info test expecting userDetails, and ensure the limited test compares
against the redacted sanitizedUserDetails.
In `@auth/store/mongo/legacy_seagull_profile_repository.go`:
- Around line 77-113: Make the read-modify-write flow around the repository
method’s initial FindOne/Decode and subsequent FindOneAndUpdate conditional on
the previously read value (and preserve the no-document upsert case safely).
Prevent stale writes from overwriting concurrent migrations or profile updates
by adding the prior value or equivalent migration-status guard to the update
filter, and avoid allowing a non-matching upsert to create duplicates. Treat a
failed conditional update or duplicate-key conflict as a retryable conflict, and
remove reliance on the post-write IsMigrating check to undo changes.
In `@go.mod`:
- Line 72: Update the pinned github.com/golang-jwt/jwt/v5 requirement in go.mod
from v5.0.0 to the first version patched for GO-2025-3553/GHSA-mh63-6h87-95cp,
and retain an explicit version pin. Refresh the corresponding module checksums
or dependency metadata as needed.
In `@permission/client/client.go`:
- Around line 107-131: Update GetUserPermissions and the permission checks in
UsersHaveSharingRelationship and HasCustodianPermissions so an absent access
record is treated as an empty permission set while genuine permission-service
failures still propagate. Ensure reverse-direction checks continue after a
missing forward record and unrelated users reach the existing HTTP 403 path; add
coverage for a reverse-only relationship and the unrelated-user response.
In `@store/structured/mongo/config.go`:
- Around line 74-80: Update Config.LoadPrefix so Seagull-specific loading cannot
fall back to unprefixed TIDEPOOL_STORE_DATABASE when the SEAGULL-prefixed
variable is absent. Use a prefixed-only envconfig loading path or explicitly
validate that the required prefixed database variable is set before returning
success, while preserving the existing Load-to-LoadPrefix delegation.
In `@user/keycloak/client.go`:
- Around line 302-313: Update getAdminToken to acquire the admin-token write
lock before refreshing, re-check adminTokenIsExpired while holding that lock,
and only call loginAsAdmin when the token remains expired; preserve read-locked
access for valid tokens. Add a nil check in loginAsAdmin after jwtToAccessToken,
returning an error for an empty token, and ensure getAdminToken never
dereferences c.adminToken when it is nil.
- Around line 282-295: Update DeleteUserSessions so every non-404 error returned
by LogoutAllSessions is propagated instead of returning the earlier token error
value. Preserve the existing nil-success behavior for APIError responses with
http.StatusNotFound, and return nil only when logout succeeds or the error is
explicitly treated as not found; avoid shadowing the outer err or return the
logout error directly.
In `@user/legacy_raw_seagull_profile.go`:
- Around line 108-120: Update AddProfileToSeagullValue and SetRawValueProfile so
extractSeagullValue errors are returned when the existing value is non-empty,
preserving the original data instead of replacing it with an empty object; only
initialize a new object for an empty value, and reuse AddProfileToSeagullValue
from SetRawValueProfile where appropriate.
- Around line 133-148: Update FallbackLegacyUserAccessor.upsertLegacyUserProfile
to route MigrationUnmigrated, MigrationInProgress, and MigrationError profiles
through the intended migration/retry handling instead of sending only unmigrated
profiles to Seagull. Update LegacySeagullDocument.MigrationStatus to explicitly
handle MigrationEnd being set without MigrationStart, rejecting it or routing it
to an appropriate non-Seagull status so reads and writes do not remain on
Seagull. Do not rely on changing IsMigrating alone.
In `@user/profile.go`:
- Around line 456-469: Update Profile.Validate to apply MaxProfileFieldLen
validation to all Clinic string fields written by ToAttributes: clinic_name,
clinic_role, clinic_telephone, and clinic_npi. Update
LegacyPatientProfile.Validate to add the same length validation for
biologicalSex, matching Profile.Validate and its normalization behavior.
In `@user/timeutil.go`:
- Around line 9-20: Update ParseTimestamp to parse timestamps with time.RFC3339
so valid UTC “Z” timestamps are accepted, while preserving
TimestampToUnixString’s existing conversion behavior. Add coverage for an RFC
3339 input ending in “Z” if tests are available.
In `@user/user.go`:
- Around line 143-151: Update the condition in TrustUser.Sanitize to check
u.UserID for nil before dereferencing it, treating a nil UserID as not the
requesting user while preserving the existing service-user and matching-ID
behavior.
- Around line 40-41: Update custodialAccountRegexp to anchor the pattern at both
the beginning and end of the string, so MatchString only accepts the complete
unclaimed-custodial address and rejects addresses with surrounding or trailing
content.
---
Minor comments:
In `@auth/service/api/v1/profile.go`:
- Around line 214-218: Update both profile update handlers, including the one
calling UpdateLegacyUserProfile and its counterpart, to respond with the
persisted profile rather than the request object. Reload the profile after a
successful update, or use the update operation’s persisted return value, and
pass that value to responder.Data while preserving existing error handling.
In `@env.sh`:
- Around line 77-83: The Seagull Tidepool store environment configuration is
inconsistent between scripts. In env.sh lines 77-83, remove the username,
password, and authSource settings so it matches the unauthenticated local Mongo
configuration; in env.test.sh lines 20-31, add the complete
SEAGULL_TIDEPOOL_STORE_* block with test values, including a test database such
as seagull_test.
In `@user/keycloak/client.go`:
- Around line 242-280: In IntrospectToken, validate customClaims.ExpiresAt after
DecodeAccessTokenCustomClaims succeeds and before calling Unix(). Handle a nil
expiration explicitly by returning an error or applying the established
missing-expiration value, while preserving the existing result mapping for
tokens with exp.
In `@user/profile.go`:
- Around line 414-421: Update addAttributes to return the accumulated ok value
after processing all values, preserving false when no call to addAttribute
succeeds. Remove the unused containsAnyAttributeKeys helper if no callers exist.
- Around line 441-447: Update Date.Validate to parse the non-empty date value
directly instead of calling v.String("date", ...), then report parsing failures
with v.ReportError using structureValidator.ErrorValueStringAsTimeNotValid(...).
Keep the error reference scoped to the Date field itself, avoiding both the
"date" child path and an empty-name trailing slash.
In `@user/test/user.go`:
- Around line 74-81: Update the RandomUser and NewObjectFromUser helpers to
populate and serialize the Profile field, then extend MatchUser with an explicit
Profile matcher alongside the existing fields. Ensure the fixture, object
conversion, and comparison all preserve and validate profile data.
In `@user/timeutil.go`:
- Around line 22-30: Update UnixStringToTimestamp to format the time in UTC so
identical Unix inputs produce deterministic output regardless of the process
time zone; replace naked returns with explicit return values, and in the related
timestamp conversion use strconv.FormatInt(parsed.Unix(), 10) instead of
fmt.Sprintf.
In `@user/user_accessor.go`:
- Around line 58-65: Update the ExpiresAt field in TokenIntrospectionResult to
use the standard JSON tag exp instead of eat, preserving the existing field type
and behavior.
---
Nitpick comments:
In `@auth/service/api/v1/permission.go`:
- Around line 20-47: Refactor requireCustodian and requireMembership to share
one authorization middleware helper that accepts the appropriate
permission-check function, preserving their existing authorization behavior. In
the helper, invert the nil guard so invalid handlerFunc, res, or req inputs fail
loudly rather than silently returning a successful response; reuse the responder
created before the authentication check instead of constructing a second one.
In `@auth/service/api/v1/profile.go`:
- Around line 144-187: Sort results by user ID after group.Wait succeeds and
before responder.Data returns the response. Update the flow around the results
accumulation in the errgroup block, preserving the existing concurrent
collection and error handling while ensuring deterministic ordering for
identical requests.
In `@auth/store/mongo/legacy_seagull_profile_repository.go`:
- Around line 24-40: Update LegacySeagullProfileRepository to retain the store
returned by NewStore, add a Terminate method that shuts it down, and make
EnsureIndexes create the required unique userId index for lookups and
conditional updates.
In `@data/service/api/v1/mocks/mocks.go`:
- Around line 89-104: The new permission.Client methods are fixed-value stubs in
two hand-written test doubles. In data/service/api/v1/mocks/mocks.go lines
89-104, update PermissionsGrantedToUser, PermissionsGrantedByUser,
UsersHaveSharingRelationship, and HasCustodianPermissions to use receiver p,
returning p.Error and p.Default consistently with GetUserPermissions. In
auth/test/client.go lines 37-51, update the same four methods to record their
inputs and drain configurable outputs following the package’s existing accessor
pattern.
In `@go.mod`:
- Line 69: Move github.com/go-resty/resty/v2 from the indirect dependency block
into the direct require block in go.mod, remove the // indirect annotation, and
run go mod tidy to normalize the module requirements.
In `@user/fallback_user_accessor.go`:
- Around line 15-27: Remove the unused roleGetter field from
FallbackLegacyUserAccessor and remove the corresponding parameter and assignment
from NewFallbackLegacyUserAccessor; update the constructor call in
service/service.go to match the reduced signature.
- Around line 62-75: Update the retry loop around upsertLegacyUserProfile so it
only waits when another attempt remains, and replace time.Sleep with a
context-aware wait using ctx. Preserve immediate returns for non-migration
errors and return the final migration error after the retry limit is exhausted,
while propagating context cancellation promptly.
In `@user/keycloak/user_accessor.go`:
- Around line 54-63: Update UpdateLegacyUserProfile to copy the supplied
LegacyUserProfile before applying the role-based Clinic clearing, and pass the
copy to ToUserProfile. Preserve the original p value so callers and retry logic
retain the clinic data.
In `@user/legacy_raw_seagull_profile.go`:
- Around line 122-131: Update MarshalThenUnmarshal to accept dst as any instead
of *LegacyUserProfile, while preserving its existing JSON marshal and unmarshal
flow so it supports arbitrary destination types described by the function name.
In `@user/profile_test.go`:
- Around line 13-38: Extend the AddProfileToSeagullValue test context with cases
for malformed JSON and an empty seagullValueBefore string. Verify both inputs
discard the existing content and produce the expected profile value without
errors, covering the fallback branch while preserving the existing happy-path
test.
- Around line 42-110: Extend the profile conversion tests beyond ToLegacyProfile
by adding round-trip coverage through ToUserProfile and through
ToAttributes/ProfileFromAttributes, using representative patient data and
asserting the reconstructed profiles. Add a clinician case with no clinic fields
and verify the resulting profile contains a non-nil empty Clinic object, as
required by the Profile conversion contract.
In `@user/profile.go`:
- Around line 151-155: Remove the direct Clinic field assignment from
LegacyUserProfile.ToUserProfile. Leave the later deep-clone handling for
p.Clinic != nil and its nil behavior unchanged, so the returned Profile never
temporarily aliases the legacy clinic data.
- Around line 141-147: In the Custodian handling block of the profile conversion
function, ensure legacyProfile.Patient is initialized locally before assigning
IsOtherPerson and FullName, rather than relying on hasPatientFields() or
IsPatientProfile to have created it. Preserve the existing FullName selection
behavior.
- Around line 423-430: Update containsAttribute to directly retrieve
attributes[attribute] and check whether that value slice contains value,
removing the loop over all map keys while preserving the existing boolean result
for missing attributes.
- Around line 471-484: Update Profile.Normalize to trim whitespace from every
TargetDevices entry before ToAttributes consumes them, while preserving the
existing normalization of the other profile fields and nested values.
- Line 317: Remove the unused username parameter from ProfileFromAttributes and
update every call site to pass only attributes and roles, preserving the
function’s existing result behavior.
In `@user/user_accessor.go`:
- Around line 76-86: Update TokenIntrospectionResult.IsServerToken to use
slices.Contains directly on t.RealmAccess.Roles, removing the redundant length
guard and manual loop while preserving the existing boolean result.
- Around line 18-19: Update ShorelineManagedRoles to use the exported constants
RolePatient, RoleClinic, RoleClinician, and RoleCustodialAccount from user.go as
its keys instead of raw role-name literals, preserving the existing managed-role
set.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce5f8465-6327-4953-9cfd-f19bbdc775af
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
appvalidate/mock.goappvalidate/test/repository_mocks.goauth/service/api/v1/appvalidate_test.goauth/service/api/v1/permission.goauth/service/api/v1/profile.goauth/service/api/v1/router.goauth/service/api/v1/router_test.goauth/service/service.goauth/service/service/service.goauth/service/service/service_test.goauth/service/test/service.goauth/store/mongo/legacy_seagull_profile_repository.goauth/test/auth_mocks.goauth/test/client.goauth/user.goconsent/test/service_mocks.godata/client/test/mock.godata/raw/service/test/client_mocks.godata/raw/test/client_mocks.godata/service/api/v1/mocks/mocklogger_test_gen.godata/service/api/v1/mocks/mocks.godata/source/test/data_set_ensurer_mocks.godata/source/test/source_mocks.godexcom/fetch/test/runner_mocks.goenv.shenv.test.shgo.modpermission/client/client.gopermission/client_mock.gopermission/permission.goprescription/application/test/mock.gostore/structured/mongo/config.gotask/test/task_mocks.gotwiist/provider/test/provider_mocks.gouser/fallback_user_accessor.gouser/keycloak/client.gouser/keycloak/user_accessor.gouser/legacy_raw_seagull_profile.gouser/profile.gouser/profile_test.gouser/test/user.gouser/test/user_mocks.gouser/timeutil.gouser/user.gouser/user_accessor.gouser/user_mock.gowork/service/test/client_mocks.gowork/test/client_mocks.gowork/test/processor_mocks.go
| 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 | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
| seagullProfile, err := r.getProfile(ctx, userID) | ||
| if stdErrs.Is(err, user.ErrUserProfileNotFound) || seagullProfile == nil { | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect UserAccessor.Get implementations and their error/nil-user contract.
set -euo pipefail
fd -t f 'user_accessor.go|fallback_user_accessor.go' | while IFS= read -r f; do
echo "===== $f ====="
cat -n "$f"
done
echo "===== Get implementations across the repo ====="
ast-grep run --lang go --pattern 'func ($_ $_) Get(ctx context.Context, $_ string) (*User, error) { $$$ }'
ast-grep run --lang go --pattern 'func ($_ $_) Get(ctx context.Context, $_ string) (*user.User, error) { $$$ }'Repository: tidepool-org/platform
Length of output: 14436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -t f '^profile\.go$' | grep -E '(^|/)auth/service/api/v1/profile\.go$' | head -n1)"
echo "===== $file outline ====="
ast-grep outline "$file"
echo "===== target section ====="
cat -n "$file" | sed -n '35,180p'
echo "===== getProfile references ====="
rg -n -C 4 'getProfile|UserAccessor\(\)\.Get|sharedUser|seagullProfile' "$file" auth user 2>/dev/null | head -n 500Repository: tidepool-org/platform
Length of output: 15691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="auth/service/api/v1/profile.go"
echo "===== handler completion and error mapping ====="
cat -n "$file" | sed -n '175,270p'
echo "===== focused tests ====="
rg -n -C 5 'GetUsersWithProfiles|PermissionsGrantedToUser|PermissionsGrantedByUser|ErrUserProfileNotFound|ErrUserNotFound' \
auth/service/api/v1 --glob '*_test.go' | head -n 800
echo "===== all errgroup.Wait uses near API handlers ====="
rg -n -C 5 'group\.Wait\(\)|errgroup\.WithContext' auth/service/api/v1 --glob '*.go'Repository: tidepool-org/platform
Length of output: 11559
Check lookup errors before nil-result checks
getProfile returns nil, err for every lookup failure. The current check treats each failure as a missing profile, so group.Wait succeeds and the handler returns 200 OK with partial results. The same ordering can suppress (nil, err) from UserAccessor().Get. Check sentinel errors, then propagate other errors, before handling nil results.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/service/api/v1/profile.go` around lines 152 - 165, In the shared-user
and profile lookup flow, update the error handling around UserAccessor().Get and
r.getProfile so non-sentinel errors are propagated before checking nil results.
Preserve the user.ErrUserNotFound and user.ErrUserProfileNotFound cases as
successful missing-resource handling, then apply the nil-result checks only
after those errors have been handled.
| sanitizedUserDetails = &user.User{ | ||
| UserID: pointer.FromString(userID), | ||
| Username: pointer.FromString("dev@tidepool.org"), | ||
| EmailVerified: pointer.FromBool(true), | ||
| Roles: &userRoles, | ||
| Profile: &userProfile, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
sanitizedUserDetails is identical to userDetails, so the sanitization tests assert nothing.
Lines 592-598 copy every field from Lines 585-591, including Profile: &userProfile. userProfile carries Birthday, DiagnosisDate, and MRN.
Two tests depend on this value:
- Line 737, "returns sharer's full user info w/ sharee." expects
*sanitizedUserDetails. - Line 840, "returns sharer's limited user info w/ sharee." expects the same
*sanitizedUserDetailsand the same permissions.
Both cases now assert the unsanitized payload, and they assert the identical thing. If TrustUserArray stops redacting patient fields for a session-token caller, neither test fails. The test at Lines 852-872 does check redaction, which shows the two cases above are the gap.
Either build sanitizedUserDetails with a redacted profile, or rename the variable and the test at Line 837 to state that the full profile is expected.
💚 Proposed change
sanitizedUserDetails = &user.User{
UserID: pointer.FromString(userID),
Username: pointer.FromString("dev@tidepool.org"),
EmailVerified: pointer.FromBool(true),
Roles: &userRoles,
- Profile: &userProfile,
+ Profile: &user.Profile{FullName: "Some User Profile"},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sanitizedUserDetails = &user.User{ | |
| UserID: pointer.FromString(userID), | |
| Username: pointer.FromString("dev@tidepool.org"), | |
| EmailVerified: pointer.FromBool(true), | |
| Roles: &userRoles, | |
| Profile: &userProfile, | |
| } | |
| sanitizedUserDetails = &user.User{ | |
| UserID: pointer.FromString(userID), | |
| Username: pointer.FromString("dev@tidepool.org"), | |
| EmailVerified: pointer.FromBool(true), | |
| Roles: &userRoles, | |
| Profile: &user.Profile{FullName: "Some User Profile"}, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/service/api/v1/router_test.go` around lines 592 - 598, Update the test
fixture sanitizedUserDetails and the limited user-info test around the sharee
cases so sanitization is actually validated: use a profile with Birthday,
DiagnosisDate, and MRN redacted while preserving the expected non-sensitive
fields and permissions. Keep the full user-info test expecting userDetails, and
ensure the limited test compares against the redacted sanitizedUserDetails.
| err := p.FindOne(ctx, selector).Decode(&doc) | ||
| // A user can have no profile set - see seagull/lib/routes/seagullApi.js `if (err.statusCode == 404 && addIfNotThere)` | ||
| if err != nil && !stdErrors.Is(err, mongo.ErrNoDocuments) { | ||
| return err | ||
| } | ||
| hasExistingProfile := err == nil | ||
| // We need to make a distinction b/t a seagull profile not existing (in which case we can upsert) versus a seagull profile actively being migrated, which is why we need to actually read the document. | ||
| if hasExistingProfile && doc.IsMigrating() { | ||
| return user.ErrUserProfileMigrationInProgress | ||
| } | ||
|
|
||
| // This will create a new value even if doc.Value is empty | ||
| updatedValueRaw, err := user.AddProfileToSeagullValue(doc.Value, profile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| uopts := options.FindOneAndUpdate().SetUpsert(true).SetReturnDocument(options.After) | ||
| uselector := bson.M{ | ||
| "userId": userID, | ||
| } | ||
| update := bson.M{ | ||
| "$set": bson.M{ | ||
| "value": updatedValueRaw, | ||
| "userId": userID, // Set because of possible upsert | ||
| }, | ||
| } | ||
| var updatedDoc user.LegacySeagullDocument | ||
| err = p.FindOneAndUpdate(ctx, uselector, update, uopts).Decode(&updatedDoc) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Handle case where a migration was started in between the start of this function and the update | ||
| if updatedDoc.IsMigrating() { | ||
| return user.ErrUserProfileMigrationInProgress | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The read-modify-write is not atomic, so a concurrent migration can be overwritten.
The method reads the document at line 77, merges the profile into doc.Value at line 89, and writes the result at line 105. Between the read and the write, a migration can start or complete. Two consequences follow:
- The write clobbers the state that the migrator produced, including the migration status stored in
value. - The check at line 110 runs after the write. It returns
ErrUserProfileMigrationInProgress, but the document is already modified. The error does not undo the write.
A concurrent update from another request also loses data, because both requests merge into their own stale copy of value.
Make the update conditional so the server rejects a stale write. Include the previously read value (or a migration-status guard) in the filter, and treat "no document matched" as a retryable conflict.
🛡️ Sketch of a conditional update
- uselector := bson.M{
- "userId": userID,
- }
+ uselector := bson.M{
+ "userId": userID,
+ }
+ if hasExistingProfile {
+ // Reject the write if the stored value changed since the read above.
+ uselector["value"] = doc.Value
+ }With SetUpsert(true) and a non-matching filter, Mongo inserts a duplicate document, so pair this with a unique index on userId, or drop the upsert and handle mongo.ErrNoDocuments as a conflict.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/store/mongo/legacy_seagull_profile_repository.go` around lines 77 - 113,
Make the read-modify-write flow around the repository method’s initial
FindOne/Decode and subsequent FindOneAndUpdate conditional on the previously
read value (and preserve the no-document upsert case safely). Prevent stale
writes from overwriting concurrent migrations or profile updates by adding the
prior value or equivalent migration-status guard to the update filter, and avoid
allowing a non-matching upsert to create duplicates. Treat a failed conditional
update or duplicate-key conflict as a retryable conflict, and remove reliance on
the post-write IsMigrating check to undo changes.
| github.com/go-task/slim-sprig/v3 v3.0.0 // indirect | ||
| github.com/goccy/go-json v0.10.3 // indirect | ||
| github.com/golang/mock v1.6.0 // indirect | ||
| github.com/golang-jwt/jwt/v5 v5.0.0 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
golang-jwt/jwt/v5 v5.0.0 has a known high-severity advisory.
OSV reports GO-2025-3553 and GHSA-mh63-6h87-95cp against this version. The flaw allows excessive memory allocation during JWT header parsing. gocloak/v13 pulls this module in, and this service parses Keycloak access tokens on the request path in user/keycloak/client.go.
Bump the module and keep the requirement pinned.
🛡️ Proposed change
- github.com/golang-jwt/jwt/v5 v5.0.0 // indirect
+ github.com/golang-jwt/jwt/v5 v5.2.2 // indirectRun the following script to confirm the first patched version:
#!/bin/bash
# Description: Check advisories for github.com/golang-jwt/jwt/v5.
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: GO, package: "github.com/golang-jwt/jwt/v5") {
nodes {
advisory { ghsaId summary severity }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}'🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 72-72: github.com/golang-jwt/jwt/v5 5.0.0: Excessive memory allocation during header parsing in github.com/golang-jwt/jwt
(GO-2025-3553)
[HIGH] 72-72: github.com/golang-jwt/jwt/v5 5.0.0: jwt-go allows excessive memory allocation during header parsing
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` at line 72, Update the pinned github.com/golang-jwt/jwt/v5
requirement in go.mod from v5.0.0 to the first version patched for
GO-2025-3553/GHSA-mh63-6h87-95cp, and retain an explicit version pin. Refresh
the corresponding module checksums or dependency metadata as needed.
Source: Linters/SAST tools
| func (c *Client) UsersHaveSharingRelationship(ctx context.Context, granteeUserID, grantorUserID string) (has bool, err error) { | ||
| fromTo, err := c.GetUserPermissions(ctx, granteeUserID, grantorUserID) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if len(fromTo) > 0 { | ||
| return true, nil | ||
| } | ||
| toFrom, err := c.GetUserPermissions(ctx, grantorUserID, granteeUserID) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if len(toFrom) > 0 { | ||
| return true, nil | ||
| } | ||
| return false, nil | ||
| } | ||
|
|
||
| func (c *Client) HasCustodianPermissions(ctx context.Context, granteeUserID, grantorUserID string) (has bool, err error) { | ||
| perms, err := c.GetUserPermissions(ctx, granteeUserID, grantorUserID) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| _, ok := perms[permission.Custodian] | ||
| return ok, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Treat a missing access record as no permission.
GetUserPermissions converts a resource-not-found response into request.ErrorUnauthorized(). At Line 109, UsersHaveSharingRelationship then exits before it checks the reverse direction. At Line 127, HasCustodianPermissions propagates the same error, and requireCustodian converts it to HTTP 500.
Return an empty permission set for an absent access record, or expose a distinct not-found result. Preserve errors for actual permission-service failures. Add tests for a reverse-only relationship and for an unrelated user receiving HTTP 403.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@permission/client/client.go` around lines 107 - 131, Update
GetUserPermissions and the permission checks in UsersHaveSharingRelationship and
HasCustodianPermissions so an absent access record is treated as an empty
permission set while genuine permission-service failures still propagate. Ensure
reverse-direction checks continue after a missing forward record and unrelated
users reach the existing HTTP 403 path; add coverage for a reverse-only
relationship and the unrelated-user response.
| func (doc *LegacySeagullDocument) MigrationStatus() migrationStatus { | ||
| if doc.MigrationStart != nil && doc.MigrationEnd != nil { | ||
| return MigrationCompleted | ||
| } | ||
| if doc.MigrationStart != nil && doc.MigrationEnd == nil && doc.MigrationError == nil { | ||
| return MigrationInProgress | ||
| } | ||
| if doc.MigrationStart != nil && doc.MigrationError != nil { | ||
| return MigrationError | ||
| } | ||
| return MigrationUnmigrated | ||
| } | ||
|
|
||
| func (doc *LegacySeagullDocument) IsMigrating() bool { | ||
| return doc.MigrationStatus() != MigrationUnmigrated | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect every caller of IsMigrating and the fallback routing that consumes migration status.
set -euo pipefail
rg -n -C 6 '\bIsMigrating\s*\(' --type=go
rg -n -C 6 'ErrUserProfileMigrationInProgress|IsMigrationCompleted|MigrationError\b' --type=goRepository: tidepool-org/platform
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(legacy_seagull_profile_repository|legacy_raw_seagull_profile|user_accessor)\.go$'
printf '%s\n' '--- migration symbols and callers ---'
rg -n -C 8 'IsMigrating|MigrationStatus|MigrationCompleted|MigrationInProgress|MigrationError|ErrUserProfileMigrationInProgress' --glob '*.go' .
printf '%s\n' '--- relevant file outlines ---'
for f in $(git ls-files | rg '(^|/)(legacy_seagull_profile_repository|legacy_raw_seagull_profile|user_accessor)\.go$'); do
echo "### $f"
wc -l "$f"
ast-grep outline "$f" 2>/dev/null | head -120
doneRepository: tidepool-org/platform
Length of output: 27228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fallback routing and retry behavior ---'
cat -n user/fallback_user_accessor.go | sed -n '1,125p'
printf '%s\n' '--- repository update path ---'
cat -n auth/store/mongo/legacy_seagull_profile_repository.go | sed -n '55,116p'
printf '%s\n' '--- migration marker writers and tests ---'
rg -n -C 8 'Migration(Start|End|Error|ErrorTime)|MigrationStatus\(\)|IsMigrating\(\)' --glob '*.go' .Repository: tidepool-org/platform
Length of output: 16562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository and fallback wiring ---'
rg -n -C 8 'NewLegacySeagullProfileRepository|LegacySeagullProfileRepository|NewFallbackLegacyUserAccessor|FallbackLegacyUserAccessor' --glob '*.go' .
printf '%s\n' '--- migration-related tests ---'
rg -n -C 10 'Migration(Start|End|Error)|MigrationInProgress|MigrationCompleted|MigrationUnmigrated|MigrationError|ErrUserProfileMigrationInProgress' --glob '*_test.go' .
printf '%s\n' '--- deterministic state/routing probe ---'
python3 - <<'PY'
from itertools import product
states = {
"unmigrated": (False, False, False),
"completed": (True, True, False),
"in_progress": (True, False, False),
"error": (True, False, True),
"end_without_start": (False, True, False),
}
def status(start, end, error):
if start and end:
return "completed"
if start and not end and not error:
return "in_progress"
if start and error:
return "error"
return "unmigrated"
for name, flags in states.items():
s = status(*flags)
legacy_repo_blocks = s != "unmigrated"
fallback_reads_seagull = s != "completed"
fallback_updates_seagull = s == "unmigrated"
print(f"{name}: status={s}, IsMigrating={legacy_repo_blocks}, "
f"fallback_reads_seagull={fallback_reads_seagull}, "
f"fallback_updates_seagull={fallback_updates_seagull}")
PYRepository: tidepool-org/platform
Length of output: 19047
Handle migration statuses explicitly in the fallback accessor.
FallbackLegacyUserAccessor.upsertLegacyUserProfile routes MigrationInProgress and MigrationError profiles to Keycloak because it sends only MigrationUnmigrated profiles to Seagull. This bypasses the intended retry path during migration and treats failed migrations as migrated. Changing IsMigrating alone does not fix this path. Also reject or handle MigrationEnd != nil with MigrationStart == nil; MigrationStatus currently classifies it as MigrationUnmigrated and keeps reads and writes on Seagull.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@user/legacy_raw_seagull_profile.go` around lines 133 - 148, Update
FallbackLegacyUserAccessor.upsertLegacyUserProfile to route MigrationUnmigrated,
MigrationInProgress, and MigrationError profiles through the intended
migration/retry handling instead of sending only unmigrated profiles to Seagull.
Update LegacySeagullDocument.MigrationStatus to explicitly handle MigrationEnd
being set without MigrationStart, rejecting it or routing it to an appropriate
non-Seagull status so reads and writes do not remain on Seagull. Do not rely on
changing IsMigrating alone.
| func (up *Profile) Validate(v structure.Validator) { | ||
| v.String("fullName", &up.FullName).LengthLessThanOrEqualTo(MaxProfileFieldLen) | ||
| v.String("diagnosisType", &up.DiagnosisType).LengthLessThanOrEqualTo(MaxProfileFieldLen) | ||
| v.String("targetTimezone", &up.TargetTimezone).LengthLessThanOrEqualTo(MaxProfileFieldLen) | ||
| v.String("about", &up.About).LengthLessThanOrEqualTo(MaxProfileFieldLen) | ||
| v.String("mrn", &up.MRN).LengthLessThanOrEqualTo(MaxProfileFieldLen) | ||
| v.String("biologicalSex", &up.BiologicalSex).LengthLessThanOrEqualTo(MaxProfileFieldLen) | ||
|
|
||
| up.Birthday.Validate(v.WithReference("birthday")) | ||
| up.DiagnosisDate.Validate(v.WithReference("diagnosisDate")) | ||
| if up.DiagnosisType != "" { | ||
| v.String("diagnosisType", &up.DiagnosisType).OneOf(DiabetesTypes...) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the missing length validations.
Profile.Validate bounds every scalar string except the Clinic fields. Profile.Normalize does normalize Clinic, and ToAttributes writes clinic_name, clinic_role, clinic_telephone, and clinic_npi to Keycloak. Those values currently have no length bound. LegacyPatientProfile.Validate also omits biologicalSex, which LegacyPatientProfile.Normalize trims and Profile.Validate bounds. The legacy write path therefore accepts values the new path rejects.
🐛 Proposed fix
up.Birthday.Validate(v.WithReference("birthday"))
up.DiagnosisDate.Validate(v.WithReference("diagnosisDate"))
if up.DiagnosisType != "" {
v.String("diagnosisType", &up.DiagnosisType).OneOf(DiabetesTypes...)
}
+ if up.Clinic != nil {
+ up.Clinic.Validate(v.WithReference("clinic"))
+ }
}
+
+func (p *ClinicProfile) Validate(v structure.Validator) {
+ v.String("name", p.Name).LengthLessThanOrEqualTo(MaxProfileFieldLen)
+ v.String("role", p.Role).LengthLessThanOrEqualTo(MaxProfileFieldLen)
+ v.String("telephone", p.Telephone).LengthLessThanOrEqualTo(MaxProfileFieldLen)
+ v.String("npi", p.NPI).LengthLessThanOrEqualTo(MaxProfileFieldLen)
+} v.String("targetTimezone", &pp.TargetTimezone).LengthLessThanOrEqualTo(MaxProfileFieldLen)
v.String("about", &pp.About).LengthLessThanOrEqualTo(MaxProfileFieldLen)
v.String("mrn", &pp.MRN).LengthLessThanOrEqualTo(MaxProfileFieldLen)
+ v.String("biologicalSex", &pp.BiologicalSex).LengthLessThanOrEqualTo(MaxProfileFieldLen)Also applies to: 519-531
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@user/profile.go` around lines 456 - 469, Update Profile.Validate to apply
MaxProfileFieldLen validation to all Clinic string fields written by
ToAttributes: clinic_name, clinic_role, clinic_telephone, and clinic_npi. Update
LegacyPatientProfile.Validate to add the same length validation for
biologicalSex, matching Profile.Validate and its normalization behavior.
| 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 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find producers and consumers of TimestampFormat.
set -euo pipefail
rg -n -C 5 'TimestampFormat|ParseTimestamp|TimestampToUnixString|UnixStringToTimestamp' --type=goRepository: tidepool-org/platform
Length of output: 159
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -i 'timeutil.go|user_accessor.go' .
printf '%s\n' '--- timestamp references ---'
rg -n -C 5 'TimestampFormat|ParseTimestamp|TimestampToUnixString|UnixStringToTimestamp' . || trueRepository: tidepool-org/platform
Length of output: 2700
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- time utility ---'
cat -n user/timeutil.go
printf '%s\n' '--- format declaration ---'
cat -n user/user_accessor.go | sed -n '1,25p'
printf '%s\n' '--- consumer context ---'
cat -n user/keycloak/client.go | sed -n '430,475p'
printf '%s\n' '--- related tests and configuration ---'
rg -n -C 4 'timeutil|TimestampToUnixString|UnixStringToTimestamp|ParseTimestamp|createdTimestamp|termsAccepted|TimestampFormat' --glob '*_test.go' --glob 'go.mod' --glob 'go.sum' .
printf '%s\n' '--- standalone Go parser check ---'
if command -v go >/dev/null 2>&1; then
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/check.go" <<'EOF'
package main
import (
"fmt"
"time"
)
func main() {
const layout = "2006-01-02T15:04:05-07:00"
for _, input := range []string{
"2024-01-02T03:04:05Z",
"2024-01-02T03:04:05+00:00",
"2024-01-02T03:04:05-05:00",
} {
parsed, err := time.Parse(layout, input)
fmt.Printf("%q => %s, %v\n", input, parsed.Format(time.RFC3339), err)
}
}
EOF
(cd "$tmpdir" && go run check.go)
else
printf '%s\n' 'go is unavailable'
fiRepository: tidepool-org/platform
Length of output: 7969
Support RFC 3339 UTC timestamps or define a numeric-offset contract.
time.Parse with TimestampFormat rejects valid timestamps such as 2024-01-02T03:04:05Z. Use time.RFC3339 and add a Z test if RFC 3339 input is supported. No in-repository producer establishes that inputs always use numeric offsets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@user/timeutil.go` around lines 9 - 20, Update ParseTimestamp to parse
timestamps with time.RFC3339 so valid UTC “Z” timestamps are accepted, while
preserving TimestampToUnixString’s existing conversion behavior. Add coverage
for an RFC 3339 input ending in “Z” if tests are available.
| 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})$`) | ||
| custodialAccountRegexp = regexp.MustCompile(`(?i)unclaimed-custodial-automation\+\d+@tidepool\.org`) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Anchor custodialAccountRegexp.
The pattern is unanchored, so MatchString matches any address that contains the pattern as a substring. An address such as victim+unclaimed-custodial-automation+1@tidepool.org.example.com is classified as an unclaimed custodial email. Anchor the expression to the whole address.
🔒 Proposed fix
- custodialAccountRegexp = regexp.MustCompile(`(?i)unclaimed-custodial-automation\+\d+@tidepool\.org`)
+ custodialAccountRegexp = regexp.MustCompile(`(?i)^unclaimed-custodial-automation\+\d+@tidepool\.org$`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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})$`) | |
| custodialAccountRegexp = regexp.MustCompile(`(?i)unclaimed-custodial-automation\+\d+@tidepool\.org`) | |
| IdExpression = regexp.MustCompile(`^([0-9a-f]{10}|[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-F]{4}\-[0-9a-F]{12})$`) | |
| custodialAccountRegexp = regexp.MustCompile(`(?i)^unclaimed-custodial-automation\+\d+@tidepool\.org$`) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@user/user.go` around lines 40 - 41, Update custodialAccountRegexp to anchor
the pattern at both the beginning and end of the string, so MatchString only
accepts the complete unclaimed-custodial address and rejects addresses with
surrounding or trailing content.
| 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. | ||
| if (u.TrustorPermissions == nil || len(*u.TrustorPermissions) == 0) && u.User.Profile != nil { | ||
| u.User.Profile.Sanitize() | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against a nil UserID before dereferencing.
Line 144 dereferences *u.UserID. If any accessor returns a user without an ID, this panics inside the HTTP handler that serves /v1/users/:userId/users. Treat a nil UserID as "not the requesting user".
🐛 Proposed fix
func (u *TrustUser) Sanitize(details request.AuthDetails) error {
- if details == nil || (!details.IsService() && details.UserID() != *u.UserID) {
+ if details == nil || !details.IsService() && (u.UserID == nil || details.UserID() != *u.UserID) {
// Note that a TrustUser includes some fields in the user that [User.Sanitize] wouldn't.
if (u.TrustorPermissions == nil || len(*u.TrustorPermissions) == 0) && u.User.Profile != nil {
u.User.Profile.Sanitize()
}
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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. | |
| if (u.TrustorPermissions == nil || len(*u.TrustorPermissions) == 0) && u.User.Profile != nil { | |
| u.User.Profile.Sanitize() | |
| } | |
| } | |
| return nil | |
| } | |
| func (u *TrustUser) Sanitize(details request.AuthDetails) error { | |
| if details == nil || !details.IsService() && (u.UserID == nil || details.UserID() != *u.UserID) { | |
| // Note that a TrustUser includes some fields in the user that [User.Sanitize] wouldn't. | |
| if (u.TrustorPermissions == nil || len(*u.TrustorPermissions) == 0) && u.User.Profile != nil { | |
| u.User.Profile.Sanitize() | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@user/user.go` around lines 143 - 151, Update the condition in
TrustUser.Sanitize to check u.UserID for nil before dereferencing it, treating a
nil UserID as not the requesting user while preserving the existing service-user
and matching-ID behavior.
4 participants
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 calledUser).platform/user.gouser.Userextended with shoreline user fields.shoreline/user/hasher.go=>platform/user/hasher.goshoreline/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), implementsplatform/user.UserAccessorinterfaceshoreline/keycloak/client.go=>platform/user/keycloak/client.go(Wrapper around gocloak).