[BACK-2780] Address code review feedback (attribute merge fix + cleanup) - #967
Open
toddkazakov wants to merge 1 commit into
Open
[BACK-2780] Address code review feedback (attribute merge fix + cleanup)#967toddkazakov wants to merge 1 commit into
toddkazakov wants to merge 1 commit into
Conversation
- Fix profile attribute merge bug: clear existing profile attributes before applying the updated profile's attributes so cleared fields no longer retain stale values in keycloak (introduces RemoveProfileAttributes and attribute name constants as a single source of truth) - Fix terms_and_conditions round-trip: convert the terms accepted timestamp back to a unix timestamp string when updating a user, matching the format read by newUserFromGocloakUser - Remove unused User fields (Emails, PwHash, Hash, PasswordExists) and methods (IsClinic, IsClinician, IsCustodialAccount, AreTermsAccepted) - Remove bson tags from User since it is never stored in mongo - Call IsUnclaimedCustodialEmail in the keycloak client so unclaimed custodial placeholder emails are not exposed as the account username - Consolidate user id validation: user.ValidateID is the single source of truth, auth.ValidateUserID delegates to it - Slim UserAccessor: remove unused FindUser/FindUsersWithIds, replace FindUserById with the embedded user.Client interface (Get) - Move keycloak-specific types (TokenIntrospectionResult et al) and time helpers from the user package to user/keycloak - Move LegacySeagullProfileRepository from auth/store/mongo to user/store/mongo - Drop the unused username parameter of ProfileFromAttributes - Remove the userlib import alias in user/keycloak per import conventions
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the code review feedback from the 2026-08-07 review on #698. Targets
jimmy/BACK-2780-new-profiles-endpointso it can be merged into that PR.Bug fixes
user/keycloak/client.go):UpdateUsernow removes all profile-owned attributes from the merged attribute map before applyingProfile.ToAttributes(), so clearing a profile field actually removes the stale attribute in keycloak while non-profile attributes are still preserved. Attribute names are now constants inuser/profile.gowithRemoveProfileAttributesas the single source of truth, covered by new tests inuser/profile_test.go.terms_and_conditionsround-trip (drive-by fix found while fixing the above):UpdateUserwroteTermsAcceptedin the2006-01-02T15:04:05-07:00format into an attribute that is read as a unix timestamp string, so any profile update silently corrupted the terms-accepted attribute. It is now converted back to a unix timestamp string on write; if conversion fails the existing attribute value is kept.Cleanup, per inline comments
User: removedEmails(always empty),PwHash,Hash,PasswordExists, and all bson tags (the struct never touches mongo); removed unusedIsClinic,IsClinician,AreTermsAccepted, andIsCustodialAccount(only populated the removedPwHash).IsUnclaimedCustodialEmailis now called in the keycloak client: unclaimed custodial placeholder emails are no longer exposed as the account's username. On update, a nil username is omitted from the keycloak payload, so the stored username is unaffected.user.ValidateID/user.IsValidIDown the (single) regexp andauth.ValidateUserIDdelegates to it. I kept the stricterauthpattern (lowercase-only), so theuserpackage no longer accepts uppercase hex in the uuid form — keycloak ids are lowercase so this should have no practical effect.UserAccessorslimmed: unusedFindUserandFindUsersWithIdsremoved (along with the now-orphanedGetUserByEmailin the keycloak client), andFindUserByIdreplaced by embedding the existinguser.Clientinterface (Get). Mocks regenerated.userpackage intouser/keycloak:TokenIntrospectionResult,AccessTokenCustomClaims,RealmAccess,IsServerToken, and the timestamp helpers fromuser/timeutil.go(now unexported;ParseTimestampand the exported wrappers had no remaining users).LegacySeagullProfileRepositoryfromauth/store/mongoto a newuser/store/mongopackage, next to theuserpackage that ownsLegacySeagullDocument. I used astore/mongosubpackage rather than theuserpackage itself to keep the mongo driver dependency out of the widely imported core package — happy to move it elsewhere.usernameparameter ofProfileFromAttributes(it only existed for the previously removed legacy email population).userlibimport alias inuser/keycloakand renamed shadowing locals instead.Not addressed (needs discussion)
has_custodianattribute → custodial role: not changed here. The attribute exists so the keycloak fake-child-migration extension can filter users, so replacing it with thecustodial_accountrealm role needs a coordinated change in that extension (and the role currently means "unclaimed account", which is not quite the same as "has a custodian in the profile"). Suggest resolving this on the review thread first.Testing
GOTEST_PKGS="./user/... ./auth/... ./data/raw/..." make test-gopasses;go build ./...andgo vet ./...are clean;goimports -local github.com/tidepool-org/platformapplied to all touched files.