Conversation
Since we don't want admins to be actual users, and we intend to only have a single admin machine, a pseudo user is good enough. Perhaps, a different StateFlow can be added for admins to be able to receive other stats too.
- Rest of admin routes finished - Fixed body size limit falsely triggering - New services: SubjectService, TeamService - No transaction in context fixes - Handle invalid signature string on admin auth verification - Moved listing users to UsersService
There was a problem hiding this comment.
Pull request overview
Adds admin authentication via SSH signatures and introduces /admin/* endpoints (plus admin notifications WS), along with new supporting services and expanded test coverage.
Changes:
- Added
AdminAuthService(challenge + signature verification) and wired admin bearer auth + admin routes. - Introduced CRUD-style services for teams/subjects/electives and admin APIs to manage users and selections.
- Added CIDR/IP utilities and extensive new Kotlin tests for admin flows and new services.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/api/types.ts | Renames user selection response type to match proto (StudentSelections). |
| web/src/api/managers/SelectionManager.ts | Updates REST decoding to new StudentSelections type. |
| web/src/api/index.ts | Re-exports the renamed StudentSelections type. |
| common/src/test/kotlin/utils/IPAddressTest.kt | Adds parsing/matching tests for new IP/CIDR utilities. |
| common/src/main/proto/api.proto | Adds AdminService messages and renames selections message. |
| common/src/main/kotlin/utils/IPAddress.kt | Introduces IP/CIDR parsing and matching utilities. |
| common/src/main/kotlin/db/DAOProtoHelpers.kt | Adjusts proto mapping for student teams access changes. |
| common/src/main/kotlin/db/DAO.kt | Adds helpers (from(...), exists(...)), changes student teams representation. |
| common/src/main/kotlin/Exceptions.kt | Adds TEAM to NotFoundEntity. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/services/TestUsersService.kt | Extends test service interface coverage for new user operations. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/services/TestTeamService.kt | Adds test double for TeamService. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/services/TestSubjectService.kt | Adds test double for SubjectService. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/services/TestElectiveService.kt | Extends test double for ElectiveService CRUD-ish methods. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/services/TestElectiveSelectionService.kt | Extends test double with force-set selections method. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/services/MockUtils.kt | Renames DAO mocking helpers for broader use. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/UsersServiceImplTest.kt | Adds tests for delete/update/setPassword admin-style operations. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/UsersRoutesTest.kt | Updates test to new proto type name for selections. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/TestUtils.kt | Adds authenticated protobuf PATCH helper. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/TeamServiceImplTest.kt | Adds TeamService integration tests. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/SubjectServiceImplTest.kt | Adds SubjectService integration tests. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/ElectiveServiceImplTest.kt | Adds ElectiveService CRUD-ish tests and set-subjects tests. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/ElectiveSelectionServiceImplTest.kt | Adds tests for force-setting student selections. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/ApplicationTest.kt | Wires new services/mocks into the test DI container. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/AdminUsersRoutesTest.kt | Adds admin route tests for user endpoints. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/AdminTeamsRoutesTest.kt | Adds admin route tests for team endpoints. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/AdminSubjectsRoutesTest.kt | Adds admin route tests for subject endpoints. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/AdminSelectionsRoutesTest.kt | Adds admin route tests for selections endpoints. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/AdminElectivesRoutesTest.kt | Adds admin route tests for elective endpoints. |
| api/src/test/kotlin/th/ac/bodin2/electives/api/AdminAuthServiceImplTest.kt | Adds unit tests for admin auth service behavior (challenge/session/IP allowlist). |
| api/src/test/kotlin/th/ac/bodin2/electives/api/AdminAuthRoutesTest.kt | Adds route tests for /admin/challenge and /admin/auth. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/utils/ClientErrorHelpers.kt | Adds ok() helper for 200 responses. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/utils/ClientAuthHelpers.kt | Adds connectingAddress helper for IP-based checks. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersServiceImpl.kt | Implements user delete/update/list + password changes + refactors session creation. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersService.kt | Extends UsersService API for admin operations and paging. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/TeamServiceImpl.kt | Adds Team CRUD service implementation. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/TeamService.kt | Adds Team service interface. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/SubjectServiceImpl.kt | Adds Subject CRUD service implementation. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/SubjectService.kt | Adds Subject service interface. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/NotificationsServiceImpl.kt | Adds admin websocket auth path and factors common session handling. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/NotificationsService.kt | Adds handleAdminConnection() API. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/ElectiveServiceImpl.kt | Adds Elective create/delete/update/setSubjects implementation. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/ElectiveService.kt | Extends Elective service interface with CRUD-ish methods. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/ElectiveSelectionServiceImpl.kt | Adds force-set selections operation. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/ElectiveSelectionService.kt | Adds force-set selections API + tighter docs. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/AdminAuthServiceImpl.kt | Implements SSH-signature based admin authentication with challenge/session. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/services/AdminAuthService.kt | Adds admin auth service interface + result types. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/routes/UsersRoutes.kt | Refactors handlers into context functions + proto rename. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/routes/ElectivesRoutes.kt | Refactors handlers into context functions. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/routes/AuthRoutes.kt | Uses shared ok() helper for logout. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/routes/AdminRoutes.kt | Adds admin routes/controllers and admin WS endpoint. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/Security.kt | Adds admin bearer auth scheme. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/HTTP.kt | Allows large request bodies for /admin/*. |
| api/src/main/kotlin/th/ac/bodin2/electives/api/Application.kt | Enables foreign keys, wires admin dependencies + conditional admin route registration. |
| api/build.gradle.kts | Enables Kotlin context parameters compiler flag. |
| api/README.md | Documents new admin env vars and fixes table formatting. |
| api/.env.example | Adds admin-related environment variable examples. |
Comments suppressed due to low confidence (2)
common/src/main/kotlin/db/DAO.kt:1
Student.from(...)always loads teams viagetTeams(ref), which will cause an N+1 query pattern when listing many students (e.g., admin paging). Consider fetching teams in bulk for the page (single query keyed by student IDs) and assembling theteamslists in memory.
common/src/main/kotlin/utils/IPAddress.kt:1InetAddress.getByName(...)can perform DNS resolution and accepts hostnames, not just literal IPs. For security-sensitive allowlisting (e.g.,ADMIN_ALLOWED_IPSparsing) this can create unexpected behavior and blocking lookups. Consider restricting inputs to literal IPv4/IPv6 formats (reject hostnames) before calling intoInetAddress.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersServiceImpl.kt
Outdated
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/AdminAuthServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/NotificationsServiceImpl.kt
Outdated
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersServiceImpl.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 57 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/src/main/kotlin/th/ac/bodin2/electives/api/utils/ClientAuthHelpers.kt
Outdated
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/AdminAuthServiceImpl.kt
Outdated
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/NotificationsServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/AdminAuthServiceImpl.kt
Outdated
Show resolved
Hide resolved
…g users sometimes If the user type isn't cached, a transaction needs to be made to the database, but the call isn't wrapped in one.
- Clarify ADMIN_PUBLIC_KEY env better - Use resolved/forwarded address for security checks - Eagerly load teams in UsersService.getStudents - Reduced max body size for admin routes to 1 MiB - Fixed skipped proto field number - Blank ADMIN_ALLOWED_IPS will use the default setting (localhost-only). Set to "*" to allow any IP.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/src/main/kotlin/th/ac/bodin2/electives/api/services/TeamServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/AdminAuthServiceImpl.kt
Show resolved
Hide resolved
If createSession is called concurrently before one session creation is finished.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 57 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/src/main/kotlin/th/ac/bodin2/electives/api/services/SubjectServiceImpl.kt
Outdated
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/SubjectServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/ElectiveServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/ElectiveSelectionServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersServiceImpl.kt
Outdated
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/routes/AdminRoutes.kt
Outdated
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersServiceImpl.kt
Show resolved
Hide resolved
…ng bypassed Ktor needs to fix this shit...
It's already in-memory anyway.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/AdminAuthServiceImpl.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/src/main/kotlin/th/ac/bodin2/electives/api/services/NotificationsServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersServiceImpl.kt
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/services/UsersService.kt
Outdated
Show resolved
Hide resolved
api/src/main/kotlin/th/ac/bodin2/electives/api/routes/AdminRoutes.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
api/src/main/kotlin/th/ac/bodin2/electives/api/services/NotificationsServiceImpl.kt:86
subjectSelectionSubscriptionsis now aConcurrentHashMap, but the nestedMutableMap/MutableListvalues are still not thread-safe.sendEnrollmentUpdates()mutates these structures whilenotifySubjectSelectionUpdate()iterates them, which can lead to races /ConcurrentModificationExceptionunder concurrent websocket activity. Use concurrent structures for the inner map + listener collection (e.g.,ConcurrentHashMap<Int, CopyOnWriteArrayList<...>>) and avoid mutating shared mutable lists without synchronization.
private val subjectSelectionSubscriptions =
ConcurrentHashMap<Int, MutableMap<Int, MutableList<SubjectSelectionUpdateListener>>>()
internal fun isBulkUpdatesEnabled() = config.bulkUpdatesEnabled
override fun notifySubjectSelectionUpdate(
electiveId: Int,
subjectId: Int,
enrolledCount: Int,
) {
logger.debug("Notifying subject selection update, electiveId: $electiveId, subjectId: $subjectId, enrolledCount: $enrolledCount")
val electiveSubscriptions = subjectSelectionSubscriptions[electiveId] ?: return
val subjectListeners = electiveSubscriptions[subjectId] ?: return
for (listener in subjectListeners) {
listener(electiveId, subjectId, enrolledCount)
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun parse(ip: String): IP { | ||
| if (ip.isBlank()) throw IllegalArgumentException("IP must not be blank") | ||
|
|
||
| return when (val address = InetAddress.getByName(ip.trim())) { | ||
| is Inet4Address -> { | ||
| val b = address.address |
There was a problem hiding this comment.
IP.parse uses InetAddress.getByName, which accepts hostnames and may perform DNS lookups. That can make request-time allowlist checks (allowedIPs / permitsIP) block on DNS, and can allow bypasses if a hostname resolves into an allowed range (especially when remoteAddress comes from forwarded headers). For an IP/CIDR parser, restrict input to literal IPv4/IPv6 and avoid name resolution (e.g., strict parsing or InetAddress.getByAddress-based logic).
| return transaction { | ||
| val studentIds = (Students innerJoin Users) | ||
| .select(Students.id) | ||
| .limit(PAGE_SIZE) | ||
| .offset(offset) | ||
| .map { it[Students.id].value } | ||
|
|
There was a problem hiding this comment.
getStudents() pagination fetches studentIds with limit/offset but without an orderBy, so page contents can be non-deterministic between calls (and may skip/duplicate rows as the DB chooses different scan orders). Add an explicit ordering (e.g., orderBy(Students.id)) to the studentIds query to make pagination stable.
| /** | ||
| * Creates a new student with the given information. | ||
| * | ||
| * @throws NotFoundException if any of the specified teams do not exist. | ||
| * @throws ConflictException if a student with the same ID already exists. | ||
| * @throws IllegalArgumentException if the password does not meet the requirements. | ||
| */ |
There was a problem hiding this comment.
The KDoc for createStudent says it throws ConflictException if a teacher with the same ID exists, but the implementation conflicts on the users table ID regardless of user type. The doc should reflect that it conflicts with any existing user ID (student or teacher).
|
|
||
| ```powershell | ||
| # Read the public key file, remove PEM headers/footers, and concatenate the lines | ||
| (Get-Content -Raw -Path public_key.pem) -replace '-----BEGIN PUBLIC KEY-----|-----END PUBLIC KEY-----|\s' -join '' |
There was a problem hiding this comment.
The PowerShell snippet for extracting the base64 public key looks syntactically incorrect: -replace ... -join '' mixes -replace with -join (which expects an array operand). Consider adjusting the example to a single -replace (or -split/-join sequence) that actually runs in PowerShell as written.
| (Get-Content -Raw -Path public_key.pem) -replace '-----BEGIN PUBLIC KEY-----|-----END PUBLIC KEY-----|\s' -join '' | |
| (Get-Content -Raw -Path public_key.pem) -replace '-----BEGIN PUBLIC KEY-----|-----END PUBLIC KEY-----|\s','' |
| /** | ||
| * Creates a new teacher with the given information. | ||
| * | ||
| * @throws ConflictException if a teacher with the same ID already exists. | ||
| * @throws IllegalArgumentException if the password does not meet the requirements. | ||
| */ |
There was a problem hiding this comment.
Similarly, the KDoc for createTeacher says it conflicts only when a teacher with the same ID exists, but ID uniqueness is on the users table. Update the doc to indicate it conflicts with any existing user ID.
| @Test | ||
| fun testParseInvalidIP() { | ||
| assertFails { | ||
| IP.parse("invalid") | ||
| } |
There was a problem hiding this comment.
testParseInvalidIP uses IP.parse("invalid"), but because IP.parse relies on InetAddress.getByName, this can trigger DNS resolution and may succeed (or be slow) in environments with search domains / custom DNS, making the test flaky. Prefer an obviously invalid literal IP (e.g., an out-of-range IPv4) so the test is deterministic and doesn't depend on DNS behavior.
Adds support for admin authentication via RSA keys and exposes administrative endpoints. Tests are also updated and should pass 100% (hopefully, it works on my machine :P).
TODO