feat(oidc): support for all in-spec attributes and scopes#777
Open
scottmckendry wants to merge 8 commits intotinyauthapp:mainfrom
Open
feat(oidc): support for all in-spec attributes and scopes#777scottmckendry wants to merge 8 commits intotinyauthapp:mainfrom
scottmckendry wants to merge 8 commits intotinyauthapp:mainfrom
Conversation
dosubot
Bot
added
the
size:XL
This comment was marked as outdated.
This comment was marked as outdated.
|
Related Documentation 2 document(s) may need updating based on files changed in this PR: Tinyauth's Space configuration
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/service/oidc_service.go (1)
708-713: Silent failure on address unmarshal may hide data corruption.If
user.Addresscontains malformed JSON (not just"null"), the error is silently ignored andAddresswill be nil. Consider logging a warning for debugging purposes.🔧 Optional: Log unmarshal errors for debugging
if slices.Contains(scopes, "address") { var addr config.AddressClaim - if err := json.Unmarshal([]byte(user.Address), &addr); err == nil { + if err := json.Unmarshal([]byte(user.Address), &addr); err != nil { + tlog.App.Debug().Err(err).Str("sub", user.Sub).Msg("Failed to unmarshal address claim") + } else { userInfo.Address = &addr } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/oidc_service.go` around lines 708 - 713, The code silently ignores json.Unmarshal errors when populating userInfo.Address from user.Address; modify the block that checks slices.Contains(scopes, "address") so that if json.Unmarshal(&addr) returns an error you log a warning containing the error and the raw user.Address (and the relevant subject or user ID) instead of dropping it silently, then only set userInfo.Address on success; reference the same symbols (slices.Contains, user.Address, json.Unmarshal, addr, userInfo.Address) and use the service's existing logger instance to emit the warning.internal/middleware/context_middleware.go (1)
116-119: Consider handling the case whenGetLocalUserreturns a user that doesn't exist.The code assumes
GetLocalUseralways returns a valid user. Ifcookie.Usernamedoesn't correspond to an existing local user (e.g., deleted after session creation), the code proceeds without error handling. The earlierSearchUsercheck (line 86-92) should catch this, but there's a potential TOCTOU gap if the user is deleted between the search and this call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/middleware/context_middleware.go` around lines 116 - 119, The code assumes m.auth.GetLocalUser(cookie.Username) always returns a valid user; update the block where cookie.Provider == "local" to check the returned localUser for nil (or a not-found indicator) before accessing localUser.Attributes, and handle the missing-user case safely (e.g., set localAttributes to an empty map/slice, skip attaching attributes, or return an auth error), and add a brief log entry referencing cookie.Username so you can trace TOCTOU deletions; ensure this change touches the GetLocalUser call site and the localAttributes assignment to avoid a nil dereference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config.go`:
- Around line 115-130: Implement a custom UnmarshalYAML on AuthConfig that
accepts both the new map format and the legacy list/string format for the Users
field: in UnmarshalYAML, unmarshal into a temporary struct mirroring AuthConfig
(to avoid recursion) and inspect the YAML node for Users; if Users is a sequence
of strings like "user:hash" (or sequence of single-key maps), convert those
entries into the map[string]UserConfig (setting Password field) before assigning
to the real AuthConfig.Users, otherwise accept the mapping form directly; after
normalizing the Users map, continue unmarshalling other fields and call
GetUsers() (or ensure existing GetUsers() will receive the normalized map).
Reference: implement UnmarshalYAML on type AuthConfig, handle field Users and
call GetUsers().
In `@internal/controller/user_controller.go`:
- Around line 157-165: In the branch that checks if userSearch.Type == "local",
replace the inconsistent lookup using req.Username when calling
controller.auth.GetLocalUser with the resolved username from userSearch (i.e.
use userSearch.Username) so both TOTP and non-TOTP flows use the same resolved
identifier; update any related sessionCookie population logic that assumes
values from localUser.Attributes (sessionCookie.Name, sessionCookie.Email) to
continue using the localUser returned for userSearch.Username to ensure
consistent behavior across branches.
In `@internal/controller/well_known_controller.go`:
- Line 62: ClaimsSupported currently lists only a subset of profile claims;
update the ClaimsSupported slice in well_known_controller.go (the
ClaimsSupported field) to include the missing profile claims: given_name,
family_name, middle_name, nickname, profile, picture, website, gender,
birthdate, zoneinfo, and locale so discovery documents accurately advertise all
implemented profile claims. Ensure the new claim names are added as strings in
the existing ClaimsSupported []string literal alongside the existing entries.
---
Nitpick comments:
In `@internal/middleware/context_middleware.go`:
- Around line 116-119: The code assumes m.auth.GetLocalUser(cookie.Username)
always returns a valid user; update the block where cookie.Provider == "local"
to check the returned localUser for nil (or a not-found indicator) before
accessing localUser.Attributes, and handle the missing-user case safely (e.g.,
set localAttributes to an empty map/slice, skip attaching attributes, or return
an auth error), and add a brief log entry referencing cookie.Username so you can
trace TOCTOU deletions; ensure this change touches the GetLocalUser call site
and the localAttributes assignment to avoid a nil dereference.
In `@internal/service/oidc_service.go`:
- Around line 708-713: The code silently ignores json.Unmarshal errors when
populating userInfo.Address from user.Address; modify the block that checks
slices.Contains(scopes, "address") so that if json.Unmarshal(&addr) returns an
error you log a warning containing the error and the raw user.Address (and the
relevant subject or user ID) instead of dropping it silently, then only set
userInfo.Address on success; reference the same symbols (slices.Contains,
user.Address, json.Unmarshal, addr, userInfo.Address) and use the service's
existing logger instance to emit the warning.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa259fe0-f366-4eeb-b986-b387e09ce3df
📒 Files selected for processing (15)
frontend/src/lib/i18n/locales/en.jsonfrontend/src/pages/authorize-page.tsxinternal/assets/migrations/000008_oidc_userinfo_profile.down.sqlinternal/assets/migrations/000008_oidc_userinfo_profile.up.sqlinternal/config/config.gointernal/controller/user_controller.gointernal/controller/well_known_controller.gointernal/middleware/context_middleware.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gointernal/utils/user_utils.gointernal/utils/user_utils_test.gosql/oidc_queries.sqlsql/oidc_schemas.sql
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
376b61c to
11e5158
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/oidc_service.go (1)
44-67: Consider removing unusedClaimSetfields.After the
generateIDTokenchange (lines 477-487), the profile-related fields inClaimSet(GivenName,FamilyName, etc.) are never populated. Per the OIDC Core §5.4 comment, scope-requested claims now go in the userinfo response only.These fields could be removed to avoid confusion, or retained if there's a future use case for returning claims in the ID token.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/oidc_service.go` around lines 44 - 67, The ClaimSet struct contains many profile-related fields that are no longer populated by generateIDToken; remove the unused fields (GivenName, FamilyName, MiddleName, Nickname, Profile, Picture, Website, Gender, Birthdate, Zoneinfo, Locale, PreferredUsername, Groups) from the ClaimSet type to avoid confusion, update any code that referenced those fields (including serialization/tests), and keep Nonce/Iat/Exp/Aud/Sub/Iss/Email/EmailVerified if still used by generateIDToken and userinfo flows; ensure generateIDToken and any userinfo code still compile and tests updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/user_controller.go`:
- Around line 108-112: The code references an undefined type utils.User; update
imports and the type to use the correct package: add an import for the config
package and change the localUser declaration/type from utils.User to config.User
(e.g., var localUser *config.User) and ensure the result of
controller.auth.GetLocalUser(userSearch.Username) is assigned to a config.User
value or pointer as appropriate so the types match (refer to localUser,
userSearch, and controller.auth.GetLocalUser to locate the change).
---
Nitpick comments:
In `@internal/service/oidc_service.go`:
- Around line 44-67: The ClaimSet struct contains many profile-related fields
that are no longer populated by generateIDToken; remove the unused fields
(GivenName, FamilyName, MiddleName, Nickname, Profile, Picture, Website, Gender,
Birthdate, Zoneinfo, Locale, PreferredUsername, Groups) from the ClaimSet type
to avoid confusion, update any code that referenced those fields (including
serialization/tests), and keep Nonce/Iat/Exp/Aud/Sub/Iss/Email/EmailVerified if
still used by generateIDToken and userinfo flows; ensure generateIDToken and any
userinfo code still compile and tests updated accordingly.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de97d413-4c97-4e84-88b6-7183d64f2439
📒 Files selected for processing (16)
frontend/src/lib/i18n/locales/en.jsonfrontend/src/pages/authorize-page.tsxinternal/assets/migrations/000008_oidc_userinfo_profile.down.sqlinternal/assets/migrations/000008_oidc_userinfo_profile.up.sqlinternal/config/config.gointernal/controller/user_controller.gointernal/controller/well_known_controller.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gointernal/utils/user_utils.gointernal/utils/user_utils_test.gosql/oidc_queries.sqlsql/oidc_schemas.sql
✅ Files skipped from review due to trivial changes (7)
- frontend/src/pages/authorize-page.tsx
- internal/controller/well_known_controller_test.go
- internal/controller/well_known_controller.go
- frontend/src/lib/i18n/locales/en.json
- sql/oidc_schemas.sql
- internal/assets/migrations/000008_oidc_userinfo_profile.up.sql
- sql/oidc_queries.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/assets/migrations/000008_oidc_userinfo_profile.down.sql
- internal/middleware/context_middleware.go
- internal/repository/models.go
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
11e5158 to
0f58663
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #777 +/- ##
==========================================
+ Coverage 20.05% 21.11% +1.06%
==========================================
Files 50 50
Lines 3995 4102 +107
==========================================
+ Hits 801 866 +65
- Misses 3121 3163 +42
Partials 73 73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
0f58663 to
c2d630b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/middleware/context_middleware.go (1)
116-129:⚠️ Potential issue | 🟠 MajorPropagate attribute-backed
nameandThis branch only fills
UserContext.Attributes;UserContext.NameandUserContext.Emailstill come from the session cookie.StoreUserinfolater persists the top-level fields, so session-backed local logins can still publish stale/defaultnameanduser.Attributes. Align this branch with the fallback logic added below.Suggested fix
if cookie.Provider == "local" { localUser := m.auth.GetLocalUser(cookie.Username) localAttributes = localUser.Attributes } + + name := cookie.Name + if localAttributes.Name != "" { + name = localAttributes.Name + } + + email := cookie.Email + if localAttributes.Email != "" { + email = localAttributes.Email + } m.auth.RefreshSessionCookie(c) c.Set("context", &config.UserContext{ Username: cookie.Username, - Name: cookie.Name, - Email: cookie.Email, + Name: name, + Email: email, Provider: cookie.Provider, IsLoggedIn: true, LdapGroups: strings.Join(ldapGroups, ","), Attributes: localAttributes, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/middleware/context_middleware.go` around lines 116 - 129, When cookie.Provider == "local" after calling localUser := m.auth.GetLocalUser(cookie.Username) and setting localAttributes, also prefer attribute-backed name/email: if localUser.Attributes contains "name" or "email" use those values to set UserContext.Name and UserContext.Email instead of the cookie values; then call m.auth.RefreshSessionCookie(c) and create the context with Username, Name, Email, Provider, IsLoggedIn, LdapGroups and Attributes (so the c.Set("context", &config.UserContext{...}) block uses the attribute-backed Name/Email when present).
🧹 Nitpick comments (1)
internal/utils/user_utils_test.go (1)
147-151: Consider adding a test case for empty username.The implementation in
ParseUsersFromMapvalidates empty usernames (after trimming), but there's no test case for this. Consider adding:// Invalid: empty username (whitespace-only key) _, err = utils.ParseUsersFromMap(map[string]config.UserConfig{ " ": {Password: hash}, }) assert.ErrorContains(t, err, "empty username")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/utils/user_utils_test.go` around lines 147 - 151, Add a test case in internal/utils/user_utils_test.go next to the existing invalid-password case that exercises ParseUsersFromMap with a whitespace-only map key to verify empty-username validation; call utils.ParseUsersFromMap with a map[string]config.UserConfig containing a key like " " and a valid Password (e.g., hash) and assert that the returned error contains "empty username" (use assert.ErrorContains with the same pattern as the empty-password test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/oidc_service.go`:
- Around line 702-713: The phone and address claims are being emitted as
placeholders even when empty; update the block that checks
slices.Contains(scopes, "phone") to only set userInfo.PhoneNumber and
userInfo.PhoneNumberVerified if user.PhoneNumber is non-empty (set the verified
pointer only when a phone exists), and update the address block to skip setting
userInfo.Address unless user.Address contains real data (e.g., user.Address is
non-empty and json.Unmarshal into config.AddressClaim yields a
non-zero/meaningful struct rather than default/empty {}). Ensure you reference
the existing symbols (scopes check, user.PhoneNumber, user.PhoneNumberVerified,
userInfo.PhoneNumberVerified, user.Address, config.AddressClaim,
userInfo.Address) when making the change.
---
Outside diff comments:
In `@internal/middleware/context_middleware.go`:
- Around line 116-129: When cookie.Provider == "local" after calling localUser
:= m.auth.GetLocalUser(cookie.Username) and setting localAttributes, also prefer
attribute-backed name/email: if localUser.Attributes contains "name" or "email"
use those values to set UserContext.Name and UserContext.Email instead of the
cookie values; then call m.auth.RefreshSessionCookie(c) and create the context
with Username, Name, Email, Provider, IsLoggedIn, LdapGroups and Attributes (so
the c.Set("context", &config.UserContext{...}) block uses the attribute-backed
Name/Email when present).
---
Nitpick comments:
In `@internal/utils/user_utils_test.go`:
- Around line 147-151: Add a test case in internal/utils/user_utils_test.go next
to the existing invalid-password case that exercises ParseUsersFromMap with a
whitespace-only map key to verify empty-username validation; call
utils.ParseUsersFromMap with a map[string]config.UserConfig containing a key
like " " and a valid Password (e.g., hash) and assert that the returned error
contains "empty username" (use assert.ErrorContains with the same pattern as the
empty-password test).
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a4466f8-2aa3-4d0d-a8b2-d5f76eeeb699
📒 Files selected for processing (16)
frontend/src/lib/i18n/locales/en.jsonfrontend/src/pages/authorize-page.tsxinternal/assets/migrations/000008_oidc_userinfo_profile.down.sqlinternal/assets/migrations/000008_oidc_userinfo_profile.up.sqlinternal/config/config.gointernal/controller/user_controller.gointernal/controller/well_known_controller.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gointernal/utils/user_utils.gointernal/utils/user_utils_test.gosql/oidc_queries.sqlsql/oidc_schemas.sql
✅ Files skipped from review due to trivial changes (6)
- internal/controller/well_known_controller_test.go
- internal/controller/well_known_controller.go
- internal/assets/migrations/000008_oidc_userinfo_profile.down.sql
- frontend/src/lib/i18n/locales/en.json
- internal/assets/migrations/000008_oidc_userinfo_profile.up.sql
- internal/config/config.go
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/pages/authorize-page.tsx
- sql/oidc_schemas.sql
- internal/controller/user_controller.go
- internal/repository/models.go
- internal/repository/oidc_queries.sql.go
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
2 times, most recently
from
eb40af6 to
f7a2ad1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/service/oidc_service.go (1)
702-713:⚠️ Potential issue | 🟡 MinorPhone and address claims may emit empty/placeholder values.
The past review suggested omitting these claims when the underlying data is absent, but the current implementation still:
- Sets
PhoneNumberVerifiedpointer even whenPhoneNumberis empty- Sets
Addresspointer for any successfully unmarshalled JSON, including{}This results in responses like
"phone_number": "", "phone_number_verified": falseor"address": {}when no data exists.Suggested fix
if slices.Contains(scopes, "phone") { - userInfo.PhoneNumber = user.PhoneNumber - verified := user.PhoneNumberVerified - userInfo.PhoneNumberVerified = &verified + if user.PhoneNumber != "" { + userInfo.PhoneNumber = user.PhoneNumber + verified := user.PhoneNumberVerified + userInfo.PhoneNumberVerified = &verified + } } if slices.Contains(scopes, "address") { var addr config.AddressClaim - if err := json.Unmarshal([]byte(user.Address), &addr); err == nil { + if err := json.Unmarshal([]byte(user.Address), &addr); err == nil && + (addr.Formatted != "" || addr.StreetAddress != "" || + addr.Locality != "" || addr.Region != "" || + addr.PostalCode != "" || addr.Country != "") { userInfo.Address = &addr } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/oidc_service.go` around lines 702 - 713, The phone and address claims are being emitted even when underlying data is absent; update the logic in the block that checks slices.Contains(scopes, "phone") to only set userInfo.PhoneNumber and userInfo.PhoneNumberVerified when user.PhoneNumber is non-empty (i.e., skip setting the PhoneNumberVerified pointer if PhoneNumber == ""), and update the address block (the code that unmarshals user.Address into config.AddressClaim and assigns userInfo.Address) to only assign a non-nil pointer when the unmarshalled addr contains meaningful fields (e.g., check for zero/empty fields on the config.AddressClaim struct or use a helper like addr.IsEmpty() before setting userInfo.Address) so empty strings/{} are not returned as claims.
🧹 Nitpick comments (1)
internal/utils/user_utils.go (1)
64-88: Consider trimmingUserAttributesstring fields for consistency.While
Username,Password, andTotpSecretare trimmed,cfg.Attributesis assigned directly. If users accidentally include leading/trailing whitespace in YAML attribute values (e.g.,name: " John Doe "), it will be preserved. This is minor since YAML parsers typically handle this, but trimming would ensure consistency with other fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/utils/user_utils.go` around lines 64 - 88, In ParseUsersFromMap, trim whitespace for each attribute value before assigning Attributes: iterate over cfg.Attributes (e.g., for k, v := range cfg.Attributes), call strings.TrimSpace on v, build a cleaned map (preserving keys) and set Attributes: cleanedMap on the config.User being appended; this mirrors the trimming applied to Username, Password, and TotpSecret and ensures attribute string fields don't retain accidental leading/trailing whitespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/service/oidc_service.go`:
- Around line 702-713: The phone and address claims are being emitted even when
underlying data is absent; update the logic in the block that checks
slices.Contains(scopes, "phone") to only set userInfo.PhoneNumber and
userInfo.PhoneNumberVerified when user.PhoneNumber is non-empty (i.e., skip
setting the PhoneNumberVerified pointer if PhoneNumber == ""), and update the
address block (the code that unmarshals user.Address into config.AddressClaim
and assigns userInfo.Address) to only assign a non-nil pointer when the
unmarshalled addr contains meaningful fields (e.g., check for zero/empty fields
on the config.AddressClaim struct or use a helper like addr.IsEmpty() before
setting userInfo.Address) so empty strings/{} are not returned as claims.
---
Nitpick comments:
In `@internal/utils/user_utils.go`:
- Around line 64-88: In ParseUsersFromMap, trim whitespace for each attribute
value before assigning Attributes: iterate over cfg.Attributes (e.g., for k, v
:= range cfg.Attributes), call strings.TrimSpace on v, build a cleaned map
(preserving keys) and set Attributes: cleanedMap on the config.User being
appended; this mirrors the trimming applied to Username, Password, and
TotpSecret and ensures attribute string fields don't retain accidental
leading/trailing whitespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7dc363c-795d-425c-8103-7a30c77e229e
📒 Files selected for processing (16)
frontend/src/lib/i18n/locales/en.jsonfrontend/src/pages/authorize-page.tsxinternal/assets/migrations/000008_oidc_userinfo_profile.down.sqlinternal/assets/migrations/000008_oidc_userinfo_profile.up.sqlinternal/config/config.gointernal/controller/user_controller.gointernal/controller/well_known_controller.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gointernal/utils/user_utils.gointernal/utils/user_utils_test.gosql/oidc_queries.sqlsql/oidc_schemas.sql
✅ Files skipped from review due to trivial changes (5)
- internal/assets/migrations/000008_oidc_userinfo_profile.down.sql
- internal/controller/well_known_controller_test.go
- internal/assets/migrations/000008_oidc_userinfo_profile.up.sql
- internal/repository/models.go
- internal/middleware/context_middleware.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/well_known_controller.go
- internal/repository/oidc_queries.sql.go
- sql/oidc_schemas.sql
- internal/utils/user_utils_test.go
dosubot
Bot
added
size:XXL
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
2 times, most recently
from
613bfa2 to
9bb49da
Compare
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
9bb49da to
db9e0b5
Compare
dosubot
Bot
added
size:XL
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
3 times, most recently
from
15a4c8c to
d483c62
Compare
Collaborator
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Rycochet
reviewed
Apr 11, 2026
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
d483c62 to
868a2b9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/service/oidc_service.go (1)
707-717:⚠️ Potential issue | 🟡 MinorOmit empty phone/address claims instead of returning placeholders.
When
phoneis requested for a user with no stored phone number, this still emitsphone_number_verified:false. Likewise, the zero-value address serialized byStoreUserinfo()becomes"{}", which unmarshals successfully here and returnsaddress:{}. Both claims should be omitted when the underlying value is absent.Suggested fix
if slices.Contains(scopes, "phone") { - userInfo.PhoneNumber = user.PhoneNumber - verified := user.PhoneNumberVerified != 0 - userInfo.PhoneNumberVerified = &verified + if user.PhoneNumber != "" { + userInfo.PhoneNumber = user.PhoneNumber + verified := user.PhoneNumberVerified != 0 + userInfo.PhoneNumberVerified = &verified + } } if slices.Contains(scopes, "address") { var addr config.AddressClaim - if err := json.Unmarshal([]byte(user.Address), &addr); err == nil { + if err := json.Unmarshal([]byte(user.Address), &addr); err == nil && + (addr.Formatted != "" || + addr.StreetAddress != "" || + addr.Locality != "" || + addr.Region != "" || + addr.PostalCode != "" || + addr.Country != "") { userInfo.Address = &addr } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/oidc_service.go` around lines 707 - 717, The code currently sets phone and address claims even when underlying values are absent; change the phone block to only set userInfo.PhoneNumber and userInfo.PhoneNumberVerified when user.PhoneNumber is non-empty (e.g., if strings.TrimSpace(user.PhoneNumber) != "") and set the verified pointer only in that case; change the address block to skip setting userInfo.Address when user.Address is empty or when unmarshalling yields a zero-value config.AddressClaim (e.g., unmarshal into addr and then check reflect.DeepEqual(addr, config.AddressClaim{}) before assigning) so empty "{}" or empty strings result in omitted claims; update the slices.Contains(scopes, "phone") and slices.Contains(scopes, "address") branches accordingly and keep json.Unmarshal error handling as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/pages/authorize-page.tsx`:
- Around line 64-75: The new scope strings phoneScopeName,
phoneScopeDescription, addressScopeName, and addressScopeDescription referenced
in authorize-page (used where id: "phone" and id: "address" are defined) are
only present in en.json; add these same keys to all other locale JSON files used
by the app with appropriate translations (or safe fallbacks) so non-English
users see translated UI text—ensure key names match exactly and update each
locale file in frontend locales set.
In `@internal/config/config.go`:
- Around line 118-145: BootstrapApp.Setup() currently logs app.config and
app.context.users including raw PII from the new UserAttributes/AddressClaim and
User.Attributes; sanitize or exclude those before logging by producing a copy of
the config and user list with UserAttributes (and User.Attributes) removed or
redacted (e.g., set to nil/zero or replace sensitive fields with placeholders)
prior to the trace log call in BootstrapApp.Setup(), or implement a
Redact/Normalized method on UserAttributes/AddressClaim and call it on each user
before logging.
In `@internal/controller/user_controller_test.go`:
- Around line 362-381: Tests currently set Attributes directly on config.User
which bypasses the new Auth.UserAttributes -> utils.GetUsers() path; update the
test setup so users' attributes are provided via the same config source that
utils.GetUsers() reads (i.e., seed them into service.AuthServiceConfig.Users /
the auth config used by utils.GetUsers() rather than directly assigning
config.User.Attributes), then exercise the real login flow (including the TOTP
branch) and assert that the saved session or pending-session
(config.UserContext) contains the Name and Email values propagated from that
config; ensure both the normal login subtest and the TOTP subtest verify
persisted session/pending-session Name/Email rather than just checking cookie
presence.
In `@internal/service/oidc_service.go`:
- Around line 69-90: The UserinfoResponse currently emits email_verified (and
phone_number_verified) without backing state; update CompileUserinfo() to only
set/emit UserinfoResponse.EmailVerified and PhoneNumberVerified when there is an
explicit stored verification flag on the user (e.g.,
userAttributes.EmailVerified / userAttributes.PhoneNumberVerified) or omit those
claims entirely if unknown; locate the CompileUserinfo function and the
UserinfoResponse struct and change the logic that unconditionally sets
email_verified=true to read the verification booleans from the user record (or
skip adding the field) so relying parties only see verified=true when a real
verification flag exists.
---
Duplicate comments:
In `@internal/service/oidc_service.go`:
- Around line 707-717: The code currently sets phone and address claims even
when underlying values are absent; change the phone block to only set
userInfo.PhoneNumber and userInfo.PhoneNumberVerified when user.PhoneNumber is
non-empty (e.g., if strings.TrimSpace(user.PhoneNumber) != "") and set the
verified pointer only in that case; change the address block to skip setting
userInfo.Address when user.Address is empty or when unmarshalling yields a
zero-value config.AddressClaim (e.g., unmarshal into addr and then check
reflect.DeepEqual(addr, config.AddressClaim{}) before assigning) so empty "{}"
or empty strings result in omitted claims; update the slices.Contains(scopes,
"phone") and slices.Contains(scopes, "address") branches accordingly and keep
json.Unmarshal error handling as-is.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3061e1c2-8741-4590-97c4-ac1c4437e3f4
📒 Files selected for processing (20)
frontend/src/lib/i18n/locales/en.jsonfrontend/src/pages/authorize-page.tsxinternal/assets/migrations/000008_oidc_userinfo_profile.down.sqlinternal/assets/migrations/000008_oidc_userinfo_profile.up.sqlinternal/bootstrap/app_bootstrap.gointernal/config/config.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gointernal/service/oidc_service_test.gointernal/utils/loaders/loader_file.gointernal/utils/user_utils.gointernal/utils/user_utils_test.gosql/oidc_queries.sqlsql/oidc_schemas.sql
✅ Files skipped from review due to trivial changes (5)
- internal/utils/loaders/loader_file.go
- internal/assets/migrations/000008_oidc_userinfo_profile.down.sql
- internal/controller/well_known_controller_test.go
- internal/assets/migrations/000008_oidc_userinfo_profile.up.sql
- internal/repository/models.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/controller/well_known_controller.go
- frontend/src/lib/i18n/locales/en.json
- internal/utils/user_utils_test.go
- internal/utils/user_utils.go
- sql/oidc_schemas.sql
- sql/oidc_queries.sql
- internal/repository/oidc_queries.sql.go
- internal/middleware/context_middleware.go
- internal/controller/user_controller.go
steveiliop56
requested changes
Apr 11, 2026
Member
steveiliop56
left a comment
steveiliop56
left a comment
There was a problem hiding this comment.
Looks really good! Some small feedback and we can merge.
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
6517df9 to
29a8fd4
Compare
dosubot
Bot
added
size:L
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
29a8fd4 to
44c8d3c
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/config/config.go (1)
146-153: Consider addingIsEmpty()helper method for AddressClaim.To support checking whether an address has meaningful data (useful in
CompileUserinfoto avoid emitting"address": {}), a helper method could be added:Suggested helper
func (a AddressClaim) IsEmpty() bool { return a.Formatted == "" && a.StreetAddress == "" && a.Locality == "" && a.Region == "" && a.PostalCode == "" && a.Country == "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 146 - 153, Add an IsEmpty helper on the AddressClaim type to let callers detect when the address has no meaningful fields so CompileUserinfo (and other callers) can skip emitting an empty "address" object; implement AddressClaim.IsEmpty() to return true when all fields (Formatted, StreetAddress, Locality, Region, PostalCode, Country) are empty strings and update usages (e.g., CompileUserinfo) to call a.IsEmpty() before emitting the address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/config/config.go`:
- Around line 146-153: Add an IsEmpty helper on the AddressClaim type to let
callers detect when the address has no meaningful fields so CompileUserinfo (and
other callers) can skip emitting an empty "address" object; implement
AddressClaim.IsEmpty() to return true when all fields (Formatted, StreetAddress,
Locality, Region, PostalCode, Country) are empty strings and update usages
(e.g., CompileUserinfo) to call a.IsEmpty() before emitting the address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4270d289-045f-4f0c-ab56-b94a7ddef234
📒 Files selected for processing (19)
frontend/src/lib/i18n/locales/en.jsonfrontend/src/pages/authorize-page.tsxinternal/assets/migrations/000008_oidc_userinfo_profile.down.sqlinternal/assets/migrations/000008_oidc_userinfo_profile.up.sqlinternal/bootstrap/app_bootstrap.gointernal/config/config.gointernal/controller/user_controller.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gointernal/service/oidc_service_test.gointernal/utils/user_utils.gointernal/utils/user_utils_test.gosql/oidc_queries.sqlsql/oidc_schemas.sql
✅ Files skipped from review due to trivial changes (7)
- internal/bootstrap/app_bootstrap.go
- internal/assets/migrations/000008_oidc_userinfo_profile.down.sql
- frontend/src/pages/authorize-page.tsx
- internal/controller/well_known_controller_test.go
- internal/repository/models.go
- internal/service/oidc_service_test.go
- frontend/src/lib/i18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/controller/well_known_controller.go
- internal/assets/migrations/000008_oidc_userinfo_profile.up.sql
- sql/oidc_schemas.sql
- internal/repository/oidc_queries.sql.go
- sql/oidc_queries.sql
- internal/utils/user_utils.go
- internal/middleware/context_middleware.go
- internal/controller/user_controller.go
steveiliop56
requested changes
Apr 16, 2026
Member
steveiliop56
left a comment
steveiliop56
left a comment
There was a problem hiding this comment.
Final comments. Can you please confirm that attributes are set in the remote- headers in the authentication response?
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
7fae43e to
75c0f33
Compare
Member
|
@scottmckendry would you like to solve conflicts or should I? It's just the go module rename. |
scottmckendry
force-pushed
the
oidcc-user-attributes-scopes
branch
from
8b1fd66 to
6510920
Compare
Member
Author
|
@steveiliop56 - good to go. I've left in my forked branch because I'm not sure how to change upstream without having to re-open the PR.. and this one has quite a bit of history 😁 |
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
3 participants
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.
A decent sized refactor that will require us to update the appropriate documentation if it gets merged. I'm happy to do this.
Should be backwards compatible (I think) with the old users config.
5 new test passes:
New scopes in authorization:
Now, I'm fine if we decide we're not interested in supporting phone number & address. That would make this PR a little bit smaller as a result. Selfishly, I just wanted to turn all of these tests green 🚀
Summary by CodeRabbit
Release Notes
phoneandaddressOAuth scopes with visual indicators in the authorization flow