feat(frontend): Migrate /settings and /login to SPA - #1922
Draft
joschahenningsen wants to merge 23 commits into
Draft
feat(frontend): Migrate /settings and /login to SPA#1922joschahenningsen wants to merge 23 commits into
joschahenningsen wants to merge 23 commits into
Conversation
joschahenningsen
force-pushed
the
frontend-grpc-api-overhaul
branch
from
August 16, 2026 09:08
9a4f5f7 to
890136a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ParseLectureHallToProto dereferenced its argument unconditionally, but GetLiveCourses declares `var lectureHall *model.LectureHall` and leaves it nil for any stream that is not held in one. Every live self-streamed lecture therefore turned the endpoint into a panic, i.e. a 500 on the start page. Handled in the parser rather than at the call site: the function takes a pointer, so nil is a value it has to answer for, and GetStream only avoided the panic by initialising to an empty struct.
GetLiveCourses started from a zero-value user instead of resolving the caller, so every `user == nil` guard below it was permanently false and courses with `loggedin` visibility were listed to anyone. Hidden and private streams fell through to IsAdminOfCourse, which for a zero user reduces to `course.UserID == u.ID` and so admitted any course with an unset owner. Resolve the caller with getCurrent and ignore the error, matching GetPublicCourses and GetCourseBySlug: the endpoint is reachable without credentials, and a nil user is what the guards are written against. The test drives the handler as an anonymous caller across all four visibility levels plus a private stream, and gives each course a real owner so it cannot pass by way of the UserID == 0 path.
Five handlers called the DAO with context.Background(), which discards the caller's cancellation and deadline: a client that goes away still pays for the query. It also blocks anything that travels in the context, so a future interceptor resolving the caller once per request would be silently ignored by exactly these call sites. The context.Background() in api.go stays. That one registers the gateway handler at startup and is not tied to a request.
Role is compared directly in ~15 places outside the middleware, and those comparisons ask at least three different questions: may this person see every course, are they an operator, and may they escalate someone's privileges. They share an answer only because one role grants all three, so none can be changed without auditing the rest, and none of them is named. Add a permission vocabulary and a single role-to-permission table. The mapping reproduces today's authority exactly: an admin is a lecturer for every course plus server administration, a lecturer may create courses and administers only what they are granted. Nothing is rewired yet; the ~15 call sites move in a follow-up. Splitting out an operator who administers the infrastructure without reading every recorded lecture then becomes a row in that table rather than a tree-wide audit, which is the point. IsAdminOfCourse now delegates to CanAdminister so the rule has one implementation rather than two that can drift. Verified equivalent to the previous body across all 648 combinations of role, user id, grants, course id and owner.
CanAdminister matches ownership on ID, and both halves are zero for a zero-value user and a course with an unset owner, so an unresolved caller administered every such course. That is the path by which the anonymous caller in GetLiveCourses reached course administration; fixing it here as well means the next handler that loses track of its caller fails closed. A user with no ID was never persisted and owns nothing, so the comparison should not be reached at all.
Replaces every authorization comparison against User.Role with a permission check, so a call site now states the capability it needs rather than the role that happens to have it. The two role gates collapse into one parameterised middleware. tools.Admin guarded ten route groups that did not all mean the same thing: nine are deployment operations and take PermAdministerServer, while the account administration group in users.go takes PermManageUsers — it was only ever grouped with the others because one role granted both. The inline checks divide the same way. Token deletion and admin-scoped token minting are account administration; server-wide statistics and the runner page are server administration; search, the calendar feed and hidden live streams are PermViewAllCourses; skipping the explicit course-admin grant on course creation and copying is PermAdministerAllCourses. No authority changes hands: the mapping was built to reproduce the current answers exactly. Two incidental improvements fall out of Can being nil-safe — the statistics and runner checks dereferenced the user without a nil guard and would have panicked rather than refused. Left alone deliberately: the switches on Role that pick a notification audience or a course listing. Those dispatch on identity rather than ask permission, and flattening them to capabilities would lose that. IsUserAdmin keeps its role query too, with a note — its two callers ask different questions and it needs splitting first.
…tion Every handler opened by restating its own name into the log and, if it needed to know who was calling, by parsing the JWT and loading the user again. Both are properties of the request rather than of the handler, so they move into interceptors. logRequest reports the method, the resulting status code and the duration — how a call ended rather than that it started. Only a fault on this side logs at error level: a rejected credential is the most common way an RPC fails and would otherwise bury the failures worth looking at. resolveCaller authenticates once and publishes the result, so a handler asking who is calling costs a context lookup. It deliberately rejects nothing: several RPCs are reachable without credentials, and which ones is a property of the RPC, so refusing here would encode that policy twice. The declarative version of that policy is the next step, and this is where it will hook in. getCurrent reads what the interceptor left behind and falls back to resolving inline when none has run, so a handler called directly from a test behaves the same. The resolution error is carried alongside the user rather than dropped, so a handler can still tell a rejected token from an absent one — which is what an expired session needs in order to answer 401 and trigger the client's refresh.
Authentication was opt-in: it happened only where a handler remembered to call getCurrent. A new RPC that forgot was a fully public endpoint that compiled, looked like its neighbours and had no test to catch it. That is the wrong default for an API about to grow an administrative surface. Every method now declares a policy — public, authenticated, or requiring a capability — and the interceptor enforces it before the handler runs. A method with no policy is refused rather than served, so forgetting one yields an endpoint that does not work instead of one anyone can call. TestEveryMethodHasAPolicy walks the service descriptor in both directions: no method without a policy, no policy without a method. That is what makes the guarantee hold at endpoint two hundred rather than only today. TestOnlyExpectedMethodsArePublic lists the thirteen anonymous endpoints explicitly, so widening access is an edit someone has to make on purpose. This also closes a crash. updateProgress dereferences the caller, but authorizeUserForStreamCourse returns no user for an anonymous caller on a public course, so an unauthenticated request panicked the handler. Declaring the RPC authenticated makes that unreachable, and the public-list test now stands guard over it. The policy lives in Go rather than in a proto option. The option would sit closer to the RPC it describes, but it needs a custom extension and a regeneration round trip, and the guarantee comes from the descriptor test either way. It can move later without changing enforcement.
One service with 27 methods was still legible, but the v1 handlers still to migrate are roughly ten times that, and moving methods between services is free now and a coordination problem later. MetaService, UserService, CourseService and StreamService. The split is grouping only: every method keeps its google.api.http path and its openapiv2 tag, all four register on the same gRPC server and the same gateway mux, and one API struct implements all of them. Diffing the generated swagger against the previous one shows the same 27 endpoints with the same paths and the same tags; only operationId gains a service prefix, and nothing consumes those — the frontend writes paths by hand and the generated TypeScript contains message types only. services.go collects, per service, its descriptor, its gRPC registration, its gateway registration and its method policies. Splitting those across three files would make adding a service three edits, and the one that is easy to forget is the policy — the edit that fails open. Now it is one. TestEveryServiceInTheProtoIsPoliced reads the file descriptor out of the proto registry and checks it against that list, so a service defined in the proto but never registered here is a build failure rather than a set of endpoints the policy test never looks at. Verified that regenerating from an unmodified proto reproduces the committed output byte for byte before making any change, so this diff is the split and nothing else.
The four policy-heavy handlers interleaved visibility rules with database calls and protobuf marshalling, which is why the GetLiveCourses leak read like setup rather than like a policy change. The rules now live in apiv2/visibility as functions over domain types, testable without a gRPC context or a mocked DAO. The package cannot reach the database. A visibility rule that needs a query has outgrown being a rule, and the import boundary makes that a compile error rather than a habit. Two rules, not one, because the handlers were never asking the same question. Listed governs what appears in a listing; Reachable governs following a direct link. They differ on hidden courses, which are unlisted rather than private — GetLiveCourses excluded them and GetCourseBySlug deliberately did not. That distinction was previously visible only by comparing two handlers in different files; it is now named, commented, and has a test of its own so that making them agree has to be a decision. Also fixes the N+1 in GetLiveCourses. GetCourseById is uncached and preloads four relations, and a course with several lectures live at once was fetched once per stream. model.User.IsEligibleToWatchCourse is left where it is. It answers a similar question to Reachable with slightly different rules, and the two agree for anonymous callers but diverge for signed-in ones, so unifying them is a behaviour change to make deliberately rather than fold into this.
The "missing row is a 404, anything else is a 500" mapping was open-coded at five call sites — five chances to invert the condition, and five places that passed gorm's own "record not found" through to the client, which says nothing about what was being looked for. e.FromGorm takes the not-found message rather than defaulting to the driver's, so each site now says what it failed to find. Only the sites that meant 404 were changed. GetBookmarks answers a missing row with an empty list and ResetPassword treats one as success; those are different intents that happen to test the same error, and folding them in would have changed behaviour.
The endpoints that serve anonymous callers discarded the error from getCurrent entirely, so a token that was presented and rejected was indistinguishable from no token at all. A client whose token had expired was quietly downgraded: it saw the logged-out view of a page it was signed in to, and got a 403 from the stream authorization path rather than the 401 that makes api.ts mint a new token and retry. The recovery path existed on the client and was never reachable. ErrNoCredentials now marks the three ways a request arrives with nothing to authenticate with, and currentOrAnonymous maps that to an anonymous caller and anything else to 401. Applied to the four optional-user sites: GetLiveCourses, GetPublicCourses, GetCourseBySlug and authorizeUserForStreamCourse. Requests that genuinely carry no credential are unaffected — the anonymous view is still the anonymous view.
GetLiveCourses discarded the error from GetCourseById, and gorm's Find reports a missing row as a zero-value struct with a nil error, so neither failure was noticed. The zero course has Visibility "", which is none of public, loggedin, enrolled or hidden, so every visibility rule waved it through: a stream pointing at a deleted course was listed to everyone under an empty name. Log the error and skip any stream whose course did not come back. The ID check is what catches the deleted-row case, since that one arrives without an error at all.
The admin route group had no authorization middleware, and AdminPage checks only that someone is signed in. Any logged-in user could render all thirteen tabs — including the list of every admin and lecturer, and the state of every worker, runner and lecture hall. The template has always hidden the links behind an admin check, which is why it went unnoticed. Hiding a link is not a guard; the URLs were reachable by typing them. Split by what each page administers rather than gated as one block. Both permissions belong to admins today so no access changes hands, but the distinction is what lets an operator role later be a change to the role table alone. The token page goes with user management, matching dao.GetAllTokens, which already scopes its rows on the same permission.
The SPA cannot render its shell without the deployment's branding, version and footer links, all of which the server-rendered pages read straight from package globals. GET /api/v2/config returns them, plus whether the database holds any users at all — a fresh installation has to offer to create the first account rather than a login form nobody can use. Public, because the shell is rendered before anyone signs in: the login page has the same header and footer as every other page. Nothing in the response is per-user. VersionTag moves to tools, set from main alongside the existing assignment to web. Reading web.VersionTag would have made the v2 API depend on the frontend it is replacing, for a string. A failure to reach the database is an error rather than a default. Returning false there would show a login form on a deployment that has no accounts, with nothing to explain why it does not work.
The SPA's stylesheet carried over .tum-live-popup-container, .tum-live-popup and .tum-live-popup h2 but not the four descendant rules that give the popup body its shape. The keyboard shortcuts dialog reuses that markup, and all of its column alignment comes from the width on `strong`, so without them it rendered as a run-on list instead of two columns. .notificationBody a was missing for the same reason. Notification bodies are inserted as raw HTML and routinely contain links, which have no styling at all once Tailwind's preflight has reset them. Copied verbatim from web/assets/css, per the note at the top of the file: the two frontends have to render a shared component identically while pages move across one at a time.
DEFAULT_PLAYBACK_SPEEDS claimed to mirror model.defaultPlaybackSpeeds but had eight entries to the server's eleven, with 0.5, 0.75, 1.25 and 1.75 disabled where the server enables them, and 2.5, 3 and 3.5 missing. This is not merely a display bug. A user who has never saved the setting sees the wrong checkboxes, and the first speed they toggle writes the whole array back — silently disabling four speeds the player was offering and dropping three more as options entirely. The defaults are also handed out as a copy. SettingsView toggles `entry.enabled` in place, so sharing the module-level array let one load's unsaved change survive into the next — including the reload that the failed -save path performs precisely to discard it.
The v1 handler enforced two things the v2 path does not, and with /api/users/settings/name deleted this is now the only way to set a name. The three-month cooldown reads CreatedAt, but the write path uses gorm's Save, which only touches UpdatedAt. CreatedAt therefore stays at the first write forever, so the limit applies to the second change and never again. v1 measured from UpdatedAt. The length limit was dropped altogether. The name column is varchar(80), so without it an over-long name fails in the driver as a 500 rather than a clean 400 — and renders on every server-side page that shows it. Both rules move into ValidatePreferredName, which takes the previous setting and the clock as arguments so they can be tested without a database or a three-month wait. maxUsernameLength is exported rather than duplicated.
…tend The SPA cached notifications under the same localStorage keys as web/ts/notifications.ts, which looked like it would carry read state across the two frontends. The entries are not interchangeable: the legacy list matches on a database id, which protobuf.UserGroupNotification has no field for, and the SPA matches on a key derived from the content, which legacy entries lack. So whichever frontend wrote last, the other found nothing it could match. Loading the start page and then a migrated page within the ten-minute throttle served the SPA legacy-shaped entries with no `key`, rendering every row with `:key="undefined"`; the reverse made every notification reappear unread on the server-rendered pages. Two caches that work beat one that does not. Adding `id` to the proto would make a shared one possible.
The store marked itself loaded in a finally block, so a request that failed for anything other than a 401 still counted as an answer. A transient 500 or a dropped connection at boot therefore left the user null and loaded true, and because load() short-circuits on loaded, nothing ever asked again: a signed-in user saw a Login button for the life of the page. A 401 is an answer — nobody is signed in — and still marks the store loaded. Anything else leaves the question open for the next caller. That makes the rejection meaningful, which in turn makes App.vue's `void auth.load()` an uncaught rejection whenever the API is down. It is fire and forget by design, so it now says so explicitly.
When a write fails, save() reloads the user so the screen stops showing a value the server rejected. That reload was unguarded, so if it failed too — the usual case, since the server that refused the write is often still down — the rejection escaped save(). Every caller but savePreferredName invokes it as `void save(...)`, where that surfaces as an unhandled promise rejection rather than as anything the user can see. The failure is now contained and the original write error left on screen: it is the actionable one, and the value shown is the one the user typed anyway.
joschahenningsen
force-pushed
the
frontend-grpc-api-overhaul
branch
from
August 16, 2026 18:19
098e586 to
6fee490
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.