diff --git a/firebase.json b/firebase.json index 86e6be0..81b9542 100644 --- a/firebase.json +++ b/firebase.json @@ -1,6 +1,7 @@ { "firestore": { - "rules": "firestore.rules" + "rules": "firestore.rules", + "indexes": "firestore.indexes.json" }, "emulators": { "auth": { diff --git a/firestore.indexes.json b/firestore.indexes.json new file mode 100644 index 0000000..5aa7d11 --- /dev/null +++ b/firestore.indexes.json @@ -0,0 +1,31 @@ +{ + "//": [ + "THE ONE COMPOSITE INDEX THIS APP NEEDS, and it exists because the notice board now", + "filters by audience and still orders by date.", + "", + "web/lib/announcements.ts goes out of its way to avoid composite indexes — it sorts", + "pinned notices in memory rather than adding a second orderBy, precisely so a fresh", + "project never meets a failed-precondition error with a console link in it. That", + "trick does not work here: the audience filter CANNOT be done in memory, because a", + "query that returns a document the rules refuse fails entirely rather than dropping", + "that row. The filter has to be in the query, so the index has to exist.", + "", + "Declared in the repo rather than clicked into the console so it deploys alongside", + "the rules that need it: firebase deploy --only firestore", + "", + "sessions and forms need no entry here. Both read their whole collection and sort", + "client-side for their own reasons, so neither query carries an orderBy, and a single", + "where() on one field is served by the automatic single-field index." + ], + "indexes": [ + { + "collectionGroup": "announcements", + "queryScope": "COLLECTION", + "fields": [ + { "fieldPath": "audience", "order": "ASCENDING" }, + { "fieldPath": "created_at", "order": "DESCENDING" } + ] + } + ], + "fieldOverrides": [] +} diff --git a/firestore.rules b/firestore.rules index abc186d..8c9ce2b 100644 --- a/firestore.rules +++ b/firestore.rules @@ -7,7 +7,7 @@ rules_version = '2'; // static export with no server — so there is no back end doing checks. Everything that // protects members' data is in this file. // -// The site now has sign-in. Five collections: +// The site now has sign-in, a dashboard and an organisers' page. Ten collections: // // users/{uid} one profile per member. Readable and writable ONLY by that // member, plus readable by admins. Contains their name, college @@ -20,6 +20,39 @@ rules_version = '2'; // applications/{id} legacy, from before sign-in. Create-only and unreadable, kept // so the rows already there stay protected. // +// And the five the dashboard brought with it: +// +// announcements/{id} the notice board. Every member reads it; admins write it. +// sessions/{id} when the club meets. Same access as the board, different shape: +// a session has a time, and that time is the ONE date in this file +// the client is allowed to choose. +// forms/{id} forms and polls. Every member reads them; admins write them. +// forms/{id}/responses/{uid} +// one answer per member, keyed by uid so a second is impossible to +// express. Theirs to write, ADMINS ONLY to list — that list rule is +// the only thing between an attributed form and a public one. +// contributions/{uid} +// GitHub counts. Read by their owner and by admins, WRITTEN BY NO +// CLIENT AT ALL: the Cloud Function uses the Admin SDK, which +// bypasses this file. A count a client can write is a count a +// client can invent, and "merged pull requests" is the one number +// here somebody has a reason to inflate. +// +// TWO THINGS CHANGED IN THE BLOCKS ABOVE WHEN THOSE ARRIVED, and both are easy to miss: +// +// `admins` IS NO LONGER WRITE-DENIED TO EVERY CLIENT. It is the core-team roster now +// (web/lib/roster.ts) — the access grant and the public team listing in one row — and +// OWNERS write it from /admin. That is a real widening, and it is fenced three ways: +// only an owner may write, NOBODY may write their OWN row, and NOBODY may delete a row. +// So one compromised admin account still cannot appoint accomplices, and a compromised +// owner cannot retire everybody else and be the only one left holding the club — +// retiring is a flag any other owner can turn back on, and the row stays as the record. +// +// isAdmin() NOW READS THAT FLAG. It used to test only that the row existed, so retiring +// somebody took their buttons away and left every permission intact. web/lib/auth.tsx +// has always applied the same two defaults — a missing flag means active, a missing role +// means plain admin — and the two files have to keep agreeing. +// // `mentors` IS THE FIRST COLLECTION A CLIENT MAY WRITE THAT IS NOT ITS OWN ROW, and that // widening was deliberate rather than incidental. It is acceptable because a mentor entry // is published, organiser-authored, non-personal copy — the same kind of thing that lives @@ -38,7 +71,7 @@ rules_version = '2'; // ONLY @sst.scaler.com MAY REGISTER, and this is where that is enforced. web/lib/auth.tsx // also passes the domain to Google and signs out anyone off-domain, but both of those // are conveniences: a client can be modified, and a token from another domain is still a -// valid token. `isMember()` below is what actually refuses. +// valid token. `isStudent()` below is what actually refuses. // // email_verified is required as well as the domain. A Google Workspace sign-in always // carries it, so this costs nothing today — but it means that if email/password sign-in @@ -61,25 +94,209 @@ service cloud.firestore { // ---------------------------------------------------------------- helpers - /** Signed in, on the club's domain, with a verified address. The domain test is a - * regex anchored at BOTH ends: `endsWith` alone would accept - * "eve@evil.com@sst.scaler.com" and a leading-wildcard match would accept - * "sst.scaler.com.evil.com". */ - function isMember() { + /** A STUDENT AT THIS COLLEGE. Signed in, on the club's domain, with a verified + * address. The domain test is a regex anchored at BOTH ends: `endsWith` alone + * would accept "eve@evil.com@sst.scaler.com" and a leading-wildcard match would + * accept "sst.scaler.com.evil.com". + * + * THIS FUNCTION WAS CALLED isMember() AND THE NAME WAS A BUG. It says nothing + * about the club — every student on the domain satisfies it the first time they + * sign in. But it was the gate on announcements, forms and sessions, so "members + * only" was never a state the club could express: everything the organisers + * posted went to the entire college. Membership is now a separate question, asked + * by isClubMember() below, and the two must never be confused again. */ + function isStudent() { return request.auth != null && request.auth.token.email != null && request.auth.token.email_verified == true && request.auth.token.email.lower().matches('^[^@]+@sst[.]scaler[.]com$'); } + /** The caller's OWN profile, or an empty map if they have never made one. + * + * Guarded with exists() rather than get()'d directly: a get() on a missing + * document is an ERROR, not an empty result, and it fails the whole rule. A + * student who signed in and never filled the profile form is the ordinary case, + * not an edge one, so this must return "no membership" for them rather than + * denying every read they attempt. */ + function profileRow() { + return exists(/databases/$(database)/documents/users/$(request.auth.uid)) + ? get(/databases/$(database)/documents/users/$(request.auth.uid)).data + : {}; + } + + /** A MEMBER OF THE CLUB, which is a smaller set than isStudent() and is the whole + * point of the split. Read off the reader's own profile, where only an admin can + * have written it — see the users/{uid} block for the field-level rule that stops + * somebody promoting themselves. + * + * ABSENT MEANS NOT A MEMBER, which is the opposite of the default this file uses + * for `active` on an admin row, and deliberately so: an absent `active` had to + * mean "yes" because rows predating the field belonged to organisers who already + * had access and would otherwise have been locked out. An absent `membership` + * belongs to a student who has signed in, which has never by itself meant + * membership in anything. Defaulting it to "member" would hand the club's private + * board to the entire domain — which is the exact bug this change exists to fix. */ + function isClubMember() { + return isStudent() && profileRow().get('membership', 'student') == 'member'; + } + /** An admin is a member whose address has a document in `admins`. Looked up by * email rather than uid so organisers can be added before they first sign in. * * Lowercased on lookup because Google returns the address as the person typed it; * the document id in `admins` must therefore always be lowercase. */ + function adminRow() { + return get(/databases/$(database)/documents/admins/$(request.auth.token.email.lower())).data; + } + + /** An admin is a member whose address has an ACTIVE row in `admins`. + * + * THE ACTIVE TEST IS NOT DECORATION. Retiring an organiser is a flag rather than a + * delete, because the roster is also the club's handover record and the row has to + * survive. Without this clause the row surviving meant the ACCESS surviving too: a + * retired organiser kept every permission on this page and only the screen stopped + * offering them the buttons. That is exactly the gap a UI-only check leaves. + * + * A MISSING FLAG MEANS ACTIVE, deliberately. Rows written before the field existed + * carry neither it nor a role, and defaulting to denied would have locked out every + * organiser the club already had, the moment these rules deployed. web/lib/auth.tsx + * applies the same two defaults; change one and you must change both. */ function isAdmin() { - return isMember() - && exists(/databases/$(database)/documents/admins/$(request.auth.token.email.lower())); + return isStudent() + && exists(/databases/$(database)/documents/admins/$(request.auth.token.email.lower())) + && adminRow().get('active', true) == true; + } + + /** An owner is an active admin whose row says so — the two or three people who may + * change the roster itself. + * + * TWO TIERS RATHER THAN ONE, because both alternatives are worse. Console-only + * appointment makes every addition wait on whoever holds console access, and that + * person eventually graduates. Letting any admin appoint any admin means one + * compromised college account can appoint accomplices and retire everybody else. + * + * A missing role means plain admin, so no rules deployment silently promotes + * anybody. */ + function isOwner() { + return isAdmin() && adminRow().get('role', 'admin') == 'owner'; + } + + /** May this reader see the document being read? ONE FUNCTION, applied identically + * to announcements, forms and sessions, so the three cannot drift. + * + * READS resource.data DIRECTLY AND TAKES NO ARGUMENT, AND THAT IS A SECURITY FIX + * RATHER THAN A STYLE. The obvious way to write this is `d.get('audience', + * 'both')`, so that a document predating the field falls back to the old + * everyone-sees-it behaviour. That version is NOT ENFORCED ON A QUERY. Tested + * against the emulator, with two documents and a non-member: + * + * resource.data.audience == 'both' unfiltered list -> REFUSED + * resource.data.get('audience', 'both') == 'both' unfiltered list -> SUCCEEDED, + * returning the members-only row + * + * A per-document `get` is refused correctly under both forms; it is specifically + * the LIST path that leaks, which is the one every dashboard panel uses. The rule + * looked right, read right, and handed the club's private board to the whole + * college. Do not reintroduce a default here — if you need one, it belongs in a + * rule that is never evaluated against a query, the way canAnswerForm() below is. + * + * HOW A LIST IS ACTUALLY JUDGED, because it is not what it looks like. Firestore + * does not run this rule over the documents a query returns and drop the ones it + * refuses. It runs it against the QUERY, and allows it only if the query's own + * constraints PROVE the rule holds for every document that could possibly match. + * So `getDocs(collection(db, 'announcements'))` is refused for an ordinary reader + * even when every notice in the collection is addressed to everyone — nothing in + * an unconstrained query proves that, and tomorrow's notice might not be. Adding + * `where('audience', 'in', ['both', 'students'])` makes it provable, and the same + * query then succeeds. That is why every read in web/lib carries the clause, and + * why it cannot be dropped after checking that no members-only notice exists yet. + * + * It is also why the defaulted version leaked rather than merely being loose: a + * `.get(field, default)` expression is satisfiable whatever the document holds, so + * the prover waved through the unconstrained query it should have refused. + * + * THE COST OF NO DEFAULT is that a notice with no `audience` field at all is + * refused rather than shown to everyone. That is not a regression, because such a + * document is already unreachable: a where clause does not match documents missing + * the field, so no constrained query can return it either. They have to be stamped + * either way — components/AudienceBackfill.tsx finds them and says so on the + * organisers' page. + * + * ADMINS SEE EVERYTHING regardless, and that is not a loophole: they are the + * people writing these documents, and an organiser who could not see the notice + * they just posted because they aimed it at students would reasonably conclude the + * post had failed. It is also what keeps unstamped legacy documents visible to the + * only people who can fix them. */ + function canSeeAudience() { + return isAdmin() + || resource.data.audience == 'both' + || (resource.data.audience == 'members' && isClubMember()) + || (resource.data.audience == 'students' && !isClubMember()); + } + + /** The same question asked about a form somebody is trying to ANSWER, rather than + * about the document being read. + * + * A SEPARATE FUNCTION BECAUSE IT IS SAFE TO BE TOLERANT HERE. This one is only + * ever evaluated on a single-document create or update against forms/{id}/ + * responses/{uid} — never against a query — so the default that makes the read + * rule leak cannot leak anything here, and it is what lets a member still answer a + * form written before audiences existed. The argument is the FORM's data, fetched + * by formDoc(), not the response being written. */ + function canAnswerForm(f) { + return isAdmin() + || f.get('audience', 'both') == 'both' + || (f.get('audience', 'both') == 'members' && isClubMember()) + || (f.get('audience', 'both') == 'students' && !isClubMember()); + } + + /** An audience field, as a writer may set it. Optional on the way in: a build that + * predates the picker omits it and gets the 'both' default, which is what those + * documents already meant. */ + function isWellFormedAudience(d) { + return !('audience' in d) + || d.audience in ['members', 'students', 'both']; + } + + /** THE FIELDS A MEMBER MAY NOT TOUCH ON THEIR OWN PROFILE, and the reason + * membership can live on a document its subject owns. + * + * Everything else at users/{uid} is the member's to write — it is their name and + * their hostel. Membership is the club's statement about them, and it is read by + * isClubMember() to decide what they can see, so a member who could write it + * could admit themselves to the club and read the organisers' board. This is the + * rule that makes that impossible. + * + * changedKeys() RATHER THAN COMPARING VALUES, because the comparison has to hold + * for documents that do not carry the field at all: `resource.data.membership` on + * a profile written before this existed is an error, not a null, and an error in + * a rule denies the write. Asking whether the key changed is well defined whether + * or not it is there. */ + function membershipUnchanged() { + return !request.resource.data.diff(resource.data).changedKeys() + .hasAny(['membership', 'membership_by', 'membership_at']); + } + + /** An organiser admitting somebody to the club, or removing them — and NOTHING + * else in the same write. + * + * This is the only rule in the file that lets one person write another person's + * document, so it is deliberately the narrowest: exactly the three membership + * keys may differ, the actor is stamped from their own token rather than from the + * request body, and the time is the server's. An admin cannot use this path to + * edit somebody's name, hostel or GitHub — those stay the member's own. + * + * THE AUDIT FIELDS ARE REQUIRED, NOT OPTIONAL. "Who is in this club" is the + * question the organisers' page is opened with, and a club whose membership + * changes without a record of who changed it cannot answer the follow-up. Same + * reasoning as `added_by` on the roster. */ + function onlyMembershipChanged() { + return request.resource.data.diff(resource.data).changedKeys() + .hasOnly(['membership', 'membership_by', 'membership_at']) + && request.resource.data.membership in ['member', 'student'] + && request.resource.data.membership_by == request.auth.token.email + && request.resource.data.membership_at == request.time; } /** Fields a member may never change after the first save. `created_at` is what @@ -107,10 +324,22 @@ service cloud.firestore { // an `isAdmin` field that a future careless rule might read. d.keys().hasOnly([ 'uid', 'email', 'name', 'hostel', 'github', 'path', + 'membership', 'membership_by', 'membership_at', 'created_at', 'updated_at' ]) && d.keys().hasAll(['uid', 'email', 'name', 'hostel', 'updated_at']) + // MEMBERSHIP IS IN THE SHAPE BUT NOT IN THIS FUNCTION'S GIFT. It is listed + // here only so that `hasOnly` does not reject a profile that already carries + // it — a member editing their name sends a merge, and the merged document + // includes whatever an organiser wrote. WHO may set it is decided by the + // rules on users/{uid} below, not here: this function is called on the + // member's own writes and an admin's alike, so enforcing it here would either + // block the organiser or permit the member. + && (!('membership' in d) || d.membership in ['member', 'student']) + && (!('membership_by' in d) + || (d.membership_by is string && d.membership_by.size() > 0)) + // Identity cannot be forged. The document id, the uid inside it and the signed-in // token must all agree, and the stored address must be the one that signed in — // so a member cannot file a profile under somebody else's name or address. @@ -227,6 +456,270 @@ service cloud.firestore { && (!('created_at' in d) || d.created_at == request.time || d.created_at == resource.data.created_at); } + /** A roster row: who runs the club, and what that entitles them to. + * + * ONE ROW CARRIES BOTH HALVES — the access grant (role, active) that this file reads + * on every request, and the public billing (name, title, photo) the team page shows. + * They are one row because they were two lists and the two lists drifted: the club + * kept its team in a content file and its access here, nothing connected them, and + * the failure was silent in both directions. + * + * VALIDATED EVEN THOUGH ONLY OWNERS WRITE IT, and here that is not belt-and-braces: + * role and active are read straight back by isAdmin() and isOwner() above. A row whose + * role is the string "Owner" grants nothing and looks right in the console; a row whose + * active flag is the string "false" reads as active forever. The two fields that decide + * access are the two least safe to leave unchecked. */ + function isWellFormedRosterRow(d, email) { + return + d.keys().hasOnly([ + 'email', 'name', 'title', 'photo', 'role', 'active', 'group', + 'batch', 'github', 'shadow_of', 'added_by', 'added_at', 'updated_at' + ]) + && d.keys().hasAll(['email', 'name', 'role', 'active', 'added_by', 'updated_at']) + + // A row cannot grant access to one address while describing another. The id is what + // isAdmin() looks up and it lowercases before looking, so a row filed under a + // capitalised address would be a grant nobody could ever use. + && d.email == email + && d.email.lower() == d.email + && d.email.matches('^[^@]+@sst[.]scaler[.]com$') + + && d.name is string && d.name.size() > 0 && d.name.size() <= 120 + && d.role in ['owner', 'admin'] + && d.active is bool + + && (!('title' in d) || (d.title is string && d.title.size() > 0 && d.title.size() <= 80)) + && (!('photo' in d) || (d.photo is string && d.photo.size() > 0 && d.photo.size() <= 300)) + && (!('group' in d) || d.group in ['officer', 'lead', 'shadow']) + && (!('batch' in d) || (d.batch is string && d.batch.size() > 0 && d.batch.size() <= 12)) + && (!('github' in d) || (d.github is string && d.github.size() > 0 && d.github.size() <= 100)) + && (!('shadow_of' in d) + || (d.shadow_of is string && d.shadow_of.size() > 0 && d.shadow_of.size() <= 80)) + + && d.added_by is string && d.added_by.size() > 0 + && d.updated_at == request.time + && (!('added_at' in d) || d.added_at == request.time || d.added_at == resource.data.added_at); + } + + /** A notice on the board. + * + * THE LINK IS THE ONE FIELD WITH A HOLE IN IT, and it is closed here rather than in + * the client. This is free text an organiser types, and it renders as a link every + * member can click — so a `javascript:` href in it is a script running inside a + * signed-in member's page. Requiring the string to start https:// is what refuses + * that, and it belongs in this file because the client is a client. */ + function isWellFormedPost(d) { + return + d.keys().hasOnly([ + 'title', 'body', 'link', 'pinned', 'category', 'archived', + 'audience', 'author_email', 'created_at', 'updated_at' + ]) + && isWellFormedAudience(d) + && d.keys().hasAll(['title', 'body', 'pinned', 'author_email', 'updated_at']) + + && d.title is string && d.title.size() > 0 && d.title.size() <= 140 + && d.body is string && d.body.size() > 0 && d.body.size() <= 4000 + && d.pinned is bool + + // Both optional, and both ABSENT on every notice posted before they existed. The + // rules must not require them: a strict list would have made every notice already + // on the board unsaveable the moment this deployed, which presents to an organiser + // as "the pin button is broken". + && (!('archived' in d) || d.archived is bool) + && (!('category' in d) || d.category in ['general', 'event', 'deadline']) + + && (!('link' in d) + || (d.link is string + && d.link.size() > 0 + && d.link.size() <= 500 + && d.link.matches('^https://[^ ]+$'))) + + && d.author_email is string && d.author_email.size() > 0 + && d.updated_at == request.time + && (!('created_at' in d) || d.created_at == request.time || d.created_at == resource.data.created_at); + } + + /** A session: what the club is doing, and WHEN. + * + * starts_at IS THE ONE DATE IN THIS FILE THE CLIENT CHOOSES. Every other timestamp is + * pinned to request.time so nothing can be backdated. This one is nearly always in the + * future — that is what scheduling means — so the same rule would make the feature + * impossible. It is a time on a poster; nothing reads it as evidence of when something + * happened, and that is what makes the exception safe rather than an oversight. */ + function isWellFormedSession(d) { + return + d.keys().hasOnly([ + 'title', 'speaker', 'location', 'notes', 'starts_at', + 'audience', 'created_by', 'created_at', 'updated_at' + ]) + && isWellFormedAudience(d) + && d.keys().hasAll(['title', 'starts_at', 'created_by', 'updated_at']) + + && d.title is string && d.title.size() > 0 && d.title.size() <= 140 + && d.starts_at is timestamp + + && (!('speaker' in d) || (d.speaker is string && d.speaker.size() > 0 && d.speaker.size() <= 120)) + && (!('location' in d) || (d.location is string && d.location.size() > 0 && d.location.size() <= 120)) + && (!('notes' in d) || (d.notes is string && d.notes.size() > 0 && d.notes.size() <= 2000)) + + && d.created_by is string && d.created_by.size() > 0 + && d.updated_at == request.time + && (!('created_at' in d) || d.created_at == request.time || d.created_at == resource.data.created_at); + } + + /** A form, which is also a poll — showing the counts back is the whole difference. + * + * field_ids IS A FLAT MIRROR OF THE QUESTIONS' IDS AND IT EXISTS FOR THIS FILE. Rules + * cannot iterate a list of maps, so there is no way to reach into `fields` and read an + * id out of it — which means no way to check that a member's answers use only keys the + * form actually asked. The mirror is the only expressible form of that check. Requiring + * the two lists to be the same LENGTH is what stops a mirror quietly ceasing to match + * the questions it mirrors. */ + function isWellFormedForm(d) { + return + d.keys().hasOnly([ + 'title', 'description', 'fields', 'field_ids', 'open', 'show_tally', + 'tally', 'audience', 'author_email', 'created_at', 'updated_at' + ]) + && isWellFormedAudience(d) + && d.keys().hasAll([ + 'title', 'fields', 'field_ids', 'open', 'show_tally', 'author_email', 'updated_at' + ]) + + && d.title is string && d.title.size() > 0 && d.title.size() <= 140 + && (!('description' in d) + || (d.description is string && d.description.size() > 0 && d.description.size() <= 2000)) + + && d.fields is list && d.fields.size() > 0 && d.fields.size() <= 30 + && d.field_ids is list && d.field_ids.size() == d.fields.size() + + && d.open is bool + && d.show_tally is bool + + && d.author_email is string && d.author_email.size() > 0 + && d.updated_at == request.time + && (!('created_at' in d) || d.created_at == request.time || d.created_at == resource.data.created_at); + } + + /** The form a response is being filed against. One extra document read per write, and + * it buys the two things the response rules cannot do without: the question ids, and + * whether the form is still taking answers. */ + function formDoc(formId) { + return get(/databases/$(database)/documents/forms/$(formId)).data; + } + + /** One member's answer. + * + * ANSWER KEYS ARE PINNED TO THE FORM'S OWN QUESTION IDS. Without that a member could + * append a hundred keys of their own to a document the organisers later export — and + * an export is a spreadsheet somebody opens, not a thing anybody reviews field by + * field. The VALUES cannot be typed here, because rules cannot iterate a map's values. + * A bounded set of keys is the half that is expressible, and it is the half that + * matters. + * + * submitted_at IS DELIBERATELY NOT FROZEN. Nothing depends on it being trustworthy — + * it is a courtesy line on the organisers' screen — and freezing it would oblige a + * member changing their answer to send back a value they were never shown. */ + function isWellFormedResponse(d, formId, uid) { + return + d.keys().hasOnly(['uid', 'email', 'name', 'answers', 'submitted_at', 'updated_at']) + && d.keys().hasAll(['uid', 'email', 'answers', 'updated_at']) + + && d.uid == uid + && d.uid == request.auth.uid + && d.email == request.auth.token.email + + && (!('name' in d) || (d.name is string && d.name.size() > 0 && d.name.size() <= 120)) + + && d.answers is map + && d.answers.keys().hasOnly(formDoc(formId).field_ids) + + && d.updated_at == request.time; + } + + /** AN APPLICATION FROM A STRANGER, and the only unauthenticated write in this file. + * + * WHY IT IS ANONYMOUS, restated because the obvious "improvement" is to require + * sign-in and it would be wrong. The club's front door cannot require the key you get + * by walking through it: the site's headline promises a reader they need nothing but a + * laptop and a GitHub account, and an auth wall on the apply form makes that sentence + * false at the exact moment somebody acts on it. web/lib/applications.ts says the same + * thing at greater length. A member's profile is a different act by a different person + * and lives at users/{uid}. + * + * SO THIS FUNCTION IS THE ENTIRE BOUNDARY. There is no uid to compare, no verified + * address, nothing to prove ownership with — every field here was typed by somebody + * the club has never met. `hasOnly` is the load-bearing half: without it a submitter + * appends a hundred keys of their own to a row an organiser later reads. + * + * WHAT STOPS A SCRIPT FILLING IT OVERNIGHT is App Check, not this file — see the note + * in web/lib/firebase.ts. Rules can say what a valid application looks like; they + * cannot say how many a stranger may send. + * + * THE CLOSED SETS MIRROR web/content/join.ts AND HAVE ALREADY DRIFTED ONCE. The + * version of this rule recovered from git accepted `level` values of none/some-git/ + * merged; the form has offered beginner/intermediate since upstream replaced the + * LEVELS array with LEVEL_LABEL. Deployed as it was, it would have refused every real + * application while looking perfectly correct. `npm run rules` now diffs all four of + * these sets against the content. */ + function isWellFormedApplication(d) { + return + d.keys().hasOnly([ + 'name', 'email', 'year_branch', 'hostel', 'github', + 'level', 'path', 'programs', 'programs_other', 'submitted_at' + ]) + && d.keys().hasAll([ + 'name', 'email', 'year_branch', 'hostel', 'level', 'path', + 'programs', 'submitted_at' + ]) + + // Required strings, bounded. The form's maxlength is a courtesy to the reader; + // these are the real limits, because a direct SDK call never sees the form. + && d.name is string && d.name.size() > 0 && d.name.size() <= 120 + && d.email is string && d.email.size() > 3 && d.email.size() <= 200 + // NOT restricted to the college domain, deliberately, and this is the one place + // that differs from every other rule here. Somebody applying from a personal + // address is an applicant to talk to, not a forgery to reject — the domain rule + // belongs on MEMBERSHIP rather than on the act of asking. + && d.email.matches('^[^@\\s]+@[^@\\s]+[.][^@\\s]+$') + && d.year_branch is string && d.year_branch.size() > 0 && d.year_branch.size() <= 120 + + && (!('github' in d) || (d.github is string && d.github.size() > 0 && d.github.size() <= 100)) + + && d.hostel in ['uniworld-1', 'uniworld-2'] + && d.level in ['beginner', 'intermediate'] + && d.path in ['build-day', 'first-contribution', 'fast-track', 'program-track'] + + // Programmes are REQUIRED and must be non-empty, which is the form's rule too — + // the browser refuses the submit without a tick. Enforced here as well because a + // direct SDK call never sees the form, and an application with an empty list would + // read as a UI bug to whoever opens it rather than as the forgery it is. + && d.programs is list + && d.programs.size() > 0 + && d.programs.size() <= 10 + && d.programs.hasOnly([ + 'gsoc', 'lfx', 'outreachy', 'sok', 'hacktoberfest', 'sob', + 'gssoc', 'ssoc', 'esoc', 'other' + ]) + // 'other' and its free text are a pair, checked in BOTH directions: no bare + // 'other' with nothing to explain it, and no stray text without the tick that is + // supposed to have produced it. + && (!d.programs.hasAny(['other']) + || ('programs_other' in d + && d.programs_other is string + && d.programs_other.size() > 0)) + && (!('programs_other' in d) + || (d.programs_other is string + && d.programs_other.size() > 0 + && d.programs_other.size() <= 120 + && d.programs.hasAny(['other']))) + + // The server's clock, not the submitter's. Everything else in this document was + // supplied by a stranger; this one field cannot be forged, so submission ORDER + // stays trustworthy even when nothing else does. + && d.submitted_at == request.time; + } + // ---------------------------------------------------------------- members match /users/{uid} { @@ -235,19 +728,40 @@ service cloud.firestore { // Firestore evaluates a list against the rule without knowing each document, so // the owner clause would make every query fail. Splitting them gives members // their own row and admins the whole table, with no rule that is true for both. - allow get: if isMember() && (request.auth.uid == uid || isAdmin()); + allow get: if isStudent() && (request.auth.uid == uid || isAdmin()); allow list: if isAdmin(); // First save. Only for yourself, and only a valid profile. - allow create: if isMember() + // + // NOBODY JOINS THE CLUB BY SIGNING UP. A first profile may not carry any of the + // membership keys at all, so the one document a student creates unsupervised + // cannot be the document that admits them. They become a member when an + // organiser says so, on the admin path below. + allow create: if isStudent() && request.auth.uid == uid - && isWellFormedProfile(request.resource.data, uid); + && isWellFormedProfile(request.resource.data, uid) + && !request.resource.data.keys() + .hasAny(['membership', 'membership_by', 'membership_at']); - // Edits. Same validation, plus identity and created_at frozen. - allow update: if isMember() + // Edits, by the member themselves. Same validation, plus identity and created_at + // frozen, plus membership out of reach — see membershipUnchanged(). + allow update: if isStudent() && request.auth.uid == uid && isWellFormedProfile(request.resource.data, uid) - && immutablesUnchanged(); + && immutablesUnchanged() + && membershipUnchanged(); + + // Edits by an ORGANISER, and only ever to membership. + // + // A SECOND `allow update` RATHER THAN AN `||` INSIDE THE FIRST, because the two + // are different transactions with different validation: the member's write is + // checked against the whole profile shape and pinned to their own uid and + // address, and neither of those can hold for an admin writing somebody else's + // row. Firestore ORs the allow rules for us, so splitting them keeps each one + // readable on its own — and neither can be loosened by accident while trying to + // fix the other. + allow update: if isAdmin() + && onlyMembershipChanged(); // Nobody deletes a profile from a client, including its owner and including // admins. A member asking to be removed is a conversation and a console action, @@ -259,14 +773,58 @@ service cloud.firestore { match /admins/{email} { // You may read exactly your OWN row, which is how the client discovers whether to - // show the dashboard link. Scoped to the caller's address so this cannot be used - // to enumerate who the admins are. - allow get: if isMember() && request.auth.token.email.lower() == email; - allow list: if false; - // Managed by hand in the Firebase console. Denied to every client so that a - // compromised admin session cannot appoint further admins — the one privilege - // escalation this model would otherwise allow. - allow write: if false; + // show the dashboard link and whether to enable the roster form. Scoped to the + // caller's address, so a member cannot use this to find out who the organisers are. + allow get: if isStudent() && request.auth.token.email.lower() == email; + + // THE WHOLE LIST, TO ADMINS ONLY. This is what the roster panel renders, and every + // admin sees it — read-only unless they are an owner, which is a decision the screen + // makes and this file does not need to. It stays denied to ordinary members because + // the document id IS an email: listing it would hand any member every organiser's + // inbox, which is also why the public team page is generated at build time from + // these rows rather than reading them live. + allow list: if isAdmin(); + + // OWNERS WRITE THE ROSTER. This is the one privilege escalation this model has to + // permit in order to be usable at all — a club whose team turns over yearly cannot + // route every appointment through whoever still has console access. Three fences + // keep the widening as narrow as it was meant to be: + // + // 1. isOwner(), so a plain admin — and therefore one compromised admin account — + // cannot appoint accomplices or retire anybody. + // 2. NOT YOUR OWN ROW, below. It stops an owner demoting themselves into a state + // only another owner can undo, and it means seizing the club takes two + // compromised owner accounts rather than one: whoever is left can always turn + // the others back on. + // 3. No delete, at all — see below. + // + // WHAT THIS STILL DOES NOT STOP: an owner retiring every other owner one at a time. + // Rules cannot count documents, so "never leave the club with no owner" is not + // expressible here; the roster screen shows the live owner count instead. The + // consequence is recoverable from the console, which is the test that made it + // acceptable to leave. + allow create: if isOwner() + && request.auth.token.email.lower() != email + && isWellFormedRosterRow(request.resource.data, email); + + // WHEN SOMEBODY WAS APPOINTED CANNOT BE ERASED BY AN EDIT. Stated separately from + // the shape check because the shape check has to tolerate the field being ABSENT — + // every row seeded from the console before it existed has no appointment date, and + // requiring one would make those rows uneditable. This says the narrower thing: if + // the stored row HAS a date, the write must carry the same one back. Without it, a + // full overwrite that simply left the field out would silently wipe it, which is + // exactly the bug the profile rules already learned once. + allow update: if isOwner() + && request.auth.token.email.lower() != email + && isWellFormedRosterRow(request.resource.data, email) + && (!('added_at' in resource.data) + || request.resource.data.added_at == resource.data.added_at); + + // Retiring is a flag, never a delete, and the difference is the point. It revokes + // just as fast — isAdmin() reads the flag on every request — and it keeps the record + // of who ran the club when, which is the thing a handover actually needs. It is also + // undoable by any other owner, which the fence above depends on. + allow delete: if false; } // ---------------------------------------------------------------- mentors @@ -280,7 +838,7 @@ service cloud.firestore { // Hidden mentors (active: false) come back too, deliberately. The client filters // them out of the picker, but a member who picked somebody before they were hidden // still needs to see a name rather than an id. - allow get, list: if isMember(); + allow get, list: if isStudent(); // ADMIN WRITE. See the note in the header for why this widening is acceptable and // where the same argument stops applying. @@ -301,14 +859,14 @@ service cloud.firestore { // Same get/list split, and for the same reason as users/: a single `read` rule // naming the owner cannot express "and admins may query the collection", because // Firestore evaluates a list without knowing the documents. - allow get: if isMember() && (request.auth.uid == uid || isAdmin()); + allow get: if isStudent() && (request.auth.uid == uid || isAdmin()); allow list: if isAdmin(); - allow create: if isMember() + allow create: if isStudent() && request.auth.uid == uid && isWellFormedEnrollment(request.resource.data, uid); - allow update: if isMember() + allow update: if isStudent() && request.auth.uid == uid && isWellFormedEnrollment(request.resource.data, uid) && request.resource.data.uid == resource.data.uid @@ -323,17 +881,236 @@ service cloud.firestore { // // Owner only. An admin cannot delete somebody's enrollment, for the same reason // they cannot delete a profile. - allow delete: if isMember() && request.auth.uid == uid; + allow delete: if isStudent() && request.auth.uid == uid; + } + + // ---------------------------------------------------------- announcements + + match /announcements/{id} { + // EVERY MEMBER LISTS THE BOARD, and that is the exception in this file rather than + // the pattern. users/ is admin-only to list because every row is somebody's address; + // a notice carries nothing personal beyond the organiser's byline. If a field is ever + // added here that names a member, this rule is the first thing to revisit. + // + // Archived notices come back too. The organisers' screen has to show them — an + // archive nobody can see is a delete with extra steps — so the member view filters + // them out in the client. + // AUDIENCE IS ENFORCED HERE, NOT IN THE PANEL THAT RENDERS IT. A members-only + // notice is withheld from a non-member by the database; the screen never sees it + // to filter. + // + // EVERY CLIENT READ OF THIS COLLECTION MUST CARRY where("audience", "in", ...), + // built from queryableAudiences() in web/lib/audience.ts. That is not defensive + // duplication — a list rule is judged against the QUERY rather than the rows it + // returns, so an unconstrained read is refused outright, even when every document + // in the collection would have been allowed. See canSeeAudience() above for the + // full note. If a panel goes blank for members but not for organisers, a missing + // where clause is the first thing to check. + allow get, list: if isStudent() && canSeeAudience(); + + // THE BYLINE IS PINNED ON CREATE AND FROZEN ON EDIT, which are two different rules + // for a reason that is easy to get wrong. Pinning it on edit as well would mean any + // admin pinning somebody else's notice silently signs their name to it; freezing it + // on create would mean pinning it to nothing at all. + allow create: if isAdmin() + && request.resource.data.author_email == request.auth.token.email + && isWellFormedPost(request.resource.data); + + allow update: if isAdmin() + && request.resource.data.author_email == resource.data.author_email + && isWellFormedPost(request.resource.data) + // WHEN THE NOTICE WENT UP CANNOT BE ERASED BY AN EDIT, stated here rather than in + // the shape check for the same reason as the roster's appointment date: the shape + // check has to tolerate the field being absent, because a create has no stored + // value to compare against. This says the narrower thing — if the notice HAS a + // date, the write must carry the same one back. + // + // It is load-bearing because setFlags() in web/lib/announcements.ts is a FULL + // overwrite, not a merge: pressing "pin" sends the whole document, so a version + // of that function which forgot to include created_at would silently wipe the + // posting date of every notice anybody pinned. Its own comment claims this rule + // exists; before this line, it did not. + && (!('created_at' in resource.data) + || request.resource.data.created_at == resource.data.created_at); + + // Deletable, unlike a profile: a notice with the wrong date has to be retractable and + // there is no roster to lose. Archiving is the ordinary way one comes off the board; + // this is for the one posted by mistake. + allow delete: if isAdmin(); + } + + // --------------------------------------------------------------- sessions + + match /sessions/{id} { + // AUDIENCE IS ENFORCED HERE, NOT IN THE PANEL THAT RENDERS IT. A members-only + // notice is withheld from a non-member by the database; the screen never sees it + // to filter. + // + // EVERY CLIENT READ OF THIS COLLECTION MUST CARRY where("audience", "in", ...), + // built from queryableAudiences() in web/lib/audience.ts. That is not defensive + // duplication — a list rule is judged against the QUERY rather than the rows it + // returns, so an unconstrained read is refused outright, even when every document + // in the collection would have been allowed. See canSeeAudience() above for the + // full note. If a panel goes blank for members but not for organisers, a missing + // where clause is the first thing to check. + allow get, list: if isStudent() && canSeeAudience(); + + // created_by gets the same pin-then-freeze treatment as a notice's byline, and for + // the same reason: editing somebody's session must not claim it. + allow create: if isAdmin() + && request.resource.data.created_by == request.auth.token.email + && isWellFormedSession(request.resource.data); + + allow update: if isAdmin() + && request.resource.data.created_by == resource.data.created_by + && isWellFormedSession(request.resource.data) + // Same freeze as a notice's posting date, and saveSession() is a full overwrite + // too — moving a session must not erase when it was first scheduled. + && (!('created_at' in resource.data) + || request.resource.data.created_at == resource.data.created_at); + + // A real delete, unlike a notice's archive. A session carries nobody's answer, so + // nothing is lost — and a cancelled session that cannot be taken off the list is the + // club telling its members to turn up to a room nobody booked. + allow delete: if isAdmin(); + } + + // ------------------------------------------------------------------ forms + + match /forms/{formId} { + // Members read every form, including closed ones, because the dashboard shows a + // member what they already answered after it closes. Note that `tally` therefore + // reaches every member regardless of the show-the-counts flag: that flag is a + // rendering decision, not a boundary, and it is safe to leave as one only because a + // tally is aggregate counts and never says who answered what. Attributed answers + // live in the subcollection below, and that IS fenced. + // AUDIENCE IS ENFORCED HERE, NOT IN THE PANEL THAT RENDERS IT. A members-only + // notice is withheld from a non-member by the database; the screen never sees it + // to filter. + // + // EVERY CLIENT READ OF THIS COLLECTION MUST CARRY where("audience", "in", ...), + // built from queryableAudiences() in web/lib/audience.ts. That is not defensive + // duplication — a list rule is judged against the QUERY rather than the rows it + // returns, so an unconstrained read is refused outright, even when every document + // in the collection would have been allowed. See canSeeAudience() above for the + // full note. If a panel goes blank for members but not for organisers, a missing + // where clause is the first thing to check. + allow get, list: if isStudent() && canSeeAudience(); + + // THE TALLY IS REFUSED TO EVERY CLIENT, and this is the same reasoning as the GitHub + // counts: a count the client supplies is a count the client invented. It is written + // only by the tallyResponses Cloud Function through the Admin SDK, which does not + // pass through this file at all. + // + // On create it must be absent. On update it must be absent or IDENTICAL to what is + // stored — absent is allowed because a form nobody has answered has no tally, and + // identical is what lets an organiser fix a typo in a poll that already has votes + // without the write being refused. web/lib/forms.ts sends the stored value straight + // back for exactly that case. + allow create: if isAdmin() + && request.resource.data.author_email == request.auth.token.email + && !('tally' in request.resource.data) + && isWellFormedForm(request.resource.data); + + allow update: if isAdmin() + && request.resource.data.author_email == resource.data.author_email + && (!('tally' in request.resource.data) + || request.resource.data.tally == resource.data.tally) + && isWellFormedForm(request.resource.data) + // Same freeze as a notice's posting date. saveForm() is a full overwrite as well, + // so without this, correcting a question would erase when the form was published. + && (!('created_at' in resource.data) + || request.resource.data.created_at == resource.data.created_at); + + allow delete: if isAdmin(); + } + + match /forms/{formId}/responses/{uid} { + // ONE ANSWER PER MEMBER, BY CONSTRUCTION — keyed by uid, the same trick as users/. + // A second response is not refused by a check somebody has to remember to write; it + // cannot be expressed. + // + // THE LIST RULE IS THE ONE THAT MATTERS HERE. Responses are attributed, which is the + // point — most of these are "sign up for X" and somebody has to chase the people who + // did not — and that is only safe because no member can enumerate them. Loosen this + // and the form system becomes a public one, silently, with every answer already in it. + allow get: if isStudent() && (request.auth.uid == uid || isAdmin()); + allow list: if isAdmin(); + + // A CLOSED FORM TAKES NO MORE ANSWERS. Enforced here rather than by hiding the + // button, because the button is a rendering decision — and "closed" is the only + // thing an organiser has to stop a sign-up once the room is full. + // + // THE AUDIENCE TEST IS REPEATED ON THE WAY IN, and it is not redundant with the + // one on the form itself. Answering is a separate request from reading: a + // non-member who is refused the members-only form can still POST a response to + // its id, because the id is not a secret and this subcollection is a different + // path with its own rules. Without this clause the club's private sign-ups would + // be open to the whole domain to fill in — the read gate is not a write gate, + // and a form nobody outside the club may SEE must also be one nobody outside it + // may ANSWER. + allow create: if isStudent() + && request.auth.uid == uid + && formDoc(formId).open == true + && canAnswerForm(formDoc(formId)) + && isWellFormedResponse(request.resource.data, formId, uid); + + allow update: if isStudent() + && request.auth.uid == uid + && formDoc(formId).open == true + && canAnswerForm(formDoc(formId)) + && isWellFormedResponse(request.resource.data, formId, uid) + && request.resource.data.uid == resource.data.uid + && request.resource.data.email == resource.data.email; + + // Nothing in the dashboard deletes an answer, so nothing here permits it. A member + // who has changed their mind edits their answer, which is the same act with the + // record intact. Whoever adds a "withdraw" button adds the rule with it — and should + // scope it to the owner, the way enrollments/ does. + allow delete: if false; + } + + // ---------------------------------------------------------- contributions + + match /contributions/{uid} { + allow get: if isStudent() && (request.auth.uid == uid || isAdmin()); + allow list: if isAdmin(); + + // WRITTEN BY NO CLIENT, INCLUDING ITS OWNER. The counts come from GitHub via a Cloud + // Function using the Admin SDK, which bypasses this file entirely — so this deny is + // not a restriction on a feature, it is the feature. "Merged pull requests" is the + // one number on the dashboard somebody has a reason to inflate, and the first time a + // member edited it by hand the panel would stop meaning anything. + allow write: if false; } // ---------------------------------------------------- legacy applications match /applications/{id} { - // Nothing writes here any more; the profile replaced it. Reads stay denied - // because these rows hold the same personal details as a profile, and the rows - // stay immutable so the history is intact. + // NOT LEGACY, AND THE COMMENT THAT SAID SO COST THE CLUB ITS FRONT DOOR. An upstream + // merge took this block as "nothing writes here any more; the profile replaced it" + // and denied create — but /join still renders components/ApplyForm.tsx, which writes + // exactly here. Every application submitted between that merge and this line was + // refused. The form rendered, the applicant filled it in, and the write failed. + // + // Nothing in the UI could have told anybody: the rules were correct in git, correct + // in review, and wrong about which features existed. + // + // CREATE IS OPEN TO STRANGERS, on purpose — see isWellFormedApplication above for + // why applying must not require sign-in, and for why that function is therefore the + // whole boundary. + allow create: if isWellFormedApplication(request.resource.data); + + // NOBODY READS THIS FROM A CLIENT, INCLUDING ADMINS. Organisers read applications in + // the Firebase console. That is a deliberate floor rather than a missing feature: + // these rows hold names, addresses and hostels belonging to people who are not + // members yet and never agreed to appear in anything, so the smallest surface that + // still lets the club act on them is the right one. An organisers' view would need + // its own admin-only rule AND a decision about retention, not a loosened read. allow read: if false; - allow create: if false; + + // Immutable once sent, so the history is intact and an applicant cannot be edited + // into somebody else after an organiser has read them. allow update, delete: if false; } diff --git a/functions/README.md b/functions/README.md index 88703c4..3da64e4 100644 --- a/functions/README.md +++ b/functions/README.md @@ -1,53 +1,117 @@ -# functions/ — not deployed, on purpose - -This directory contains one Cloud Function that would email the organisers whenever -somebody applies. **It is deliberately not in use.** Applications are read in the -Firebase console instead. - -## Why it is switched off - -Cloud Functions cannot make outbound network calls on Firebase's free **Spark** plan, -and sending email is an outbound call. So this function requires the **Blaze** -(pay-as-you-go) plan, which requires a card on file — even though a student club's -volume sits inside the free allowance and the bill would in practice be zero. - -The club chose to stay on the free plan and read submissions in the console. That loses -nothing: every field of every application is stored and visible there. - -## It cannot fire by accident - -`firebase.json` does **not** declare a `functions` block, so `firebase deploy` ignores -this directory entirely. Nothing here runs, costs anything, or affects the site. The -form does not depend on it — the email was always downstream of the Firestore write, so -an applicant's submission succeeds whether or not any of this exists. - -## If you do want email later - -1. Upgrade the project to Blaze in the Firebase console. -2. Add a `functions` block to `firebase.json`: - ```json - "functions": { "source": "functions" } - ``` -3. Set the two secrets — they go to Google Secret Manager, never into this repo: - ```bash - firebase functions:secrets:set SMTP_URL # e.g. smtps://user%40gmail.com:app-password@smtp.gmail.com:465 - firebase functions:secrets:set MAIL_TO # where notifications land - ``` - URL-encode the username and password. An `@` in the username or a `/` in the - password will otherwise truncate the connection string and fail with a confusing - auth error. -4. `cd functions && npm install && cd .. && firebase deploy --only functions` +# functions/ + +Three Cloud Functions, in two unrelated jobs. **One of them is dormant and two are +live** — which half you care about depends on why you opened this directory. + +| Function | Status | What it does | +|---|---|---| +| `emailOnApplication` | **dormant** | emails the organisers when a row lands in `applications` | +| `syncContributions` | live | 04:00 IST daily, fetches members' GitHub pull requests | +| `refreshContributions` | live | the same fetch for one member, from the dashboard button | + +All three need the **Blaze** (pay-as-you-go) plan, because all three make outbound +network calls and Firebase's free **Spark** plan does not allow those. A student club's +volume sits well inside the free allowance and the bill is in practice zero, but a +billing account has to exist on the project. That is Google's restriction on egress, not +a choice made here. + +> **This directory used to say it could not fire by accident**, because `firebase.json` +> declared no `functions` block and `firebase deploy` therefore ignored it. That is no +> longer true — the block was added so the contribution sync could deploy. Everything +> here now deploys when you run `firebase deploy`, including the dormant one. + +--- + +## The GitHub sync (live) + +This is what fills the "Your open source" panel on `/dashboard`. + +### Why it is a function and not a fetch in the browser + +Two reasons, and each one alone would be enough: + +* **Rate limits.** An unauthenticated browser gets 60 GitHub requests an hour *per IP*, + which on a college network is 60 an hour for the entire club. +* **Trust.** A count the client writes is a count the client can invent. "Merged pull + requests" is the one number on that dashboard somebody has a reason to inflate, so + `contributions/{uid}` is `allow write: if false` for **every** client including its + owner, and only the Admin SDK — which bypasses the rules — writes it. + +### Setting it up + +```bash +firebase functions:secrets:set GITHUB_TOKEN # a token with NO scopes at all +firebase deploy --only functions +``` + +The token is **optional but not really**. It only ever reads public data, so no scopes +are needed — it exists purely for the rate limit. Without it GitHub allows 10 searches a +minute and 60 user lookups an *hour*, so the nightly sweep stops after about fifteen +members, and the failure arrives as a 403 that reads like a permissions problem. With it: +30 a minute and 5,000 an hour. + +### What it costs to run + +The sweep spends **two search requests and one user lookup per member**, paced one member +every five seconds, and stops at the 100 stalest rows per run. So a club of 100 is +refreshed daily and a club of 400 every four days, in a deterministic order — and the +dashboard's "checked N days ago" line is what makes that visible rather than mysterious. +If it ever needs to go faster, the cap and the gap are both named constants at the top of +`index.js`. + +The callable is rate-limited to one refresh per member per ten minutes, enforced in the +function rather than in the client, because a client-side timer is a suggestion. + +--- + +## The application email (dormant) + +It triggers on `applications/{id}`, which is the **legacy** collection from before +sign-in existed. Nothing writes there any more — the profile at `users/{uid}` replaced +it — so this function never fires today. + +It is kept rather than deleted because pointing it at `users/{uid}` is a decision about +whether organisers want an email per sign-up, not a tidy-up. If you make that decision, +change the `document` option in `index.js` and check `format.js` still matches the +profile's field list, which has lost four fields since this was written. + +To switch it on you also need its two secrets, which go to Google Secret Manager and +never into this repo: + +```bash +firebase functions:secrets:set SMTP_URL # e.g. smtps://user%40gmail.com:app-password@smtp.gmail.com:465 +firebase functions:secrets:set MAIL_TO # where notifications land +``` + +URL-encode the username and password. An `@` in the username or a `/` in the password +will otherwise truncate the connection string and fail with a confusing auth error. `SMTP_URL` is a connection string rather than a vendor SDK, so Gmail with an app password, the domain's own mailbox, SendGrid, Mailgun and Resend all work without changing the code. -## Testing it without any of that +--- + +## Testing, without Firebase, billing, a token or a deploy ```bash cd functions && npm test ``` -20 assertions against the formatter — the part with all the logic and the only part an -organiser sees. Needs no Firebase, no billing, no SMTP and no deploy. It prints a sample -email at the end so you can read what an organiser would receive. +Two suites, both pure: + +* **`test-format.mjs`** — 20 assertions against the email formatter, the part with all + the logic and the only part an organiser ever sees. It prints a sample email at the + end so you can read what would arrive. +* **`test-github.mjs`** — 33 assertions against `github.js`, driven by recorded GitHub + payloads with `fetch` stubbed out. The cases worth knowing about are the ones that are + easy to get wrong and impossible to notice: a real account with no pull requests versus + a handle that does not exist (they look almost identical from the API and the dashboard + has to word them completely differently), the distinct-repository count (derived, not + returned — an earlier version counted only the eight rows the dashboard lists and + produced a number that looked entirely plausible), and that a rate limit **throws** + rather than resolving to zeroes, because a silent zero would overwrite a real member's + real counts. + +The rules that protect what these functions write are exercised separately, against the +real emulator, by `web/scripts/rules-emulator.mjs`. diff --git a/functions/github.js b/functions/github.js new file mode 100644 index 0000000..4fde2b3 --- /dev/null +++ b/functions/github.js @@ -0,0 +1,211 @@ +// Ask GitHub what one handle has contributed, and reduce the answer to the six values +// the dashboard shows. +// +// SEPARATE FROM index.js SO IT CAN BE TESTED WITHOUT FIREBASE. Everything here is a pure +// function of a fetch response, so test-github.mjs drives it with recorded payloads and +// no emulator, no credentials and no network. The same split as format.js. +// +// THE SEARCH API, NOT THE EVENTS API, and this is the decision the whole file rests on: +// +// /users/{handle}/events is the obvious endpoint and is useless here. It returns the +// last 90 days AND at most 300 events, so a member who +// contributed steadily for two years would show a partial +// quarter. It is an activity feed, not a history. +// /search/issues answers "every PR this account has ever opened, filtered by +// state" in one request, with a total count that does not lie +// about the tail. That is exactly the question. +// +// WHAT IT COSTS, AND WHICH LIMIT IT COSTS AGAINST. GitHub meters search separately from +// everything else, and this file touches both meters: +// +// /search/issues TWO requests per member (merged, then open), against the SEARCH +// limit — 10 per minute unauthenticated, 30 with a token. This is the +// binding constraint, and the throttle in index.js is sized against it. +// /users/{handle} ONE request per member, against the CORE limit — 60 per hour +// unauthenticated, 5,000 with a token. Effectively free with a token +// and the first thing to break without one. +// +// So a token is not optional past about fifteen members, and the failure without one +// arrives as a 403 that reads like a permissions problem. +// +// `is:merged` RATHER THAN `is:closed`. A closed PR is one that was rejected or abandoned +// just as often as one that landed, and counting those as contributions would make the +// number flattering and worthless. Open ones are counted separately and labelled as +// what they are: in flight. + +/** How many recent pull requests to store per member. The dashboard shows a list, not an + * archive — and every stored row is a row in a document that is read on every dashboard + * load, so this is a page-weight decision as much as a design one. */ +const RECENT = 8; + +/** Firestore's hard limit is 1MB per document; this is nowhere near it, but a title is + * attacker-influenced in the sense that anybody can open a PR with a 2000-character + * title on a public repo. Truncated so a single hostile title cannot bloat the row. */ +const MAX_TITLE = 160; + +/** GitHub's largest allowed page size on the search API. Asked for on the merged search + * so the distinct-repository count is derived from a full page rather than from the + * handful of rows the dashboard lists — see fetchContributions. */ +const PAGE_MAX = 100; + +/** GitHub handles: alphanumerics and single hyphens, 1-39 characters, no leading or + * trailing hyphen. Validated BEFORE it goes anywhere near a URL. + * + * This is the one piece of member-supplied text this function puts into a request, and + * it arrives from a free-text field on the profile. Encoding it would be enough to make + * it safe; rejecting it outright is better, because a handle that cannot be valid can + * never be found and the request is wasted. */ +const HANDLE = /^[a-zA-Z0-9](?:[a-zA-Z0-9]|-(?=[a-zA-Z0-9])){0,38}$/; + +function isValidHandle(h) { + return typeof h === "string" && HANDLE.test(h.trim()); +} + +/** "https://github.com/owner/name/pull/12" -> "owner/name". + * + * Derived from repository_url rather than from html_url: the former is a stable API + * field ("https://api.github.com/repos/owner/name"), the latter is a web URL whose shape + * GitHub is free to change. */ +function repoOf(item) { + const u = item?.repository_url ?? ""; + const m = u.match(/\/repos\/([^/]+\/[^/]+)$/); + return m ? m[1] : "unknown"; +} + +function toPull(item, state) { + const title = String(item?.title ?? "").slice(0, MAX_TITLE); + const pull = { + title: title || "(no title)", + repo: repoOf(item), + url: String(item?.html_url ?? ""), + state, + }; + // Omitted rather than written null when absent, so the stored shape matches the + // optional field in web/lib/contributions.ts. + if (item?.pull_request?.merged_at) pull.merged_at = item.pull_request.merged_at; + else if (item?.closed_at && state === "merged") pull.merged_at = item.closed_at; + return pull; +} + +/** One search request. Returns { total, items } or throws. + * + * `fetch` is global on Node 18+, which is what functions/package.json pins via + * engines.node — so there is no HTTP dependency in this file at all. + * + * A User-Agent is REQUIRED by GitHub: without one the API returns 403 with a message + * about it, which reads like a rate limit and is not one. */ +async function search(q, token, perPage) { + const url = + "https://api.github.com/search/issues" + + `?q=${encodeURIComponent(q)}&sort=updated&order=desc&per_page=${perPage}`; + + const headers = { + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "scaler-open-source-club", + }; + if (token) headers.Authorization = `Bearer ${token}`; + + const res = await fetch(url, { headers }); + + if (res.status === 403 || res.status === 429) { + // Distinguished from a generic failure because the caller throttles on it. GitHub + // uses 403 for rate limiting on the search API, which is why this is not simply + // "not authorised". + const err = new Error("GitHub rate limit"); + err.code = "rate-limit"; + throw err; + } + if (!res.ok) { + const err = new Error(`GitHub returned ${res.status}`); + err.code = "github-down"; + throw err; + } + + const body = await res.json(); + return { total: Number(body?.total_count ?? 0), items: body?.items ?? [] }; +} + +/** Does this account exist? Asked separately so "no such handle" is distinguishable from + * "this person has no pull requests" — which look identical through the search API, and + * which the dashboard has to word completely differently. */ +async function exists(handle, token) { + const headers = { + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "scaler-open-source-club", + }; + if (token) headers.Authorization = `Bearer ${token}`; + const res = await fetch(`https://api.github.com/users/${encodeURIComponent(handle)}`, { + headers, + }); + if (res.status === 404) return false; + if (res.status === 403 || res.status === 429) { + const err = new Error("GitHub rate limit"); + err.code = "rate-limit"; + throw err; + } + // Anything else that is not a clean 200 is treated as "exists", so a transient blip on + // this cheap check never wipes a member's real numbers with a not_found flag. + return true; +} + +/** + * Everything the dashboard shows for one handle. + * + * Returns the exact shape stored at contributions/{uid}, minus uid and synced_at, which + * the caller adds. Throws with `code` set to 'rate-limit' or 'github-down'; a handle that + * does not exist is NOT a throw, because it is a normal answer the UI renders. + */ +async function fetchContributions(handle, token) { + const h = String(handle ?? "").trim(); + if (!isValidHandle(h)) { + const err = new Error("Not a possible GitHub handle"); + err.code = "bad-handle"; + throw err; + } + + if (!(await exists(h, token))) { + return { github: h, merged: 0, open: 0, repos: 0, recent: [], not_found: true }; + } + + // Two requests, merged first. If the second is rate-limited the first is still thrown + // away — a row with real merged counts and a zeroed `open` would be a lie that looks + // like data, and the caller's retry is cheap. + // + // PAGE_MAX ON THE MERGED SEARCH, NOT RECENT, AND THAT IS NOT A TYPO. The list only + // shows RECENT rows, so asking for eight would seem to be enough — but `repos` is + // counted from the items in this response, and eight items can name at most eight + // repositories. Sized down to RECENT, a member with thirty merged PRs across twelve + // projects would read "8 projects touched" forever, and the number would look like a + // fact rather than a truncation. One page of 100 costs exactly the same one request. + const merged = await search(`type:pr is:merged author:${h}`, token, PAGE_MAX); + const open = await search(`type:pr is:open author:${h}`, token, 1); + + const all = merged.items.map((i) => toPull(i, "merged")); + + return { + github: h, + merged: merged.total, + open: open.total, + // Distinct repositories among the merged PRs this request returned. GitHub's search + // API has no distinct-repository count, so this is derived — and it is therefore + // EXACT up to PAGE_MAX merged pull requests and an undercount past it. A second page + // would be a second request against a 30-per-minute limit, for a member who has + // merged more than a hundred patches and does not need this dashboard to tell them + // they are active. If the club ever has such members, paginate here rather than + // quietly leaving the number wrong. + repos: new Set(all.map((p) => p.repo)).size, + recent: all.slice(0, RECENT), + }; +} + +module.exports = { + fetchContributions, + isValidHandle, + repoOf, + toPull, + RECENT, + MAX_TITLE, + PAGE_MAX, +}; diff --git a/functions/index.js b/functions/index.js index 2736dd9..2377abd 100644 --- a/functions/index.js +++ b/functions/index.js @@ -1,4 +1,21 @@ -// Email the organisers whenever somebody applies. +// The club's Cloud Functions. Four of them, in three unrelated jobs: +// +// emailOnApplication emails the organisers when a row lands in `applications`. +// syncContributions nightly, fetches every member's GitHub pull requests. +// refreshContributions the same fetch for one member, on demand from the dashboard. +// tallyResponses recounts a poll whenever somebody answers it. +// +// A NOTE ON THE FIRST ONE, because it is easy to read this file and assume it still +// fires. It triggers on `applications/{id}`, which is the LEGACY collection from before +// sign-in existed — nothing writes there any more; the profile at `users/{uid}` replaced +// it. So this function is currently dormant. It is kept rather than deleted because the +// rows already in that collection are still protected by it in the rules, and because +// pointing it at `users/{uid}` is a decision about whether organisers want an email per +// sign-up, not a tidy-up. Whoever makes that decision should change the `document` below +// and check format.js still matches the profile's field list. +// +// --------------------------------------------------------------------------------- +// EMAIL THE ORGANISERS WHENEVER SOMEBODY APPLIES. // // Triggered by the document being created, NOT by the form. That ordering is the whole // design: the applicant's submit succeeds or fails on the Firestore write alone, so if @@ -103,3 +120,315 @@ exports.emailOnApplication = onDocumentCreated( } }, ); + +// --------------------------------------------------------------------------------- +// GITHUB CONTRIBUTIONS +// +// Two entry points onto the same work: a scheduled sweep that keeps every member's row +// warm, and a callable so a member who merged something an hour ago does not have to wait +// for tomorrow. +// +// WHY THIS IS A FUNCTION AND NOT A FETCH IN THE BROWSER. Both halves of the answer are in +// web/lib/contributions.ts, and both are decisive: a browser gets 60 unauthenticated +// GitHub requests an hour PER IP, which on a college network is 60 an hour for the whole +// club; and a count the client writes is a count the client can invent. The Admin SDK +// here bypasses firestore.rules, which is exactly why `contributions/{uid}` is +// `allow write: if false` for every client including its owner. +// +// CREDENTIALS. One optional secret: +// +// firebase functions:secrets:set GITHUB_TOKEN +// +// A fine-grained personal access token with NO scopes at all is enough — this only reads +// public data, and the token is for the rate limit rather than for access. Without it the +// sweep still runs and simply gets much less far before GitHub refuses; with it, the +// search limit goes from 10 requests a minute to 30. + +const { onSchedule } = require("firebase-functions/v2/scheduler"); +const { onCall, HttpsError } = require("firebase-functions/v2/https"); +const admin = require("firebase-admin"); + +const { fetchContributions, isValidHandle } = require("./github"); + +const GITHUB_TOKEN = defineSecret("GITHUB_TOKEN"); + +// Initialised once, at module load, guarded because both functions in this file share the +// process and a second initializeApp() throws. +if (!admin.apps.length) admin.initializeApp(); + +/** Collection names, which MUST match web/lib/firebase.ts. Different strings here means + * the sweep writes rows the dashboard never reads — and nothing fails, which is what + * makes it worth stating rather than inlining. `npm run rules` asserts these. */ +const USERS = "users"; +const CONTRIBUTIONS = "contributions"; +const FORMS = "forms"; +const RESPONSES = "responses"; + +/** Two search requests per member against a 30-per-minute authenticated limit means one + * member every 4.5 seconds is the ceiling. 5s leaves a little headroom. */ +const GAP_MS = 5000; + +/** How many members one scheduled run will refresh. + * + * A CAP RATHER THAN A LONGER TIMEOUT, and the cap is the honest half of the design. + * Cloud Functions v2 stops at 3,600s; at GAP_MS per member a full club sweep would + * approach that as membership grows, and a run killed mid-sweep leaves an arbitrary half + * of the club stale with nothing saying so. Instead each run takes the 100 STALEST rows, + * so the whole club is covered every ceil(n/100) days in a deterministic order. At 100 + * members that is daily; at 400 it is every four days, and the dashboard's "checked N + * days ago" line is what makes that visible rather than mysterious. */ +const MAX_PER_RUN = 100; + +/** A member may ask for a refresh this often. Enforced HERE rather than in the client, + * because a client-side timer is a suggestion — and this is the only path by which a + * signed-in member can cause outbound requests, so it is the one that needs a limit. */ +const COOLDOWN_MS = 10 * 60 * 1000; + +const sleep = (ms) => new Promise((r) => setTimeout(r, ms)); + +/** Fetch one member's contributions and store them. + * + * `synced_at` is written even for a not_found handle, and deliberately: without it a + * typo'd handle would be retried on every run forever, and the cooldown would never + * engage for the person most likely to press the button repeatedly. */ +async function syncOne(uid, handle, token) { + const data = await fetchContributions(handle, token); + await admin + .firestore() + .collection(CONTRIBUTIONS) + .doc(uid) + .set({ uid, ...data, synced_at: admin.firestore.FieldValue.serverTimestamp() }); +} + +/** Read a row back as plain JSON, for handing to a callable's caller. + * + * NEEDED BECAUSE THE WRITE ABOVE STORES A SENTINEL, not a time. Returning the object + * that was written hands the client `{_methodName: "serverTimestamp"}`, which toDate() + * cannot read and which renders as "checked never" immediately after a successful sync — + * the one moment the reader is looking straight at it. */ +async function readOne(uid) { + const snap = await admin.firestore().collection(CONTRIBUTIONS).doc(uid).get(); + if (!snap.exists) return null; + const d = snap.data(); + return { + ...d, + // A Firestore Timestamp does not survive the callable's JSON encoding as anything + // useful, so it crosses as ISO and toDate() parses the string on the other side. + synced_at: d.synced_at?.toDate?.().toISOString() ?? null, + }; +} + +exports.syncContributions = onSchedule( + { + // 04:00 IST — the middle of the night for the only people who use this. The timezone + // is stated rather than left at UTC so the schedule means what it reads. + schedule: "0 4 * * *", + timeZone: "Asia/Kolkata", + secrets: [GITHUB_TOKEN], + timeoutSeconds: 3600, + // ONE INSTANCE, ALWAYS. Two overlapping sweeps would double the request rate against + // a limit this is carefully sized under, and the throttle would mean nothing. + maxInstances: 1, + retryCount: 0, + }, + async () => { + const token = GITHUB_TOKEN.value() || ""; + if (!token) { + logger.warn( + "GITHUB_TOKEN is not set. The sweep runs against the unauthenticated limit " + + "(10 searches a minute) and will not get far. Set it with: " + + "firebase functions:secrets:set GITHUB_TOKEN", + ); + } + + const db = admin.firestore(); + // Every member who gave a handle. `github` is optional on the profile, so this is a + // subset of the roster. + const members = await db.collection(USERS).get(); + const withHandles = members.docs + .map((d) => ({ uid: d.id, handle: (d.data().github ?? "").trim() })) + .filter((m) => isValidHandle(m.handle)); + + // STALEST FIRST, so MAX_PER_RUN rotates through the club rather than refreshing the + // same hundred people every night. Never-synced rows sort first, having no timestamp. + const existing = await db.collection(CONTRIBUTIONS).get(); + const syncedAt = new Map( + existing.docs.map((d) => [d.id, d.data().synced_at?.toMillis?.() ?? 0]), + ); + withHandles.sort((a, b) => (syncedAt.get(a.uid) ?? 0) - (syncedAt.get(b.uid) ?? 0)); + + const batch = withHandles.slice(0, MAX_PER_RUN); + logger.info( + `Syncing ${batch.length} of ${withHandles.length} members with a GitHub handle.`, + ); + + let done = 0; + let failed = 0; + for (const m of batch) { + try { + await syncOne(m.uid, m.handle, token); + done++; + } catch (err) { + failed++; + if (err.code === "rate-limit") { + // STOP, DO NOT CARRY ON. Once GitHub is refusing, every further request in this + // run is also refused — continuing turns one rate limit into a hundred, and the + // remaining members are picked up tomorrow anyway because they are now the + // stalest rows. + logger.warn(`Rate limited after ${done} members. Stopping; the rest are next run.`); + break; + } + // Anything else is one member's problem, not the sweep's. Logged and skipped. + logger.error("Could not sync one member.", { + uid: m.uid, + error: err instanceof Error ? err.message : String(err), + }); + } + await sleep(GAP_MS); + } + + logger.info(`Sync finished. ${done} updated, ${failed} failed.`); + }, +); + +exports.refreshContributions = onCall( + { secrets: [GITHUB_TOKEN], timeoutSeconds: 60, maxInstances: 10 }, + async (request) => { + const auth = request.auth; + // THE UID COMES FROM THE TOKEN, NEVER FROM A PARAMETER. That is the whole reason this + // is a callable rather than an HTTP endpoint: a uid passed in the body is a uid + // somebody can change, and this would become "refresh anybody's row". + if (!auth?.uid) throw new HttpsError("unauthenticated", "Sign in first."); + + // THE SAME DOMAIN RULE AS firestore.rules, RESTATED — because the Admin SDK below + // does not go through the rules at all. Without this, a Google account on any domain + // could call this function and spend the club's GitHub rate limit. If the domain in + // web/lib/firebase.ts changes, it changes here too. + const email = String(auth.token?.email ?? "").toLowerCase(); + if (auth.token?.email_verified !== true || !/^[^@]+@sst\.scaler\.com$/.test(email)) { + throw new HttpsError("permission-denied", "Members only."); + } + + const db = admin.firestore(); + const profile = await db.collection(USERS).doc(auth.uid).get(); + const handle = (profile.data()?.github ?? "").trim(); + if (!isValidHandle(handle)) return { ok: false, reason: "no-handle" }; + + const current = await db.collection(CONTRIBUTIONS).doc(auth.uid).get(); + const last = current.data()?.synced_at?.toMillis?.() ?? 0; + // The cooldown is SKIPPED when the stored row is for a different handle. Somebody who + // has just corrected a typo should not be told to wait ten minutes to see the fix — + // that is the one moment they will certainly press the button twice. + const sameHandle = + (current.data()?.github ?? "").trim().toLowerCase() === handle.toLowerCase(); + if (sameHandle && Date.now() - last < COOLDOWN_MS) { + return { ok: false, reason: "cooldown" }; + } + + try { + await syncOne(auth.uid, handle, GITHUB_TOKEN.value() || ""); + const stored = await readOne(auth.uid); + if (stored?.not_found) return { ok: false, reason: "not-found" }; + return { ok: true, contributions: stored }; + } catch (err) { + // Logged with the distinction, returned without it: "we are being throttled" and + // "GitHub is down" are different things to whoever reads the logs and the same + // sentence to a member, who can only try again either way. + logger.error("Refresh failed.", { + uid: auth.uid, + code: err.code ?? "unknown", + error: err instanceof Error ? err.message : String(err), + }); + return { ok: false, reason: "github-down" }; + } + }, +); + +// --------------------------------------------------------------------------------- +// POLL TALLIES +// +// Recomputes forms/{formId}.tally whenever a response is written. +// +// WHY A FUNCTION AND NOT A CLIENT-SIDE COUNTER. `tally` is refused to every client by +// firestore.rules — including the admin who created the form — because a count the client +// supplies is a count the client invented. The obvious alternative, letting a member +// increment a counter as they vote, cannot be made safe in rules: a rule can check that +// the new count is the old count plus one, but it CANNOT check that the increment came +// with an actual vote, so anybody could push a number up without answering anything. +// +// IT RECOMPUTES FROM SCRATCH RATHER THAN INCREMENTING, and that is the decision worth +// defending. Incrementing is O(1) and wrong in three ordinary situations: a member +// changing their answer (decrement the old, increment the new), a retried function +// invocation (double count), and any write that lands while another is in flight. Reading +// every response and counting is O(n) per write, which for a club poll is a few hundred +// documents — and it is ALWAYS right, including after a bug, because it derives the +// answer rather than accumulating it. If the club ever runs a poll with tens of thousands +// of responses, revisit this; at that point a distributed counter is the standard answer. +// +// ONLY `choice` AND `multi` FIELDS ARE COUNTED. Tallying free text would produce one +// bucket per person, which is not a tally. + +const { onDocumentWritten } = require("firebase-functions/v2/firestore"); + +exports.tallyResponses = onDocumentWritten( + { + document: "forms/{formId}/responses/{uid}", + // ONE AT A TIME PER DEPLOYMENT. Two invocations recomputing the same form + // concurrently would both read, both count, and the slower one would write a total + // that is missing the other's response. Serialising costs latency nobody perceives on + // a club poll and removes the whole class of lost update. + maxInstances: 1, + retry: false, + timeoutSeconds: 120, + }, + async (event) => { + const formId = event.params.formId; + const db = admin.firestore(); + + const formRef = db.collection(FORMS).doc(formId); + const form = await formRef.get(); + if (!form.exists) { + logger.warn("A response was written to a form that does not exist.", { formId }); + return; + } + + const data = form.data(); + // Nothing to show, nothing to compute. A sign-up sheet is not a poll and its + // organisers read the responses directly. + if (data.show_tally !== true) return; + + const counted = (data.fields ?? []).filter( + (f) => (f.type === "choice" || f.type === "multi") && Array.isArray(f.options), + ); + if (!counted.length) return; + + const responses = await formRef.collection(RESPONSES).get(); + + // Every option starts at zero, so an option nobody picked renders as "0" rather than + // vanishing from the bars — which would make a poll look like it had fewer choices + // than it offered. + const tally = {}; + for (const f of counted) { + tally[f.id] = {}; + for (const o of f.options) tally[f.id][o] = 0; + } + + for (const doc of responses.docs) { + const answers = doc.data().answers ?? {}; + for (const f of counted) { + const a = answers[f.id]; + // An answer naming an option the form no longer offers is DROPPED rather than + // added as a new bucket: options can be edited after responses arrive, and a + // tally that grew a row for a deleted option would read as a live choice. + for (const v of Array.isArray(a) ? a : [a]) { + if (typeof v === "string" && v in tally[f.id]) tally[f.id][v] += 1; + } + } + } + + // merge:true so this touches nothing else on the form. A full set would race with an + // organiser editing the question at the same moment and silently revert their edit. + await formRef.set({ tally }, { merge: true }); + logger.info("Tally updated.", { formId, responses: responses.size }); + }, +); diff --git a/functions/package.json b/functions/package.json index 581c0eb..be010fd 100644 --- a/functions/package.json +++ b/functions/package.json @@ -1,6 +1,6 @@ { "name": "osc-functions", - "description": "Emails the organisers when somebody applies to the club", + "description": "Emails the organisers when somebody applies, and syncs members GitHub contributions", "private": true, "main": "index.js", "engines": { @@ -9,7 +9,7 @@ "scripts": { "serve": "firebase emulators:start --only functions,firestore --project demo-osc", "logs": "firebase functions:log", - "test": "node test-format.mjs" + "test": "node test-format.mjs && node test-github.mjs" }, "dependencies": { "firebase-admin": "^13.0.2", diff --git a/functions/test-github.mjs b/functions/test-github.mjs new file mode 100644 index 0000000..d11475a --- /dev/null +++ b/functions/test-github.mjs @@ -0,0 +1,193 @@ +// Drive github.js against recorded GitHub payloads. +// +// node test-github.mjs +// +// NO NETWORK, NO CREDENTIALS, NO EMULATOR — the same bargain as test-format.mjs. Every +// function under test is a pure function of a fetch response, so `fetch` is replaced with +// a stub that returns whatever the case needs. That is what lets this run in CI on a +// checkout with no Firebase project and no GitHub token. +// +// WHAT IS WORTH TESTING HERE, and it is not "does it call the right URL": +// +// * the handle validator, because it is the one place member-supplied text reaches a +// URL, and because its regex is easy to get subtly wrong (leading hyphens, doubled +// hyphens, 40 characters); +// * "no such account" versus "an account with no pull requests", because they arrive +// from GitHub looking almost identical and the dashboard has to word them completely +// differently; +// * the distinct-repository count, because it is derived rather than returned, and the +// bug it replaced — counting only the eight rows the dashboard lists — produced a +// number that looked entirely plausible; +// * that a rate limit THROWS with a code rather than resolving to zeroes, because a +// silent zero would overwrite a real member's real counts with nothing. + +import { fetchContributions, isValidHandle, repoOf, toPull } from "./github.js"; + +let pass = 0; +let fail = 0; + +function ok(label, cond, detail = "") { + if (cond) { + pass++; + console.log(` PASS ${label}`); + } else { + fail++; + console.log(` FAIL ${label}${detail ? ` ${detail}` : ""}`); + } +} + +const eq = (label, actual, expected) => + ok(label, Object.is(actual, expected), `got ${JSON.stringify(actual)}, want ${JSON.stringify(expected)}`); + +/** A search result item, in the shape /search/issues actually returns. */ +const item = (n, repo = "octo/hello") => ({ + title: `Fix the thing ${n}`, + html_url: `https://github.com/${repo}/pull/${n}`, + repository_url: `https://api.github.com/repos/${repo}`, + pull_request: { merged_at: "2026-05-01T10:00:00Z" }, +}); + +/** Replace global fetch with a router keyed on a substring of the URL. Restored by the + * caller; every case installs its own. */ +function stubFetch(routes) { + globalThis.fetch = async (url) => { + for (const [needle, res] of routes) { + if (String(url).includes(needle)) { + return typeof res === "function" ? res() : res; + } + } + throw new Error(`unstubbed fetch: ${url}`); + }; +} + +const json = (body, status = 200) => ({ + ok: status >= 200 && status < 300, + status, + json: async () => body, +}); + +const realFetch = globalThis.fetch; + +console.log("\ngithub.js\n"); + +// ------------------------------------------------------------------ handles +console.log("-- handles --"); +for (const good of ["a", "octocat", "Octo-Cat", "a1-b2-c3", "x".repeat(39)]) { + ok(`"${good}" is a possible handle`, isValidHandle(good)); +} +for (const bad of ["", "-lead", "trail-", "double--hyphen", "x".repeat(40), "has space", "has/slash", "under_score", null, 42]) { + ok(`${JSON.stringify(bad)} is refused`, !isValidHandle(bad)); +} + +// ------------------------------------------------------------------ parsing +console.log("\n-- parsing one pull request --"); +eq("repo comes from repository_url", repoOf(item(1, "torvalds/linux")), "torvalds/linux"); +eq("an unparseable repository_url degrades", repoOf({ repository_url: "nonsense" }), "unknown"); +eq("a missing title does not render as empty", toPull({}, "merged").title, "(no title)"); +ok( + "a hostile title is truncated", + toPull({ title: "x".repeat(500) }, "merged").title.length === 160, +); +ok( + "merged_at is omitted rather than null when absent", + !("merged_at" in toPull({ title: "t" }, "open")), +); + +// --------------------------------------------------------------- the fetch +console.log("\n-- a member with contributions --"); +stubFetch([ + ["/users/", json({ login: "asha" })], + [ + "is%3Amerged", + json({ + total_count: 12, + // Ten items across four distinct repositories. The list the dashboard shows is + // capped at eight, so this is the case that catches a repo count derived from the + // truncated list instead of the full page. + items: [ + item(1, "a/one"), item(2, "a/one"), item(3, "b/two"), item(4, "b/two"), + item(5, "c/three"), item(6, "c/three"), item(7, "a/one"), item(8, "a/one"), + item(9, "d/four"), item(10, "d/four"), + ], + }), + ], + ["is%3Aopen", json({ total_count: 2, items: [] })], +]); +{ + const c = await fetchContributions("asha", ""); + eq("merged is the search total, not the page length", c.merged, 12); + eq("open is counted separately", c.open, 2); + eq("repos counts DISTINCT repositories across the whole page", c.repos, 4); + eq("the stored list is capped at RECENT", c.recent.length, 8); + eq("not_found is absent for a real account", c.not_found, undefined); + eq("every stored row is labelled merged", c.recent.every((p) => p.state === "merged"), true); +} + +console.log("\n-- a real account that has contributed nothing --"); +stubFetch([ + ["/users/", json({ login: "newbie" })], + ["is%3Amerged", json({ total_count: 0, items: [] })], + ["is%3Aopen", json({ total_count: 0, items: [] })], +]); +{ + const c = await fetchContributions("newbie", ""); + eq("counts are zero", c.merged + c.open + c.repos, 0); + // THE DISTINCTION THE UI DEPENDS ON. Zeroes with not_found absent means "nothing yet", + // which the dashboard words encouragingly. Zeroes WITH not_found means "check your + // handle for a typo". Collapsing them would tell a first-year they had made a mistake. + eq("but not_found stays absent", c.not_found, undefined); +} + +console.log("\n-- a handle that does not exist --"); +stubFetch([["/users/", json({ message: "Not Found" }, 404)]]); +{ + const c = await fetchContributions("nosuchuser", ""); + eq("not_found is set", c.not_found, true); + eq("and no search was attempted", c.recent.length, 0); +} + +console.log("\n-- failures --"); +stubFetch([["/users/", json({ message: "rate limit" }, 403)]]); +{ + let code = ""; + try { + await fetchContributions("asha", ""); + } catch (e) { + code = e.code; + } + // MUST THROW, NOT RESOLVE TO ZEROES. A silent zero here overwrites a member's real + // counts with nothing, and the dashboard would show it as fact. + eq("a rate limit throws with a code", code, "rate-limit"); +} + +stubFetch([["/users/", json({}, 500)]]); +{ + // 500 is deliberately NOT treated as "no such account" — see the comment in exists(). + // A transient GitHub blip must not flag a valid handle as a typo. + stubFetch([ + ["/users/", json({}, 500)], + ["is%3Amerged", json({ total_count: 1, items: [item(1)] })], + ["is%3Aopen", json({ total_count: 0, items: [] })], + ]); + const c = await fetchContributions("asha", ""); + eq("a 500 on the user lookup does not flag a typo", c.not_found, undefined); +} + +{ + let code = ""; + try { + await fetchContributions("-not-a-handle-", ""); + } catch (e) { + code = e.code; + } + eq("an impossible handle is refused before any request", code, "bad-handle"); +} + +globalThis.fetch = realFetch; + +console.log( + fail === 0 + ? `\n ${pass} passed.\n` + : `\n ${pass} passed, ${fail} FAILED.\n`, +); +process.exit(fail === 0 ? 0 : 1); diff --git a/web/app/(app)/admin/page.tsx b/web/app/(app)/admin/page.tsx index 15662ea..dd84f5f 100644 --- a/web/app/(app)/admin/page.tsx +++ b/web/app/(app)/admin/page.tsx @@ -1,47 +1,49 @@ import type { Metadata } from "next"; import AdminDashboard from "@/components/AdminDashboard"; +import AudienceBackfill from "@/components/AudienceBackfill"; +import Composer from "@/components/Composer"; +import FormBuilder from "@/components/FormBuilder"; +import Roster from "@/components/Roster"; +import Sessions from "@/components/Sessions"; -// THE ORGANISERS' DASHBOARD. Counts first, list second — see the header of -// AdminDashboard.tsx for what it is for and, more importantly, for what it is not: -// this route is not a privilege gate. The page ships to anybody who asks for it, -// because the site is a static export with no server to refuse them. What refuses -// them is the `list` rule on users/{uid} in firestore.rules, which only an address -// in the `admins` collection satisfies. A non-admin who loads this URL gets a page -// that cannot fetch anything. +// THE ORGANISERS' PAGE. Same shell as the member dashboard — it comes from +// (app)/layout.tsx, so this file is only the content. // -// NOT IN PAGES, so it appears in neither the nav strip nor the footer's route list. -// It is reached from the finished profile on /join, and only when the signed-in -// address is an admin. That is a convenience rather than concealment — the URL is -// guessable and that is fine. +// NOT A PRIVILEGE GATE. The page ships to anybody who asks for it, because the site is a +// static export with no server to refuse them. What refuses them is the `list` rule on +// users/{uid} and the admin-only writes on every collection below, none of which any +// client can talk its way past. A non-admin who loads this URL gets a page whose every +// panel renders its own "not for you" state. // -// `noindex`, because a page that lists members has no business in a search index -// even though it renders nothing without an authorised session. +// THE ORDER IS BY HOW OFTEN AN ORGANISER DOES THE THING: membership is the question the +// page is opened with, notices and sessions are weekly, forms every few weeks, and the +// roster once a term — which is why it is last, where nobody reaches it by accident. export const metadata: Metadata = { title: "Organisers", - description: "Club membership by batch, year, branch and hostel, and the mentorship cohort.", + description: "Club membership, sessions, notices and forms.", robots: { index: false, follow: false }, }; export default function Admin() { return ( -
-
-

Organisers only

-

- Who is in the club. +
+
+

+ Admin dashboard

-

- Every registered member and the breakdowns most often asked for — by batch, by - year, by branch, by hostel — then the mentor list and who has chosen whom. Counts - are over the whole membership; the search and filters below narrow only the - lists. +

+ Who is in the club, what they have been told, and what you have asked them.

+
-
- -
-

-
+ {/* Renders only while there is something to migrate — see the component. */} + + + + + + + ); } diff --git a/web/app/(app)/dashboard/page.tsx b/web/app/(app)/dashboard/page.tsx index 67bda5e..f8b835e 100644 --- a/web/app/(app)/dashboard/page.tsx +++ b/web/app/(app)/dashboard/page.tsx @@ -1,64 +1,23 @@ -import { Suspense } from "react"; import type { Metadata } from "next"; import MemberDashboard from "@/components/MemberDashboard"; -// A MEMBER'S OWN PAGE. Where signing in leads, and where everything the club asks a -// member to do afterwards lives. +// THE MEMBER'S DASHBOARD. The shell around it comes from (app)/layout.tsx, so this file is +// only ever the content — see that layout for why the chrome lives there. // -// It is also the one route that decides whether somebody still needs onboarding — /join -// sends every signed-in reader here without reading their profile, so that question has -// exactly one answer in exactly one place. See the header of MemberDashboard.tsx. +// NOT A PRIVILEGE GATE. The site is a static export, so this markup ships to anybody who +// asks for it. What refuses a stranger is firestore.rules: users/{uid} and +// contributions/{uid} are owner-or-admin, and the board, forms and sessions all require a +// verified college address. // -// NOT A PRIVILEGE GATE. This HTML ships to anybody who asks for it; the site is a static -// export with no server to refuse them. What refuses them is firestore.rules. See -// lib/auth.tsx before concluding that a page nobody links to is a page nobody can read. -// -// ABSENT FROM PAGES, so it is in neither the nav strip nor the footer's route list — the -// nav's one persistent button already changes to "Profile" and points here once somebody -// is signed in, and the same word in the strip as well would be the site's one action -// said twice. -// -// `noindex`, because a page that only means anything to one signed-in person has no -// business in a search index. +// `noindex`, because a page whose entire content is one member's own record has no +// business in a search index even though it renders nothing without a session. export const metadata: Metadata = { title: "Your dashboard", - description: "Your club details, and the programmes you are enrolled in.", + description: "Your details, what you have merged, and what the club has pinned up.", robots: { index: false, follow: false }, }; export default function Dashboard() { - return ( -
- {/* `.page-top` is the shared clearance under the floating header — see the note in - globals.css. It is the same number for the app shell because the app shell is - deliberately the same height as the site nav. */} -
- {/* THE CAP IS ON AN INNER DIV, NOT ON `.section`. This is the trap globals.css - documents against `.page-top` and `pt-*`, hit in the other direction: every - custom class in that file is declared after `@tailwind utilities`, so at equal - specificity source order hands the win to `.section` and its `max-w-[88rem]` - silently beats a `max-w-6xl` sitting next to it. The markup said 72rem and the - screen said 88rem, with nothing anywhere to explain the difference. - 72rem: the details are a four-tile row, which wants the room, and the mentor - picker is prose a person reads, which does not. This is where they stop - fighting. */} -
-

- Your club. -

- - {/* useSearchParams inside the dashboard needs a Suspense boundary, or - `next build` refuses to prerender this route — at build time rather than at - runtime, which is the good version of that error. The fallback reserves - roughly the first card's height so the page does not jump. */} -
- }> - - -
-
-
-
- ); + return ; } diff --git a/web/app/(app)/layout.tsx b/web/app/(app)/layout.tsx index c190b3a..b2a5d74 100644 --- a/web/app/(app)/layout.tsx +++ b/web/app/(app)/layout.tsx @@ -1,12 +1,14 @@ -import AppHeader from "@/components/AppHeader"; -import AppFooter from "@/components/AppFooter"; +import Shell from "@/components/dashboard/Shell"; // The SIGNED-IN area: /onboarding, /dashboard, /admin. // -// It carries none of the public site's chrome — see the note in (site)/layout.tsx for -// what was removed and why. What it has instead is a header that says who you are signed -// in as and a footer that carries the three privacy anchors, which are the only part of -// the site footer a member actually needs from here. +// It carries none of the public site's chrome — no six-link nav arguing for the club, no +// sitemap footer. Every reader here has already joined. What it has instead is the app +// shell: a flush top bar with the view switch, a sidebar, and a three-link footer. +// +// THE SHELL IS HERE RATHER THAN INSIDE EACH PAGE, which is the point of the route group +// and the reason it replaced the pathname check that did this job before. A page in this +// folder cannot forget the shell, and a page outside it cannot accidentally get one. // // THIS IS NOT A PRIVILEGE BOUNDARY. A layout decides what to paint, not who may read. // Every one of these routes is static HTML on a CDN and anybody can fetch it; what @@ -14,11 +16,5 @@ import AppFooter from "@/components/AppFooter"; // before concluding that an app shell makes anything safe. export default function AppLayout({ children }: { children: React.ReactNode }) { - return ( - <> - - {children} - - - ); + return {children}; } diff --git a/web/app/(site)/how-to-join/page.tsx b/web/app/(site)/how-to-join/page.tsx index 83fb046..4ecaf68 100644 --- a/web/app/(site)/how-to-join/page.tsx +++ b/web/app/(site)/how-to-join/page.tsx @@ -396,7 +396,7 @@ export default function HowToJoin() { // 11px, not 10: the QA sweep flags anything under 11px as // too small to read on a phone, and a decorative glyph is // not a reason to make an exception nobody can see. - className="mt-0.5 flex h-[1.15rem] w-[1.15rem] shrink-0 items-center justify-center rounded-full border border-haze/40 text-[13px] leading-none text-haze" + className="mt-0.5 flex h-[1.15rem] w-[1.15rem] shrink-0 items-center justify-center rounded-full border border-haze/40 text-[0.8125rem] leading-none text-haze" > ✕ @@ -635,7 +635,7 @@ export default function HowToJoin() { // small to read on a phone, and a decorative frame is no reason // to make an exception. The comment strings were shortened to // suit, rather than the frame widened into the sentence. - className="hidden w-44 shrink-0 self-start overflow-hidden rounded-xl border border-white/10 p-3 font-mono text-[13px] leading-relaxed lg:block" + className="hidden w-44 shrink-0 self-start overflow-hidden rounded-xl border border-white/10 p-3 font-mono text-[0.8125rem] leading-relaxed lg:block" style={{ background: "#0F172A" }} >

diff --git a/web/app/(site)/join/page.tsx b/web/app/(site)/join/page.tsx index 1379b3c..3838a45 100644 --- a/web/app/(site)/join/page.tsx +++ b/web/app/(site)/join/page.tsx @@ -1,8 +1,9 @@ import type { Metadata } from "next"; import Link from "next/link"; -import JoinGate from "@/components/JoinGate"; +import ApplyForm from "@/components/ApplyForm"; import Duo from "@/components/Duo"; import Note from "@/components/fx/Note"; +import { DASHBOARD_HREF } from "@/content/site"; // THE APPLICATION FORM. One route, one job. // @@ -170,19 +171,55 @@ export default function Join() {

No prior experience

+ + {/* THE WAY BACK IN. The nav carries a "Sign in" link of its own now, so + this is no longer the only thing between a returning member and their + dashboard — but it stays, because that link is sm+ only and because + this is the page somebody lands on when they press "Join" out of habit. + It is the sentence that stops them filling in an application they have + already sent. + + BEFORE THE FORM, NOT AFTER IT. Under the fields it would be found by + somebody who had already filled them in, which is the one moment the + sentence is no longer useful — a second application is exactly what it + exists to prevent. + + Small and quiet on purpose: almost nobody reading this page is a + member, and a sign-in prompt with equal weight to the form would ask + every first-time reader to work out which of two things they are. + + SAME TAB, AND IT USED TO OPEN A NEW ONE. `target="_blank"` on an + internal route left this application page open behind the reader as a + stale, signed-out copy of a site they had just signed into, and left + the back button — the thing somebody presses the moment they realise + they are in the wrong place — doing nothing at all. Signing in is not + a reference you consult beside the form; it is where you were going + instead of filling it in. */} +

+ Already joined?{" "} + + Sign in to your dashboard + {" "} + — no need to apply twice. +

- {/* WAS , THE ANONYMOUS ONE-SHOT FORM. It is now sign-in - first: register with a college Google account, then fill a profile - once that you can come back and edit. The column this sits in, the - copy beside it and the two tiles above are unchanged — the flow - changed, not the page. - - Everything about the gate is client-side, because the site is a - static export. Route-gating is therefore cosmetic and the data is - protected by firestore.rules instead. See the note at the top of - lib/auth.tsx before assuming a hidden page is a safe one. */} - + {/* THE ANONYMOUS APPLICATION FORM, and it is back here after a spell as a + sign-in gate. For a while this column held : register with + a college Google account first, then fill a profile. That put an + account requirement in front of the club's front door — a stranger + could not apply without already holding the thing that membership + grants — and it made the headline three inches to the left false at + the exact moment somebody acted on it. + + Sign-in did not go away; it stopped being this page's business. It + lives on /dashboard now (components/SignInCard.tsx), which is the one + place that genuinely needs to know who you are. This page asks, that + page identifies, and neither has to care about the other. + + The column this sits in, the copy beside it and the two tiles above + are unchanged — the flow changed, not the page. */} + diff --git a/web/app/(site)/page.tsx b/web/app/(site)/page.tsx index 3b18366..90aa207 100644 --- a/web/app/(site)/page.tsx +++ b/web/app/(site)/page.tsx @@ -74,11 +74,21 @@ function Sources({ cell }: { cell: Cell }) { // guessed, and invisible until the hover underline gave the row a visible // hover state to disagree with. // - // 12px of SYMMETRIC padding instead. No negative margin, so no overlap, and - // 19px of line box plus 24px lands at 43px — over the 40px floor scripts/qa.mjs - // enforces, which is the stricter of the two numbers in play (WCAG 2.5.8 asks - // 24px at AA; 2.5.5 and Apple's HIG want 44). A first pass used 3px and cleared - // 24 but not 40, trading the overlap bug for a small-target one. + // 14px of SYMMETRIC padding instead. No negative margin, so no overlap, and + // 16px of line box plus 28px lands at 44px — which is the HIGHER of the two + // numbers in play rather than a squeak past the lower one (WCAG 2.5.8 asks + // 24px at AA; 2.5.5 and Apple's HIG want 44, and scripts/qa.mjs enforces 40). + // A first pass used 3px and cleared 24 but not 40, trading the overlap bug for + // a small-target one. + // + // IT WAS 12px UNTIL THE TYPE SCALE CAME BACK DOWN, and the failure is worth + // recording because of how narrow it was. At the old `text-xs` of 14px the box + // measured 42.67px and passed; at 12px it measures 39.9996px, and the sweep + // tests `height < 40`. Four ten-thousandths of a pixel, reported as "40x153" — + // a number that looks like it should pass, in a check it fails. The 1.3333 + // leading ratio is where the fraction comes from. Padding to 44 rather than + // back to 40 is what stops the same 2px anywhere in this scale from doing it + // again. // The cost is about 20px of extra height per citation, in the two cells that // carry more than one. {s.label} ↗ @@ -159,7 +169,7 @@ export default function Home() { {/* Same reason as the build-day cards: shrink-0 on text from a data file is a viewport overflow waiting for a longer value. */} - + {e.language} diff --git a/web/app/(site)/privacy/page.tsx b/web/app/(site)/privacy/page.tsx index 7e3f5f7..d4ea4f0 100644 --- a/web/app/(site)/privacy/page.tsx +++ b/web/app/(site)/privacy/page.tsx @@ -184,7 +184,7 @@ export default function Privacy() { -

+

Something here wrong, or out of date against the code? This site is one of the club's own repositories —{" "} diff --git a/web/app/(site)/programmes/page.tsx b/web/app/(site)/programmes/page.tsx index a97f64e..96c8439 100644 --- a/web/app/(site)/programmes/page.tsx +++ b/web/app/(site)/programmes/page.tsx @@ -91,7 +91,7 @@ function ProgrammeField({ p }: { p: ProgrammeInfo }) { {/* The tier, stated in words as well as carried by the colour. The colour is never the only signal. */}

{["Window", "Programme", "Opens", "Start prepping", "What you do first"].map((h) => ( - + {h} ))} @@ -560,13 +560,13 @@ export default function Programmes() { // as too small to read on a phone, and it flags every line of // these preview frames. Same fix already applied to the bento // frames further up this file. - className="ml-1.5 font-mono text-[13px]" + className="ml-1.5 font-mono text-[0.8125rem]" style={{ color: "#94A3B8" }} > {track.preview.title} -

+
{track.preview.lines.map((l) => (

+ {p.size} )} @@ -139,7 +139,7 @@ export default function Projects() { {p.stack.map((s) => (

  • {s}
    • @@ -277,7 +277,7 @@ export default function Projects() { {r.stack.map((s) => (
  • {s}
    • @@ -371,7 +371,7 @@ export default function Projects() { {/* The org, set as type in a bordered plate rather than as a logo. Their trademark, and the site's CSP blocks remote images anyway — see content/projects.ts. */} - + {p.org} {p.tag ? ( @@ -441,7 +441,7 @@ export default function Projects() { )} -

    +

    Contributor counts and merge ratios were read from the GitHub API on 2026-07-29. They move — open the repository if you want today's number.

    diff --git a/web/app/globals.css b/web/app/globals.css index 2cb2ecd..1a638fb 100644 --- a/web/app/globals.css +++ b/web/app/globals.css @@ -426,7 +426,7 @@ html { scroll-padding-top is load-bearing alongside it: the nav is `fixed` and outside flow, so an anchor target would otherwise come to rest behind the plate. The browser applies this offset to every scroll-into-view — the nav - links, the outline rail, the sticky CTA and a deep link pasted into the + links, the outline panel, the sticky CTA and a deep link pasted into the address bar all land in the same place. The number tracks the nav, and the nav FLOATS: 0.75rem of top inset plus a @@ -436,8 +436,37 @@ html { plate went from 3rem to 3.5rem when the Join button had to reach a 44px touch target, and this number went with it. */ scroll-behavior: smooth; - scroll-padding-top: 5.5rem; + /* IN PX, BECAUSE THE THING IT CLEARS IS IN PX. The plate holds a 44px touch + target and so keeps a 56px height while the root scales (see `font-size` + below): 0.75rem of inset plus 56px is a 65px bottom edge, 68px at sm+ where + the inset grows. 80px clears the taller of the two with air to spare. */ + scroll-padding-top: 80px; -webkit-text-size-adjust: 100%; + + /* THE WHOLE PAGE, AT THREE QUARTERS. The design was drawn large enough that a + 1080p laptop had to be zoomed to 75% before a section fit on screen — which + is the browser telling you the root is too big, not the reader. One number + here does what that zoom did, at every reader's default zoom, on the site and + in the dashboard alike: 75% of 16px is a 12px root, and every rem on the page + — type scale, spacing, radii, the nav plate, the container caps — is a + multiple of it. + + A PERCENTAGE RATHER THAN `font-size: 12px`, and the difference matters. A + percentage resolves against whatever the reader set as their browser's + default text size, so somebody running 20px because they need to still gets + their proportional increase (15px here). A hard px value would overrule that + preference outright, which is the accessibility failure this looks like but + is not. + + WHAT IT DOES NOT SCALE, deliberately: the px in this file. Hairlines, borders + and the 44px minimum touch targets are physical constants — a 0.75px rule is + a blurry rule, and a thumb does not get smaller because the type did. Type + was the part that had to move, so `text-[13px]`-style utilities were all + rewritten in rem to come with it, and the vw terms in the fluid clamps came + down by the same quarter — leave those in px and the headings would hold + their old size across the middle of the viewport range while everything + around them shrank. */ + font-size: 75%; } body { @@ -498,7 +527,7 @@ body::before { blocks would be a fairground. */ .label { font-family: var(--font-label), system-ui, sans-serif; - font-size: 1rem; + font-size: 0.875rem; /* Stated, not inherited from the face — see the type note above. */ font-weight: 600; letter-spacing: 0.07em; @@ -542,10 +571,15 @@ body::before { it per route is a copy per route that can drift. THE DERIVATION: the nav floats rather than sitting on the top edge — 0.75rem of - inset plus a 3.5rem plate puts its bottom at 4.25rem, and 4.5rem at sm+ where the - inset grows. These values clear that with 1.75rem / 3rem of air, so a page title - has room to breathe under the glass instead of tucking against it. Move the nav's - inset or height and this moves too, along with `scroll-padding-top` above. + inset plus a 56px plate puts its bottom at 65px, and 68px at sm+ where the inset + grows. These values clear that with 21px / 36px of air, so a page title has room + to breathe under the glass instead of tucking against it. Move the nav's inset or + height and this moves too, along with `scroll-padding-top` above. + + PX ON BOTH SIDES OF THAT SUM. The plate stopped scaling with the root when the + page went to 75% — it contains a 44px touch target, which is a physical floor — + so a clearance in rem would have shrunk away from a bar that did not. The air + either side is the old 28px / 48px taken down by the same quarter. DO NOT PAIR THIS WITH A pt-* UTILITY. Every custom class in this file is declared after `@tailwind utilities`, so at equal specificity source order hands the win to @@ -553,11 +587,11 @@ body::before { markup and 6rem on the screen. Use `.page-top` for the clearance and `pb-*` for whatever the band needs underneath. */ .page-top { - padding-top: 6rem; + padding-top: 86px; } @media (min-width: 640px) { .page-top { - padding-top: 7.5rem; + padding-top: 104px; } } @@ -642,7 +676,7 @@ body::before { everything beside it. The plate had room at 14px with the nav well short of its wrap point; 16px spends most of that margin, so the sm breakpoint is worth a look if a seventh link is ever added. */ - font-size: 1rem; + font-size: 0.875rem; font-weight: 500; letter-spacing: -0.005em; transition: color 180ms ease-in-out; @@ -703,7 +737,7 @@ body::before { background: rgb(var(--accent)); color: rgb(var(--bg)); font-family: var(--font-label), system-ui, sans-serif; - font-size: 0.875rem; + font-size: 0.75rem; font-weight: 700; letter-spacing: 0.1em; text-transform: uppercase; @@ -1402,7 +1436,7 @@ body::before { background: rgb(var(--accent)); color: rgb(var(--bg)); font-family: var(--font-label), system-ui, sans-serif; - font-size: 0.9375rem; + font-size: 0.8125rem; font-weight: 800; letter-spacing: 0.04em; font-variant-numeric: tabular-nums; @@ -1522,7 +1556,7 @@ body::before { background: var(--tint-soft); color: var(--tint-ink); font-family: var(--font-label), system-ui, sans-serif; - font-size: 1rem; + font-size: 0.875rem; font-weight: 800; letter-spacing: 0.02em; font-variant-numeric: tabular-nums; @@ -1546,7 +1580,7 @@ body::before { beside it, it has lowercase to be short of, and at 12px Plus Jakarta Sans's x-height put it below anything else readable here. The caps-bearing pills stay at 0.75rem for the reason given in tailwind.config.ts. */ - font-size: 0.9375rem; + font-size: 0.8125rem; font-weight: 700; letter-spacing: 0.01em; line-height: 1.2; @@ -1853,7 +1887,7 @@ body::before { border: 2px solid #000000; box-shadow: 3px 3px 0 0 #000000; font-family: var(--font-label), system-ui, sans-serif; - font-size: 0.875rem; + font-size: 0.75rem; font-weight: 700; letter-spacing: 0.02em; line-height: 1; @@ -1965,7 +1999,7 @@ body::before { background: #0f172a; color: #e2e8f0; font-family: var(--font-sans), system-ui, sans-serif; - font-size: 0.9375rem; + font-size: 0.8125rem; font-weight: 500; line-height: 1.35; letter-spacing: 0; @@ -2040,7 +2074,7 @@ body::before { background: #0f172a; color: #e2e8f0; font-family: var(--font-sans), system-ui, sans-serif; - font-size: 0.875rem; + font-size: 0.75rem; font-weight: 500; /* The captions above are uppercase and tracked out; this is prose, so it resets both — otherwise it inherits the chart's caption feel and reads as another @@ -2273,7 +2307,7 @@ body::before { margin: 0.5rem 0 0; padding: 0; list-style: none; - font-size: 0.875rem; + font-size: 0.75rem; line-height: 1.5; color: rgb(var(--haze)); } @@ -2311,7 +2345,7 @@ body::before { background: #0f172a; color: #e2e8f0; font-family: var(--font-mono), ui-monospace, monospace; - font-size: 0.8125rem; + font-size: 0.6875rem; line-height: 1.4; opacity: 0; transform: translateY(6px); @@ -2347,7 +2381,7 @@ body::before { and lands at 6.98 light / 5.63 dark, which is why .btn-primary uses it too. */ color: rgb(var(--bg)); font-family: var(--font-label), system-ui, sans-serif; - font-size: 1.125rem; + font-size: 1rem; font-weight: 700; letter-spacing: 0.02em; font-variant-numeric: tabular-nums; @@ -2375,13 +2409,13 @@ body::before { padding: 11px 21px; border-radius: 10px; font-family: var(--font-label), system-ui, sans-serif; - /* 17px/700, up from 15px/700 with the +2px pass. WATCH THE HERO PAIR AT 390px. - This step was 17px once before, under Staatliches, and moving to Plus Jakarta - Sans — which runs about a third wider at the same size — is what forced it down - to 15. The weight is unchanged and the labels are short, so the two hero buttons - should still hold one line, but this is the one control on the page where the - bump lands closest to a known wrap point rather than in open space. */ - font-size: 1.0625rem; + /* 15px/700, back down with the rest of the page after the +2px pass took it to 17. + 15 is where this step sat under Plus Jakarta Sans, which runs about a third wider + at the same size than the Staatliches it replaced — so the hero pair, the one + place on the page where this control comes near a wrap point rather than sitting + in open space, is being returned to a width that was known to hold one line at + 390px rather than moved somewhere new. */ + font-size: 0.9375rem; font-weight: 700; letter-spacing: 0.04em; text-transform: uppercase; @@ -2451,7 +2485,7 @@ body::before { .btn-compact { min-height: 44px; padding: 0 16px; - font-size: 0.9375rem; + font-size: 0.8125rem; } /* The electric-blue indicator on the secondary CTA. A right-aligned bolt in a @@ -2469,7 +2503,7 @@ body::before { border-radius: 5px; background: rgb(var(--accent)); color: rgb(var(--bg)); - font-size: 0.875rem; + font-size: 0.75rem; line-height: 1; } @@ -2670,30 +2704,19 @@ body::before { } } -/* With the outline panel open, the page reserves room for it rather than being - covered by it. Written as a root-level attribute so one flag moves everything — - the panel is 13.5rem plus its right offset, so 15rem clears it with margin. - Only at lg+, which is the only place the panel can appear at all. */ -@media (min-width: 1024px) { - :root[data-outline="1"] main { - padding-right: 15rem; - } +/* THE OUTLINE PANEL RESERVES NO PAGE WIDTH, and the rules that used to do it are + gone rather than mislaid. It was a rail pinned to the right of the viewport, so + `:root[data-outline="1"] main { padding-right: 15rem }` could hold a gutter open + underneath it — along with a correction to `.band::before` and `.stories`, whose + full-bleed `50% - 50vw` offsets assume the section is centred and stop being + true the moment one side is padded. - /* The band's full-bleed offsets assume the section is centred in the viewport: - `50% - 50vw` equals minus its own left inset only when the gaps either side - are equal. Reserving 15rem on the right breaks that, so the band stopped - 15rem short of the right edge and left a strip of page background beside it. - Caught by looking at it in Chrome — no automated check here measures whether - a decorative band reaches the edge. - - Derivation: with padding-right P the section's left inset becomes - (V - P - W)/2, which is P/2 less than the centred case, so each offset shifts - by P/2 = 7.5rem — left inward, right outward. */ - :root[data-outline="1"] .band::before { - left: calc(50% - 50vw + 7.5rem); - right: calc(50% - 50vw - 7.5rem); - } -} + The panel now opens as a menu under its own toggle in the nav (see + components/Outline.tsx), which ends that arrangement two ways over: its distance + from the right edge is whatever the nav's right-hand group measures rather than a + constant CSS can name, and a page that jumps 240px sideways when a dropdown opens + is a worse thing to do to a reader than briefly covering the corner it opens + over. Selecting an entry closes it. */ /* An alternating section band. Full-bleed by design — Apple's grey sections run edge to edge while the content inside keeps its own measure, which is what @@ -2846,10 +2869,11 @@ body::before { gap: 1.75rem; padding-right: 1.75rem; font-family: var(--font-label), system-ui, sans-serif; - /* Was exempt from the x-height bump the sans steps took, being capitals — see the - note in tailwind.config.ts for why that exemption does not carry over to the - +2px pass. Still clear of the 11px floor the QA sweep enforces. */ - font-size: 1rem; + /* Was exempt from the x-height bump the sans steps took, being capitals, then not + exempt from the +2px pass — see the note in tailwind.config.ts for why a uniform + instruction overrides a per-face one. That pass is reversed and this comes back + with it, still clear of the 11px floor the QA sweep enforces. */ + font-size: 0.875rem; font-weight: 800; letter-spacing: 0.12em; text-transform: uppercase; @@ -2861,7 +2885,7 @@ body::before { /* --sky rather than --accent: this is a decorative glyph at display weight, not a 13px link, so it takes the brighter step. */ color: rgb(var(--sky)); - font-size: 0.875rem; + font-size: 0.75rem; } /* Scrolls, but without a bar drawn through the nav's bottom hairline. */ @@ -3276,13 +3300,6 @@ button:focus-visible .icon-slide { position: relative; margin-inline: calc(50% - 50vw); } -@media (min-width: 1024px) { - :root[data-outline="1"] .stories { - margin-left: calc(50% - 50vw + 7.5rem); - margin-right: calc(50% - 50vw - 7.5rem); - } -} - /* No padding and no gap on the rail: a slide is exactly one rail-width, so the inset that lines its content up with the rest of the page has to live INSIDE the slide, where it is `.section` doing its normal job rather than a second @@ -3590,31 +3607,38 @@ button:focus-visible .icon-slide { /* THE COMMIT GRAPH'S TWO LANES, plotting themselves left to right. - NOT the [data-draw] rule the heading underlines use, and the reason is measured - rather than stylistic. That rule leans on `pathLength="1"` to make one dash - value serve paths of any length; this SVG also carries - `vector-effect: non-scaling-stroke`, which keeps its rails 2px at every width - and, as a side effect, makes dash lengths resolve in SCREEN PIXELS. The two - together produce a dash of "the whole path" = 304 user units read as 304px - against a lane rendered 1199px long, i.e. four dashes and three gaps in what is - supposed to be one continuous line. It rendered as a broken diagram. - - So each lane declares its own length in container-width units instead — 95cqw - for the 304-unit rail, 57cqw for the 181-unit branch, both authored at the path - in CommitGraph.tsx. Those track the rendered width of a `w-full` svg exactly, so - one dash covers one lane at every viewport, and the fallback keeps a - never-visible lane from becoming a permanently invisible one if a future call - site forgets the variable. + NOT the [data-draw] rule the heading underlines use, and not a stroke dash at + all, for a reason that is measured rather than stylistic. Every dash technique + needs to know how long the path is, and this SVG carries + `vector-effect: non-scaling-stroke` to keep its rails 2px at every width — which + takes dash lengths out of the viewBox scale and leaves them resolving against + the rendered picture instead. Whose pixels, though, is not the same answer in + every browser. `pathLength="1"` gave a dash of 304 USER units read as 304px + against a lane 1199px long: four dashes and three gaps. Sizing the dash in + container-width units instead (95cqw for the rail, 57cqw for the branch) fixed + that at 1x and broke again on a 150%-scaled Windows display, where 1077 CSS px + of dash was read as 1077 DEVICE px against a rail 1701 device px long and each + lane stopped two thirds of the way across. Both times it rendered as a broken + diagram, because both times a length had to be guessed in someone else's unit. + + So the lane is not uncovered by a dash, it is WIPED by a clip — off the path's + own bounding box, which the browser measures itself. Both lanes only ever + advance in x, so a wipe and a draw are the same picture, and there is no length + left in the effect to be wrong about. Nothing needs retuning when a path or a + breakpoint changes. + + The -6px of slack on the three edges that do not animate is because a clip + takes the FILL box: the grey rail's bounding box is a line of zero height, and + without the bleed the clip would shave off its 2px stroke and its round caps. Same 0.7s and same 0.15s step behind the block as the heading rules, so the two drawn things on the page share one rhythm. */ :root.reveal-on [data-reveal-group] .graph-lane { - stroke-dasharray: var(--lane, 100vw); - stroke-dashoffset: var(--lane, 100vw); - transition: stroke-dashoffset 0.7s ease calc(var(--reveal-delay, 0s) + 0.15s); + clip-path: inset(-6px 100% -6px -6px); + transition: clip-path 0.7s ease calc(var(--reveal-delay, 0s) + 0.15s); } :root.reveal-on [data-reveal-group].is-in .graph-lane { - stroke-dashoffset: 0; + clip-path: inset(-6px -6px -6px -6px); } /* The nodes on the commit graph, popping in along the lane the path has just diff --git a/web/components/AdminDashboard.tsx b/web/components/AdminDashboard.tsx index 23f7598..044a5f6 100644 --- a/web/components/AdminDashboard.tsx +++ b/web/components/AdminDashboard.tsx @@ -39,7 +39,14 @@ import AdminMentorship from "@/components/AdminMentorship"; import { Bars, Counts, ctl, labelOf, tally } from "@/components/admin/ui"; import { useAuth } from "@/lib/auth"; import { batchBucket, branchBucket, yearBucket } from "@/lib/batch"; -import { fmtDate, readAllProfiles, toDate, type Profile } from "@/lib/profile"; +import { + fmtDate, + isClubMember, + readAllProfiles, + setMembership, + toDate, + type Profile, +} from "@/lib/profile"; import { readAllEnrollments, readMentors, type Enrollment, type Mentor } from "@/lib/mentorship"; import { HOSTELS, PATHS } from "@/content/join"; @@ -53,6 +60,10 @@ function weekStart(d: Date): Date { export default function AdminDashboard() { const { user, isAdmin } = useAuth(); + /** The uid whose membership is being written, so one row can show it is busy without + * freezing the table. Null when nothing is in flight. */ + const [saving, setSaving] = useState(null); + const [memberError, setMemberError] = useState(""); const [rows, setRows] = useState(null); const [mentors, setMentors] = useState(null); const [enrollments, setEnrollments] = useState(null); @@ -71,6 +82,54 @@ export default function AdminDashboard() { /** Populated only when the clipboard refused, so the addresses are still gettable. */ const [emailList, setEmailList] = useState(""); + /** Admit somebody to the club, or take them back out. + * + * THE TABLE IS UPDATED FROM THE WRITE, NOT RE-READ. A full reload of every profile to + * reflect one changed field would cost a read per member and visibly redraw the + * table under the organiser's cursor — and the one thing that changed is the one + * thing this function already knows. A failed write puts the row back and says so, + * rather than leaving the screen claiming something the database refused. + * + * `membership_at` IS SET TO null RATHER THAN A LOCAL Date. The server stamps the real + * value and this row is not re-read; writing `new Date()` here would put a + * client clock into the table where every other timestamp came from the server, and + * it would be wrong by however far the two disagree. Nothing on this screen renders + * it, so null is the honest placeholder until the next full load. + */ + const toggleMembership = useCallback( + async (r: Profile) => { + if (!user?.email) return; + const next = !isClubMember(r); + setMemberError(""); + setSaving(r.uid); + try { + await setMembership(r.uid, next, user.email); + setRows((prev) => + prev + ? prev.map((x) => + x.uid === r.uid + ? { + ...x, + membership: next ? "member" : "student", + membership_by: user.email ?? undefined, + membership_at: null, + } + : x, + ) + : prev, + ); + } catch (e) { + console.error("[osc] could not change membership", e); + setMemberError( + `Could not change membership for ${r.name || r.email}. The rules refused it, or the connection dropped.`, + ); + } finally { + setSaving(null); + } + }, + [user?.email], + ); + const load = useCallback(async () => { setError(""); setReloading(true); @@ -269,7 +328,7 @@ export default function AdminDashboard() { return (
    {error && ( -

    +

    {error}

    )} @@ -306,7 +365,7 @@ export default function AdminDashboard() { const peak = Math.max(1, ...stats.weeks.map(([, x]) => x)); return (
    - {n || ""} + {n || ""} {/* A minimum height on a zero week, so the axis reads as a row of weeks rather than stopping wherever the data stopped. */}
    ( {/* Every other label only — eight dates side by side collide below about 700px and there is no room for rotation in a 6rem block. */} @@ -346,7 +405,7 @@ export default function AdminDashboard() { />
    -

    +

    Batch, branch and year are read from each member's college address rather than asked for — 23bcs10045 is the 2023–27 batch, branch BCS. An address that does not follow that pattern is counted as{" "} @@ -462,7 +521,7 @@ export default function AdminDashboard() { value={emailList} onFocus={(e) => e.currentTarget.select()} rows={3} - className="w-full resize-y rounded-md border border-seam bg-sunk p-3 font-mono text-[13px] text-haze" + className="w-full resize-y rounded-md border border-seam bg-sunk p-3 font-mono text-[0.8125rem] text-haze" />