feat: add FHIR resource ID handling in session middleware and authentication#304
feat: add FHIR resource ID handling in session middleware and authentication#304gilanglahat22 wants to merge 10 commits intokonsulin-care:developfrom
Conversation
PR TypeEnhancement Description
|
| Relevant files | |||||||
|---|---|---|---|---|---|---|---|
| Enhancement |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a typed context key type and a new FHIR resource ID context key; SuperTokens hooks now resolve and embed a user's FHIR resource ID into the access-token payload; middleware extracts that FHIR resource ID (and typed role/uid keys) into request context; user usecase exposes a read-only FHIR ID lookup. ChangesFHIR resource ID propagation & typed context keys
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/delivery/http/middlewares/session.go (1)
130-143:⚠️ Potential issue | 🟡 MinorInconsistent context key population in
EnsureAnonymousSession.This middleware sets
keyRolesandkeyUIDbut doesn't set the newkeyFHIRResourceIdor the typedconstvars.CONTEXT_*keys thatSessionOptionalnow provides. Downstream handlers expectingCONTEXT_FHIR_RESOURCE_IDmay encounter missing context values.Proposed fix
if sess == nil { ctx := context.WithValue(r.Context(), keyRoles, []string{constvars.KonsulinRoleGuest}) ctx = context.WithValue(ctx, keyUID, "anonymous") + ctx = context.WithValue(ctx, keyFHIRResourceId, "") + + ctx = context.WithValue(ctx, constvars.CONTEXT_FHIR_ROLE, []string{constvars.KonsulinRoleGuest}) + ctx = context.WithValue(ctx, constvars.CONTEXT_UID, "anonymous") + ctx = context.WithValue(ctx, constvars.CONTEXT_FHIR_RESOURCE_ID, "") m.Log.Info("Ensuring anonymous session for request",
🧹 Nitpick comments (1)
internal/app/services/core/auth/auth_supertoken_impl.go (1)
60-70: Optional: Single-pass role check.Two loops iterate the roles array. Could consolidate into one pass, though current approach is readable.
Single-pass alternative
- for _, role := range roles { - if role == constvars.KonsulinRolePractitioner && initializedResources.PractitionerID != "" { - return fmt.Sprintf("Practitioner/%s", initializedResources.PractitionerID), nil - } - } - - for _, role := range roles { - if role == constvars.KonsulinRolePatient && initializedResources.PatientID != "" { - return fmt.Sprintf("Patient/%s", initializedResources.PatientID), nil - } - } + hasPractitioner, hasPatient := false, false + for _, role := range roles { + if role == constvars.KonsulinRolePractitioner { + hasPractitioner = true + } else if role == constvars.KonsulinRolePatient { + hasPatient = true + } + } + + if hasPractitioner && initializedResources.PractitionerID != "" { + return fmt.Sprintf("Practitioner/%s", initializedResources.PractitionerID), nil + } + if hasPatient && initializedResources.PatientID != "" { + return fmt.Sprintf("Patient/%s", initializedResources.PatientID), nil + }
| initializeResourceCtx, initializeResourceCtxCancel := context.WithDeadline(ctx, time.Now().Add(10*time.Second)) | ||
| defer initializeResourceCtxCancel() | ||
|
|
||
| initializedResources, err := uc.UserUsecase.InitializeNewUserFHIRResources(initializeResourceCtx, initFHIRResourcesInput) |
There was a problem hiding this comment.
Mau tanya mas @gilanglahat22 , Apakah ada alasan khusus kenapa memilih reuse function InitializeNewUserFHIRResources ketimbang membuat function baru yang khusus untuk melakukan query FHIR resources yang dimiliki oleh requester?
Kalau dilihat secara internal dari InitializeNewUserFHIRResources, focus function tersebut kan write operation, sedangkan yang dibutuhkan di bagian ini seharusnya read operations. Kemudian juga secara penamaan, function InitializeNewUserFHIRResources sudah jelas tujuannya tidak diperuntukan untuk mengambil FHIR resources berdasarkan SuperTokenUserID
|
|
||
| // getFhirResourceIdForUser determines the FHIR resource ID based on user's roles and FHIR resource IDs | ||
| // Priority: Practitioner > Patient > Person | ||
| func (uc *authUsecase) getFhirResourceIdForUser(ctx context.Context, userID string, roles []string) (string, error) { |
There was a problem hiding this comment.
this comment is not a change request to the code, just for discussion
mas @lamurian , aku recheck current issue yang akan di solve oleh PR ini dan secara teknis ini adalah function utama yang akan mengambil FHIR resources yang dimiliki oleh user. Secara teknis juga implementasinya sudah sesuai dengan issuenya.
Namun yang mau saya tanyakan adalah ini jadinya tetap tidak menghandle case dimana ketika user role adalah Practitioner, dia kan akan memiliki setidaknya 2 FHIR resources, Patient dan juga Practitioner. Untuk case demikian, apakah jadinya kita tidak akan embed semua resources milik user?
There was a problem hiding this comment.
Hi Mas @luckyAkbar, thank you for bringing this topic up. I believe it's best to best to embed all user resource IDs in the metadata. I expect the metadata for roles and user ID to be:
{
"roles": [
{
"name": "Patient",
"id": "PatientID"
},
{
"name": "Practitioner",
"id": "PractitionerID"
},
{
"name": "Clinic Admin",
"id": "ClinicAdminID"
},
{
"name": "Researcher",
"id": "ResearcherID"
}
]
}
When accessing resources related to each roles:
-
Patient$\to$ Access resourcePatient/PatientID -
Practitioner$\to$ Access resourcePractitioner/PractitionerID -
Clinic Admin$\to$ Access resourcePerson/ClinicAdminID -
Researcher$\to$ Access resourcePerson/ResearcherID
Mas @gilanglahat22 please note that for roles Clinic Admin and Researcher, the Person resource is used because there's no dedicated FHIR resource for these roles..
There was a problem hiding this comment.
this comment is not a change request to the code, just for discussion
mas @lamurian , aku recheck current issue yang akan di solve oleh PR ini dan secara teknis ini adalah function utama yang akan mengambil FHIR resources yang dimiliki oleh user. Secara teknis juga implementasinya sudah sesuai dengan issuenya.
Namun yang mau saya tanyakan adalah ini jadinya tetap tidak menghandle case dimana ketika user role adalah
Practitioner, dia kan akan memiliki setidaknya 2 FHIR resources,Patientdan jugaPractitioner. Untuk case demikian, apakah jadinya kita tidak akan embed semua resources milik user?
Setuju mas @luckyAkbar, memang harusnya tidak reuse InitializeNewUserFHIRResources karena fokusnya write operation. Sudah saya buatkan function baru khusus untuk read operation:
func (uc *userUsecase) LookupUserFHIRResourceIDs(ctx context.Context, input *LookupUserFHIRResourceIDsInput) (*InitializeNewUserFHIRResourcesOutput, error)Function ini hanya query existing FHIR resources berdasarkan SuperTokenUserID, tanpa create resource baru. Sudah menggunakan FindPractitionerByIdentifier, FindPatientByIdentifier, dan PersonFhirClient.Search.
There was a problem hiding this comment.
Hi Mas @luckyAkbar, thank you for bringing this topic up. I believe it's best to best to embed all user resource IDs in the metadata. I expect the metadata for roles and user ID to be:
{ "roles": [ { "name": "Patient", "id": "PatientID" }, { "name": "Practitioner", "id": "PractitionerID" }, { "name": "Clinic Admin", "id": "ClinicAdminID" }, { "name": "Researcher", "id": "ResearcherID" } ] }When accessing resources related to each roles:
Patient
→
Access resourcePatient/PatientIDPractitioner
→
Access resourcePractitioner/PractitionerIDClinic Admin
→
Access resourcePerson/ClinicAdminIDResearcher
→
Access resourcePerson/ResearcherIDMas @gilanglahat22 please note that for roles
Clinic AdminandResearcher, thePersonresource is used because there's no dedicated FHIR resource for these roles..
Terima kasih discussionnya mas @luckyAkbar dan mas @lamurian. Untuk sekarang implementasinya masih menggunakan priority-based approach (Practitioner > Patient > Person) untuk menentukan fhirResourceId yang di-embed di access token.
Untuk case user dengan multiple roles (e.g., Practitioner + Patient), saat ini kita hanya embed satu primary resource ID. Jika ke depan ada kebutuhan untuk embed semua resource IDs seperti yang mas @lamurian sarankan:
{
"roles": [
{"name": "Patient", "id": "PatientID"},
{"name": "Practitioner", "id": "PractitionerID"}
]
}Bisa di-refactor lagi. Untuk saat ini PR ini fokus fix bug validation error dulu. Apakah perlu dibuatkan issue terpisah untuk enhancement ini?
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
May 2, 2026 1:36p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
…ess token enrichment based on user roles and document the solution.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/app/services/core/auth/auth_supertoken_impl.go`:
- Around line 479-503: The code in authUsecase.CreateNewSession uses two
separate checks for fhirErr (if fhirErr != nil and if fhirErr == nil) after
calling uc.getFhirResourceIdForUser; simplify to a single if/else: call
uc.getFhirResourceIdForUser, then if fhirErr != nil log the error and set
accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" in the if
branch, else set accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId]
= fhirResourceId and log the success; remove the redundant second if and keep
the same logging/messages and keys (accessTokenPayload,
supertokenAccessTokenPayloadFhirResourceId,
supertokenAccessTokenPayloadRolesKey).
In `@internal/app/services/core/users/user_usecase_impl.go`:
- Around line 384-438: LookupUserFHIRResourceIDs currently swallows all errors
from uc.PractitionerFhirClient.FindPractitionerByIdentifier,
uc.PatientFhirClient.FindPatientByIdentifier, and uc.PersonFhirClient.Search and
always returns (output, nil), making callers' err checks (e.g.,
getFhirResourceIdForUser) useless; update LookupUserFHIRResourceIDs to return a
meaningful error when appropriate — e.g., if all three lookups failed (no IDs
found and each call returned a non-nil error) return a consolidated error (or
wrap the first error) so callers can handle failure, otherwise keep returning
(output, nil) when at least one lookup succeeded; reference the function name
LookupUserFHIRResourceIDs and the FHIR client calls
(FindPractitionerByIdentifier, FindPatientByIdentifier, PersonFhirClient.Search)
to locate where to implement this logic.
|
|
||
| // Get FHIR resource ID for the user based on their roles | ||
| ctx := context.Background() | ||
| fhirResourceId, fhirErr := uc.getFhirResourceIdForUser(ctx, userID, userRoles) | ||
| if fhirErr != nil { | ||
| uc.Log.Error("authUsecase.CreateNewSession error getting FHIR resource ID", | ||
| zap.String("user_id", userID), | ||
| zap.Error(fhirErr), | ||
| ) | ||
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" | ||
| } | ||
|
|
||
| if fhirErr == nil { | ||
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = fhirResourceId | ||
| uc.Log.Info("authUsecase.CreateNewSession added FHIR resource ID to access token", | ||
| zap.String("user_id", userID), | ||
| zap.String("fhir_resource_id", fhirResourceId), | ||
| ) | ||
| } | ||
| } else { | ||
| accessTokenPayload[supertokenAccessTokenPayloadRolesKey] = map[string]interface{}{ | ||
| supertokenAccessTokenPayloadRolesValueKey: []interface{}{constvars.KonsulinRoleGuest}, | ||
| } | ||
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify the fhirErr check — use else instead of two separate if blocks.
Lines 483-497 check fhirErr != nil and then separately check fhirErr == nil. Use a single if/else.
Suggested diff
fhirResourceId, fhirErr := uc.getFhirResourceIdForUser(ctx, userID, userRoles)
if fhirErr != nil {
uc.Log.Error("authUsecase.CreateNewSession error getting FHIR resource ID",
zap.String("user_id", userID),
zap.Error(fhirErr),
)
accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = ""
- }
-
- if fhirErr == nil {
+ } else {
accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = fhirResourceId
uc.Log.Info("authUsecase.CreateNewSession added FHIR resource ID to access token",
zap.String("user_id", userID),
zap.String("fhir_resource_id", fhirResourceId),
)
}📝 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.
| // Get FHIR resource ID for the user based on their roles | |
| ctx := context.Background() | |
| fhirResourceId, fhirErr := uc.getFhirResourceIdForUser(ctx, userID, userRoles) | |
| if fhirErr != nil { | |
| uc.Log.Error("authUsecase.CreateNewSession error getting FHIR resource ID", | |
| zap.String("user_id", userID), | |
| zap.Error(fhirErr), | |
| ) | |
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" | |
| } | |
| if fhirErr == nil { | |
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = fhirResourceId | |
| uc.Log.Info("authUsecase.CreateNewSession added FHIR resource ID to access token", | |
| zap.String("user_id", userID), | |
| zap.String("fhir_resource_id", fhirResourceId), | |
| ) | |
| } | |
| } else { | |
| accessTokenPayload[supertokenAccessTokenPayloadRolesKey] = map[string]interface{}{ | |
| supertokenAccessTokenPayloadRolesValueKey: []interface{}{constvars.KonsulinRoleGuest}, | |
| } | |
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" | |
| } | |
| // Get FHIR resource ID for the user based on their roles | |
| ctx := context.Background() | |
| fhirResourceId, fhirErr := uc.getFhirResourceIdForUser(ctx, userID, userRoles) | |
| if fhirErr != nil { | |
| uc.Log.Error("authUsecase.CreateNewSession error getting FHIR resource ID", | |
| zap.String("user_id", userID), | |
| zap.Error(fhirErr), | |
| ) | |
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" | |
| } else { | |
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = fhirResourceId | |
| uc.Log.Info("authUsecase.CreateNewSession added FHIR resource ID to access token", | |
| zap.String("user_id", userID), | |
| zap.String("fhir_resource_id", fhirResourceId), | |
| ) | |
| } | |
| } else { | |
| accessTokenPayload[supertokenAccessTokenPayloadRolesKey] = map[string]interface{}{ | |
| supertokenAccessTokenPayloadRolesValueKey: []interface{}{constvars.KonsulinRoleGuest}, | |
| } | |
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" | |
| } |
🤖 Prompt for AI Agents
In `@internal/app/services/core/auth/auth_supertoken_impl.go` around lines 479 -
503, The code in authUsecase.CreateNewSession uses two separate checks for
fhirErr (if fhirErr != nil and if fhirErr == nil) after calling
uc.getFhirResourceIdForUser; simplify to a single if/else: call
uc.getFhirResourceIdForUser, then if fhirErr != nil log the error and set
accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" in the if
branch, else set accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId]
= fhirResourceId and log the success; remove the redundant second if and keep
the same logging/messages and keys (accessTokenPayload,
supertokenAccessTokenPayloadFhirResourceId,
supertokenAccessTokenPayloadRolesKey).
|
From Gilang Ramadhan ‣ udah aku coba fix ya Mas @lamurian @luckyAkbar monggo minta tolong dicek 👍 |
… are found during user initialization.
… improve SuperTokens role creation error handling with nil checks.
… logic into dedicated methods and create PR reply documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/delivery/http/middlewares/session.go (1)
120-148:⚠️ Potential issue | 🟠 Major
EnsureAnonymousSessionmust setconstvars.CONTEXT_*keys to matchSessionOptional.This middleware only sets the deprecated local keys (
keyRoles,keyUID) but omitsconstvars.CONTEXT_UIDandconstvars.CONTEXT_FHIR_RESOURCE_ID. Multiple downstream handlers read from these constvars keys and will receive zero values when this middleware is used, causing bugs. Add:ctx = context.WithValue(ctx, constvars.CONTEXT_FHIR_ROLE, []string{constvars.KonsulinRoleGuest}) ctx = context.WithValue(ctx, constvars.CONTEXT_UID, "anonymous") ctx = context.WithValue(ctx, constvars.CONTEXT_FHIR_RESOURCE_ID, "")
🤖 Fix all issues with AI agents
In `@internal/app/delivery/http/middlewares/session.go`:
- Around line 85-89: The comment above the context.WithValue calls is garbled;
replace it with a clear sentence such as: "Typed context keys
(constvars.ContextKey) — these will replace the deprecated local ContextKey keys
above." Update the comment located near the context.WithValue(...) lines that
set constvars.CONTEXT_FHIR_ROLE, constvars.CONTEXT_UID, and
constvars.CONTEXT_FHIR_RESOURCE_ID so it clearly states that typed constvars
keys are used and will deprecate untyped string context keys.
In `@internal/app/services/core/auth/auth_supertoken_impl.go`:
- Around line 167-177: Remove the five calls to the undefined package-level
function ensureRoleExists() (the bare calls at the start of the list) and keep
only the method calls on the use case receiver uc (uc.ensureRoleExists(...));
specifically delete the calls to
ensureRoleExists(constvars.KonsulinRolePatient),
ensureRoleExists(constvars.KonsulinRoleGuest),
ensureRoleExists(constvars.KonsulinRoleClinicAdmin),
ensureRoleExists(constvars.KonsulinRolePractitioner), and
ensureRoleExists(constvars.KonsulinRoleResearcher) so only
uc.ensureRoleExists(...) and
uc.ensureRoleExists(constvars.KonsulinRoleSuperadmin) remain, relying on the
defined method uc.ensureRoleExists.
🧹 Nitpick comments (1)
internal/app/services/core/auth/auth_supertoken_impl.go (1)
244-263: DefaultuserRolesalways includes Patient, then appends SuperTokens roles — potential duplicate.
userRolesstarts as[Patient](line 244), then line 262 appends the user's actual roles. If the user already has the Patient role in SuperTokens, the slice becomes[Patient, Patient, ...]. This likely doesn't cause a bug sinceToogleByRolesandgetFhirResourceIdForUserjust scan for membership, but it's worth cleaning up.Proposed fix
- if userRolesResp.OK != nil { - // override the default roles with the user roles from supertokens - userRoles = append(userRoles, userRolesResp.OK.Roles...) - } + if userRolesResp.OK != nil && len(userRolesResp.OK.Roles) > 0 { + // override the default roles with the user roles from supertokens + userRoles = userRolesResp.OK.Roles + }
| ensureRoleExists(constvars.KonsulinRolePatient) | ||
| ensureRoleExists(constvars.KonsulinRoleGuest) | ||
| ensureRoleExists(constvars.KonsulinRoleClinicAdmin) | ||
| ensureRoleExists(constvars.KonsulinRolePractitioner) | ||
| ensureRoleExists(constvars.KonsulinRoleResearcher) | ||
| uc.ensureRoleExists(constvars.KonsulinRolePatient) | ||
| uc.ensureRoleExists(constvars.KonsulinRoleGuest) | ||
| uc.ensureRoleExists(constvars.KonsulinRoleClinicAdmin) | ||
| uc.ensureRoleExists(constvars.KonsulinRolePractitioner) | ||
| uc.ensureRoleExists(constvars.KonsulinRoleResearcher) | ||
| uc.ensureRoleExists(constvars.KonsulinRoleSuperadmin) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a package-level ensureRoleExists function (non-method)
echo "=== All ensureRoleExists declarations ==="
rg -n 'func\s+ensureRoleExists' --type=go
echo ""
echo "=== Method receiver version ==="
rg -n 'func\s+\(.*\)\s+ensureRoleExists' --type=goRepository: konsulin-care/konsulin-api
Length of output: 259
🏁 Script executed:
sed -n '160,185p' internal/app/services/core/auth/auth_supertoken_impl.goRepository: konsulin-care/konsulin-api
Length of output: 980
🏁 Script executed:
# Check for any imports or aliases of ensureRoleExists
rg -n 'ensureRoleExists' internal/app/services/core/auth/auth_supertoken_impl.go | head -20Repository: konsulin-care/konsulin-api
Length of output: 752
🏁 Script executed:
# Check if ensureRoleExists could be defined elsewhere in the file or imported
echo "=== Check file for all 'ensureRoleExists' mentions ==="
rg -c 'ensureRoleExists' internal/app/services/core/auth/auth_supertoken_impl.go
echo ""
echo "=== Check package-level functions in the file ==="
rg -n '^func [a-zA-Z]' internal/app/services/core/auth/auth_supertoken_impl.go | head -10
echo ""
echo "=== Verify if code compiles ==="
cd internal/app/services/core/auth && go build -o /tmp/test 2>&1 | grep -i "ensureRoleExists\|undefined" || echo "No compilation errors related to ensureRoleExists"Repository: konsulin-care/konsulin-api
Length of output: 516
Remove undefined ensureRoleExists() calls — code does not compile.
Lines 167–171 call bare ensureRoleExists() without a receiver, but no package-level function with that name exists. Only the method uc.ensureRoleExists() (defined at line 183) is available. These calls cause 5 compilation errors. Remove lines 167–171:
Proposed fix
- ensureRoleExists(constvars.KonsulinRolePatient)
- ensureRoleExists(constvars.KonsulinRoleGuest)
- ensureRoleExists(constvars.KonsulinRoleClinicAdmin)
- ensureRoleExists(constvars.KonsulinRolePractitioner)
- ensureRoleExists(constvars.KonsulinRoleResearcher)
uc.ensureRoleExists(constvars.KonsulinRolePatient)
uc.ensureRoleExists(constvars.KonsulinRoleGuest)
uc.ensureRoleExists(constvars.KonsulinRoleClinicAdmin)
uc.ensureRoleExists(constvars.KonsulinRolePractitioner)
uc.ensureRoleExists(constvars.KonsulinRoleResearcher)
uc.ensureRoleExists(constvars.KonsulinRoleSuperadmin)📝 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.
| ensureRoleExists(constvars.KonsulinRolePatient) | |
| ensureRoleExists(constvars.KonsulinRoleGuest) | |
| ensureRoleExists(constvars.KonsulinRoleClinicAdmin) | |
| ensureRoleExists(constvars.KonsulinRolePractitioner) | |
| ensureRoleExists(constvars.KonsulinRoleResearcher) | |
| uc.ensureRoleExists(constvars.KonsulinRolePatient) | |
| uc.ensureRoleExists(constvars.KonsulinRoleGuest) | |
| uc.ensureRoleExists(constvars.KonsulinRoleClinicAdmin) | |
| uc.ensureRoleExists(constvars.KonsulinRolePractitioner) | |
| uc.ensureRoleExists(constvars.KonsulinRoleResearcher) | |
| uc.ensureRoleExists(constvars.KonsulinRoleSuperadmin) | |
| uc.ensureRoleExists(constvars.KonsulinRolePatient) | |
| uc.ensureRoleExists(constvars.KonsulinRoleGuest) | |
| uc.ensureRoleExists(constvars.KonsulinRoleClinicAdmin) | |
| uc.ensureRoleExists(constvars.KonsulinRolePractitioner) | |
| uc.ensureRoleExists(constvars.KonsulinRoleResearcher) | |
| uc.ensureRoleExists(constvars.KonsulinRoleSuperadmin) |
🤖 Prompt for AI Agents
In `@internal/app/services/core/auth/auth_supertoken_impl.go` around lines 167 -
177, Remove the five calls to the undefined package-level function
ensureRoleExists() (the bare calls at the start of the list) and keep only the
method calls on the use case receiver uc (uc.ensureRoleExists(...));
specifically delete the calls to
ensureRoleExists(constvars.KonsulinRolePatient),
ensureRoleExists(constvars.KonsulinRoleGuest),
ensureRoleExists(constvars.KonsulinRoleClinicAdmin),
ensureRoleExists(constvars.KonsulinRolePractitioner), and
ensureRoleExists(constvars.KonsulinRoleResearcher) so only
uc.ensureRoleExists(...) and
uc.ensureRoleExists(constvars.KonsulinRoleSuperadmin) remain, relying on the
defined method uc.ensureRoleExists.
There was a problem hiding this comment.
Removed the redundant and undefined bare function calls to ensureRoleExists(...) in internal/app/services/core/auth/auth_supertoken_impl.go (lines 167-171). The code now correctly uses only the method calls uc.ensureRoleExists(...).
There was a problem hiding this comment.
Thank you for addressing this concern Mas @gilanglahat22. But there seems to be other underlying problems that cause compilation failure, as detected in our PR screening.
…nsureRoleExists` method calls to use the receiver.
… across various modules and refactor auth supertoken functions.
|
|
|
||
| if fhirErr == nil { | ||
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = fhirResourceId | ||
| uc.Log.Info("authUsecase.CreateNewSession added FHIR resource ID to access token", | ||
| zap.String("user_id", userID), | ||
| zap.String("fhir_resource_id", fhirResourceId), | ||
| ) |
There was a problem hiding this comment.
untuk block if fhirErr == nil ini juga menurut saya tidak diperlukan, mas, karena sudah pasti value fhirErr adalah nil, kan.
Lebih baik if checking nya itu mengecek fhirResourceId, apakah dia nil atau apakah mengandung value tertentu yang dianggap valid untuk digunakan di accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId]
jadi semisal contohnya
if fhirResourceId != "" {
// assignments here
}
|
|
||
| if userID == "" { | ||
| accessTokenPayload[supertokenAccessTokenPayloadRolesKey] = map[string]interface{}{ | ||
| supertokenAccessTokenPayloadRolesValueKey: []interface{}{constvars.KonsulinRoleGuest}, | ||
| } | ||
| accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = "" |
There was a problem hiding this comment.
kemudian untuk bagian ini, sepemahaman saya adalah ketika userID dari supertokens tidak ditemukan, maka payload di accessTokenPayload hanya akan diinisiasi sbg role Guest dan juga resource id nya di set ke empty string.
Supaya untuk mempermudah kodenya di baca, kita bisa pakai gaya penulisan code negative space programming. Karena case userID == "" ini dianggap sebagai error / default behaviour, kita bisa langsung melakukan menambahkan return line (Seperti yang ada di baris 578) untuk langsung skip semua kode yang ada di bawahnya.
Setelah itu, else block yang menempel (line 539) bisa langsung kita hapus karena ketika case userID == "" kode akan langsung exit di block tersebut. Akibatnya kode yang ditulis itu lebih rata ke kiri ketimbang berada di dalam nested if yang dalam.
Hal yang sama juga saya sarankan untuk menghilangkan block else di baris ke 570 - 576 dengan cara lakukan error check yang lebih eksplisit dari hasil operasi function line 540. Gambaran saya seperti ini
rolesResp, err := userroles.GetRolesForUser(tenantId, userID)
if err != nil || roleresp.OK == nil {
// bisa di log dulu kenapa supertokens error
accessTokenPayload[supertokenAccessTokenPayloadRolesKey] = map[string]interface{}{
supertokenAccessTokenPayloadRolesValueKey: []interface{}{constvars.KonsulinRoleGuest},
}
accessTokenPayload[supertokenAccessTokenPayloadFhirResourceId] = ""
return originalCreateSession....
}
// rest of the code here (line 542 - last return line
Menurut saya ini gak mengubah flow execution yang sudah di buat, melainkan hanya melakukan early exit ketika kode mengalami kondisi yang tidak diinginkan / tidak ideal dan sekaligus mengurangi nested kode, mas @gilanglahat22 . Please LMK your response ya mas
|
From Gilang Ramadhan ‣ Sure, I'll fix it right now. Sorry, I'm just getting to it |
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 18 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/services/core/webhook/auth_helper.go (1)
27-57:⚠️ Potential issue | 🟠 MajorUnify API-key context key reads with the typed constant.
This function and
webhook_controller.goread"api_key_auth"as raw strings while other middleware code uses the typed constantContextAPIKeyAuth. Since the constant is defined in the middleware, inconsistent raw-string readers create a maintenance hazard: future changes to the constant value won't automatically propagate to these locations. Apply the constant in bothauth_helper.goandwebhook_controller.gofor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/services/core/webhook/auth_helper.go` around lines 27 - 57, The code in extractAuthContext (and the reader in webhook_controller.go) uses the raw string "api_key_auth"; change those to use the typed constant ContextAPIKeyAuth (the exported constant from the middleware package) so the key is consistent with the middleware definition. Update the imports in internal/app/services/core/webhook/auth_helper.go and webhook_controller.go to include the middleware package (or the package that defines ContextAPIKeyAuth), replace ctx.Value("api_key_auth") with ctx.Value(middleware.ContextAPIKeyAuth) in extractAuthContext, and make the analogous replacement where webhook_controller.go reads the API-key auth flag.
🤖 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/app/delivery/http/middlewares/session.go`:
- Around line 81-88: EnsureAnonymousSession is only seeding the deprecated local
context keys (keyUID, keyRoles, keyFHIRResourceId), causing handlers that expect
the typed contract from SessionOptional to miss auth info; update
EnsureAnonymousSession to also set the typed keys constvars.CONTEXT_UID,
constvars.CONTEXT_FHIR_ROLE, and constvars.CONTEXT_FHIR_RESOURCE_ID on the
request context (in addition to the existing local keys) so anonymous flows
publish the same context contract as SessionOptional; locate the seeding logic
in EnsureAnonymousSession and add context.WithValue calls that mirror the ones
in SessionOptional using those constvars constants.
In `@internal/app/services/core/auth/auth_supertoken_impl.go`:
- Around line 237-259: The code currently seeds userRoles with Patient and then
appends fetched roles in SupertokenCreateCode, which forces Patient onto
existing users; change the logic in the block that calls
userroles.GetRolesForUser (and where userRoles is set) to replace the default
when a successful response returns roles: set userRoles = userRolesResp.OK.Roles
instead of appending; optionally keep the default Patient only when the fetch
succeeds but returns zero roles (i.e., if userRolesResp.OK.Roles is empty, leave
userRoles as []string{constvars.KonsulinRolePatient}); ensure this change is
applied around the userRecord != nil branch handling in auth_supertoken_impl.go.
- Around line 168-188: ensureRoleExists currently swallows errors, so
InitializeSupertoken may report success even if critical roles weren't created;
change ensureRoleExists(role string) to return error (or bool) and return the
userroles.CreateNewRoleOrAddPermissions error when it fails, and update
InitializeSupertoken to check each ensureRoleExists call (for
KonsulinRolePatient, KonsulinRoleGuest, KonsulinRoleClinicAdmin,
KonsulinRolePractitioner, KonsulinRoleResearcher, KonsulinRoleSuperadmin) and
return an error if any role creation fails; preserve/info-log the "role already
exists" path but propagate failures up so InitializeSupertoken returns a non-nil
error instead of logging only.
---
Outside diff comments:
In `@internal/app/services/core/webhook/auth_helper.go`:
- Around line 27-57: The code in extractAuthContext (and the reader in
webhook_controller.go) uses the raw string "api_key_auth"; change those to use
the typed constant ContextAPIKeyAuth (the exported constant from the middleware
package) so the key is consistent with the middleware definition. Update the
imports in internal/app/services/core/webhook/auth_helper.go and
webhook_controller.go to include the middleware package (or the package that
defines ContextAPIKeyAuth), replace ctx.Value("api_key_auth") with
ctx.Value(middleware.ContextAPIKeyAuth) in extractAuthContext, and make the
analogous replacement where webhook_controller.go reads the API-key auth flag.
🪄 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: 5951d6c0-7133-48b3-be2d-eb0bc7bd6930
📒 Files selected for processing (6)
internal/app/delivery/http/controllers/webhook_controller.gointernal/app/delivery/http/middlewares/session.gointernal/app/delivery/http/routers/auth_router_test.gointernal/app/services/core/auth/auth_supertoken_impl.gointernal/app/services/core/payments/payment_usecase_impl.gointernal/app/services/core/webhook/auth_helper.go
| ctx := context.WithValue(r.Context(), keyRoles, roles) | ||
| ctx = context.WithValue(ctx, keyUID, uid) | ||
| ctx = context.WithValue(ctx, keyFHIRResourceId, fhirResourceId) | ||
|
|
||
| // Typed context keys (constvars.ContextKey) - these will replace the deprecated local ContextKey keys above. | ||
| ctx = context.WithValue(ctx, constvars.CONTEXT_FHIR_ROLE, roles) | ||
| ctx = context.WithValue(ctx, constvars.CONTEXT_UID, uid) | ||
| ctx = context.WithValue(ctx, constvars.CONTEXT_FHIR_RESOURCE_ID, fhirResourceId) |
There was a problem hiding this comment.
Keep the anonymous fallback on the same typed context contract.
SessionOptional now publishes constvars.CONTEXT_UID, constvars.CONTEXT_FHIR_ROLE, and constvars.CONTEXT_FHIR_RESOURCE_ID, but EnsureAnonymousSession below still seeds only the deprecated local keys. Any handler that relies on the anonymous middleware path without passing through SessionOptional will miss the new auth context entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/app/delivery/http/middlewares/session.go` around lines 81 - 88,
EnsureAnonymousSession is only seeding the deprecated local context keys
(keyUID, keyRoles, keyFHIRResourceId), causing handlers that expect the typed
contract from SessionOptional to miss auth info; update EnsureAnonymousSession
to also set the typed keys constvars.CONTEXT_UID, constvars.CONTEXT_FHIR_ROLE,
and constvars.CONTEXT_FHIR_RESOURCE_ID on the request context (in addition to
the existing local keys) so anonymous flows publish the same context contract as
SessionOptional; locate the seeding logic in EnsureAnonymousSession and add
context.WithValue calls that mirror the ones in SessionOptional using those
constvars constants.
| ensureRoleExists(constvars.KonsulinRolePatient) | ||
| ensureRoleExists(constvars.KonsulinRoleGuest) | ||
| ensureRoleExists(constvars.KonsulinRoleClinicAdmin) | ||
| ensureRoleExists(constvars.KonsulinRolePractitioner) | ||
| ensureRoleExists(constvars.KonsulinRoleResearcher) | ||
| ensureRoleExists(constvars.KonsulinRoleSuperadmin) | ||
|
|
||
| log.Println("Successfully initialized supertokens SDK") | ||
| return nil | ||
| } | ||
|
|
||
| func ensureRoleExists(role string) { | ||
| resp, err := userroles.CreateNewRoleOrAddPermissions(role, []string{}, nil) | ||
| if err != nil { | ||
| log.Println("Error creating 'guest' role", zap.Error(err)) | ||
| log.Printf("Error creating '%s' role: %v\n", role, err) | ||
| return | ||
| } | ||
| if !resp.OK.CreatedNewRole { | ||
| log.Println("'guest' role already exists") | ||
| if resp.OK != nil && !resp.OK.CreatedNewRole { | ||
| log.Printf("'%s' role already exists\n", role) | ||
| } | ||
| } |
There was a problem hiding this comment.
Propagate required-role bootstrap failures.
ensureRoleExists only logs errors, so InitializeSupertoken can return success while required roles were never created. The first login or role-assignment path then fails later at runtime instead of failing fast during startup.
Suggested fix
- ensureRoleExists(constvars.KonsulinRolePatient)
- ensureRoleExists(constvars.KonsulinRoleGuest)
- ensureRoleExists(constvars.KonsulinRoleClinicAdmin)
- ensureRoleExists(constvars.KonsulinRolePractitioner)
- ensureRoleExists(constvars.KonsulinRoleResearcher)
- ensureRoleExists(constvars.KonsulinRoleSuperadmin)
+ for _, role := range []string{
+ constvars.KonsulinRolePatient,
+ constvars.KonsulinRoleGuest,
+ constvars.KonsulinRoleClinicAdmin,
+ constvars.KonsulinRolePractitioner,
+ constvars.KonsulinRoleResearcher,
+ constvars.KonsulinRoleSuperadmin,
+ } {
+ if err := ensureRoleExists(role); err != nil {
+ return err
+ }
+ }
@@
-func ensureRoleExists(role string) {
+func ensureRoleExists(role string) error {
resp, err := userroles.CreateNewRoleOrAddPermissions(role, []string{}, nil)
if err != nil {
- log.Printf("Error creating '%s' role: %v\n", role, err)
- return
+ return fmt.Errorf("create role %q: %w", role, err)
}
+ if resp.OK == nil {
+ return fmt.Errorf("create role %q: nil response", role)
+ }
if resp.OK != nil && !resp.OK.CreatedNewRole {
log.Printf("'%s' role already exists\n", role)
}
+ return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/app/services/core/auth/auth_supertoken_impl.go` around lines 168 -
188, ensureRoleExists currently swallows errors, so InitializeSupertoken may
report success even if critical roles weren't created; change
ensureRoleExists(role string) to return error (or bool) and return the
userroles.CreateNewRoleOrAddPermissions error when it fails, and update
InitializeSupertoken to check each ensureRoleExists call (for
KonsulinRolePatient, KonsulinRoleGuest, KonsulinRoleClinicAdmin,
KonsulinRolePractitioner, KonsulinRoleResearcher, KonsulinRoleSuperadmin) and
return an error if any role creation fails; preserve/info-log the "role already
exists" path but propagate failures up so InitializeSupertoken returns a non-nil
error instead of logging only.
| // by default, always assumes the user roles is Patient | ||
| // because if the user is the first time user, supertokens is not yet assigns any roles to the user. | ||
| // We're also assumes the registered user is always have a Patient role. | ||
| userRoles := []string{ | ||
| constvars.KonsulinRolePatient, | ||
| } | ||
| userID := "" | ||
|
|
||
| if userRecord != nil { | ||
| userID = userRecord.ID | ||
| userRolesResp, err := userroles.GetRolesForUser(uc.InternalConfig.Supertoken.KonsulinTenantID, userRecord.ID) | ||
| if err != nil { | ||
| uc.Log.Error("authUsecase.SupertokenCreateCode failed to fetch user roles by user ID", | ||
| zap.String("user_id", userRecord.ID), | ||
| zap.Error(err), | ||
| ) | ||
| return response, err | ||
| } | ||
|
|
||
| if userRolesResp.OK != nil { | ||
| // override the default roles with the user roles from supertokens | ||
| userRoles = append(userRoles, userRolesResp.OK.Roles...) | ||
| } |
There was a problem hiding this comment.
Don't force Patient onto existing users.
userRoles starts with Patient, but Line 258 appends stored roles instead of replacing them. Existing Clinic Admin / Researcher users will still toggle PatientRolesExists here and can get the wrong FHIR resources initialized.
Suggested fix
userRoles := []string{
constvars.KonsulinRolePatient,
}
@@
- if userRolesResp.OK != nil {
- // override the default roles with the user roles from supertokens
- userRoles = append(userRoles, userRolesResp.OK.Roles...)
+ if userRolesResp.OK != nil && len(userRolesResp.OK.Roles) > 0 {
+ // Keep the default only for brand-new users that still have no stored roles.
+ userRoles = append([]string{}, userRolesResp.OK.Roles...)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/app/services/core/auth/auth_supertoken_impl.go` around lines 237 -
259, The code currently seeds userRoles with Patient and then appends fetched
roles in SupertokenCreateCode, which forces Patient onto existing users; change
the logic in the block that calls userroles.GetRolesForUser (and where userRoles
is set) to replace the default when a successful response returns roles: set
userRoles = userRolesResp.OK.Roles instead of appending; optionally keep the
default Patient only when the fetch succeeds but returns zero roles (i.e., if
userRolesResp.OK.Roles is empty, leave userRoles as
[]string{constvars.KonsulinRolePatient}); ensure this change is applied around
the userRecord != nil branch handling in auth_supertoken_impl.go.
3 participants
Implementation Solutions
This document describes the solution implementations for the following issues in Konsulin API.
Issue 1: Enrich SuperTokens Profile Metadata with Roles and FHIR Resource IDs
Problem
SuperTokens profiles did not provide sufficient metadata for direct authorization checks. Whenever resource ownership validation was needed, the system had to perform additional queries to the database or FHIR server, causing overhead and latency.
Solution
Profile metadata was enriched with:
Patient/{ID},Practitioner/{ID}, orPerson/{ID}based on the user’s primary roleThis metadata is embedded in the SuperTokens access token payload so it can be read without extra queries.
Implementation Details
1. Access Token Payload Enrichment
File:
internal/app/services/core/auth/auth_supertoken_impl.goNew constant:
New helper:
getFhirResourceIdForUser(ctx, userID, roles) (string, error)It derives the FHIR resource ID from the user’s roles with this priority:
Practitioner/{ID}(highest)Patient/{ID}Person/{ID}Session creation change: The
CreateNewSessionoverride addsfhirResourceIdto the access token payload by callinggetFhirResourceIdForUser()and settingaccessTokenPayload[supertokenAccessTokenPayloadFhirResourceId].2. Middleware Enhancement
File:
internal/app/delivery/http/middlewares/session.goNew constants:
keyFHIRResourceId,supertokenAccessTokenPayloadFhirResourceIdSessionOptional middleware: Reads
fhirResourceIdfrom the access token payload and stores it in the request context (keyFHIRResourceId,CONTEXT_FHIR_RESOURCE_ID).3. Context Constant
File:
internal/pkg/constvars/internal_app.goAccess Token Payload Shape
{ "st-role": { "v": ["Patient", "Practitioner"] }, "fhirResourceId": "Practitioner/abc123" }Benefits
Usage (middleware or handler)
Files Modified
internal/app/services/core/auth/auth_supertoken_impl.go– constant,getFhirResourceIdForUser(),CreateNewSessionoverrideinternal/app/delivery/http/middlewares/session.go– constants,SessionOptionalreading/storingfhirResourceIdinternal/pkg/constvars/internal_app.go–CONTEXT_FHIR_RESOURCE_IDTesting
getFhirResourceIdForUser()for different role combinations and priority (Practitioner > Patient > Person).Issue 2: Use FHIR Bundle for Batch PractitionerRole Availability Updates
Problem
The PractitionerAvailabilityEditor updated multiple PractitionerRole resources one-by-one in a loop. If one update failed mid-batch, earlier updates were already persisted, leading to an inconsistent state that was hard to recover.
Solution
The availability update flow was refactored to use a FHIR Bundle transaction: the client sends one request with all PractitionerRole updates in a Bundle, and the backend processes it atomically (all-or-nothing). The backend already had the infrastructure to support FHIR Bundle transactions.
Implementation Details (Backend Support)
1. Bundle FHIR Client
File:
internal/app/services/fhir_spark/bundle/bundle_fhir_impl.go2. Middleware Auth – Bundle Validation
File:
internal/app/delivery/http/middlewares/auth.goAuthusesscanBundle()to validate every entry in the Bundle (resourceType, RBAC, resource ownership, structure). If any entry fails, the whole request is rejected.3. Middleware Bridge – Bundle Proxy
File:
internal/app/delivery/http/middlewares/proxy.goBridgeforwards the request (including Bundle) to the FHIR server withContent-Type: application/fhir+json.4. Router
File:
internal/app/delivery/http/routers/router.goFHIR Bundle Transaction Shape
{ "resourceType": "Bundle", "type": "transaction", "entry": [ { "request": { "method": "PUT", "url": "PractitionerRole/{id1}" }, "resource": { "resourceType": "PractitionerRole", "id": "{id1}", "availableTime": [ { "daysOfWeek": ["mon", "tue"], "availableStartTime": "09:00:00", "availableEndTime": "17:00:00" } ] } } ] }Request and Validation Flow
POST /fhir(Bundle) → Auth → Bridge → FHIR serverscanBundle()on each entry (RBAC + ownership). If all pass → Bridge forwards; otherwise → error.Benefits
Flow Comparison
Before (sequential)
Update #1 ✅ → #2 ✅ → #3 ❌ → inconsistent state; user must fix manually.
After (Bundle)
Single Bundle → all succeed or all fail → consistent state; user can retry the whole operation.
Error Handling
If any entry fails validation or the FHIR server rejects the transaction, the backend can return an OperationOutcome; the FHIR server rolls back the whole transaction. The frontend gets a single error and can retry the full batch.
Files Involved
internal/app/services/fhir_spark/bundle/bundle_fhir_impl.go– Bundle clientinternal/app/delivery/http/middlewares/auth.go–scanBundle()internal/app/delivery/http/middlewares/proxy.go– Bridgeinternal/app/delivery/http/routers/router.go–/fhirrouteinternal/pkg/fhir_dto/bundle.go– Bundle DTOFrontend implementation:
konsulin-app(schedule API and practitioner-availability-editor).Testing
scanBundle()with different Bundle structures and error cases.References
Summary by CodeRabbit
New Features
Refactor