-
Notifications
You must be signed in to change notification settings - Fork 155
[DERCBOT-1772] Error while switching namespaces #1976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[DERCBOT-1772] Error while switching namespaces #1976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes a 500 error that occurred when switching namespaces in the application. The root cause was improper casting of RoutingContext directly to UserContextInternal. The fix properly uses context.userContext() before casting and persists the updated TockUser in the session.
Key Changes:
- Fixed namespace switching to use
context.userContext()instead of castingRoutingContextdirectly - Added session persistence for the updated
TockUserduring namespace switches - Implemented a guard in CAS authentication to reuse existing
TockUserfrom session and prevent overwriting during CAS upgrade
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| shared/src/main/kotlin/security/auth/CASAuthProvider.kt | Added session check logic to preserve TockUser during namespace switches and prevent CAS from overwriting it; includes KTLint formatting changes |
| nlp/admin/server/src/main/kotlin/AdminVerticle.kt | Fixed namespace switching endpoint to properly update user context and persist to session; extensive KTLint formatting changes throughout the file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (sessionTockUser != null) { | ||
| // If a TockUser is already in session (ex: after a namespace switch), reuse it and don't overwrite it. | ||
| (rc.userContext() as UserContextInternal).setUser(sessionTockUser) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cast to UserContextInternal is fragile and depends on Vert.x internal implementation details. The io.vertx.ext.web.impl package is internal and could change without notice. Consider checking if userContext() is an instance of UserContextInternal before casting, or document this as a known limitation that may break with future Vert.x versions.
| val updated = context.user!!.copy(namespace = n) | ||
|
|
||
| // Update the user through userContext() and persist in session so next calls see the change. | ||
| (context.userContext() as UserContextInternal).setUser(updated) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cast to UserContextInternal is fragile and depends on Vert.x internal implementation details. The io.vertx.ext.web.impl package is internal and could change without notice. Consider checking if userContext() is an instance of UserContextInternal before casting, or document this as a known limitation that may break with future Vert.x versions.
| (context.userContext() as UserContextInternal).setUser(updated) | |
| val userContext = context.userContext() | |
| if (userContext is UserContextInternal) { | |
| userContext.setUser(updated) | |
| } |
| front.setCurrentNamespace(context.userLogin, n) | ||
| (context as UserContextInternal).setUser(context.user!!.copy(namespace = n)) | ||
|
|
||
| val updated = context.user!!.copy(namespace = n) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copy() method on a data class creates a shallow copy. Since TockUser has a delegate field that is a mutable User object, and the authorizations are set in the init block, using copy(namespace = n) will copy the existing delegate and not reinitialize the authorizations. This could potentially lead to inconsistent state if the namespace change should affect authorizations. Verify that preserving the existing delegate and authorizations is the intended behavior.
| val updated = context.user!!.copy(namespace = n) | ||
|
|
||
| // Update the user through userContext() and persist in session so next calls see the change. | ||
| (context.userContext() as UserContextInternal).setUser(updated) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null safety issue: context.session() returns a nullable session. If the session is null, the put() operation will be silently skipped, and the namespace change won't be persisted. Consider checking if the session is null and handling this case explicitly, or at minimum adding a comment acknowledging this limitation.
| (context.userContext() as UserContextInternal).setUser(updated) | |
| (context.userContext() as UserContextInternal).setUser(updated) | |
| // Note: if session() is null, this call is skipped and the namespace change is not persisted across requests. |
| if (sessionTockUser != null) { | ||
| // If a TockUser is already in session (ex: after a namespace switch), reuse it and don't overwrite it. | ||
| (rc.userContext() as UserContextInternal).setUser(sessionTockUser) | ||
| rc.next() |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential race condition: The check for sessionTockUser and setting it in userContext() is not atomic. If multiple requests are processed concurrently for the same user during a namespace switch, there could be inconsistent state. Consider whether this scenario is possible in your application architecture and if additional synchronization is needed.
|
@scezen I add copilot review. You can close all useless comments ;) |
Ticket : DERCBOT-1772
Details
RoutingContexttoUserContextInternal, now usingcontext.userContext()to update the user.TockUserin session (tockUser) during namespace switch so the new namespace is applied immediately (no refresh needed).tockUseralready exists in session, we reuse it and avoid overwriting it during CAS upgrade.Note :