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 9 commits intokonsulin-care:developfrom
Conversation
PR TypeEnhancement Description
|
| Relevant files | |||||||
|---|---|---|---|---|---|---|---|
| Enhancement |
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds lookup and propagation of a user's FHIR resource ID: new context key, middleware extracts Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. 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?
|
Here's the code health analysis summary for commits Analysis Summary
|
…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 |
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