Docs/authentication

[DevOlabode]

Documentation for the authentication feature of this project.

[ieliofficial]

Nice work on this 👏 The structure is solid: clear phases, env setup, troubleshooting, and acceptance criteria. That's how a real design doc is laid out, so the instincts are right.

I read through it closely. One framing suggestion first, then the specific technical issues (each with a quick diagram).

Suggestion: split this into two docs

Right now the file mixes two jobs:

1. The design: what the auth + storage flow should look like (OAuth, callback, session, storage selection, save scan). This part is genuinely good and reads well as a reference.
2. The implementation: concrete "create this file, paste this code" steps.

The problem is that several code snippets won't actually run against the current codebase (details below), so anyone following them literally hits errors, even though the design around them is fine. Mixing the two means a small code mistake undermines a doc that's otherwise useful.

I'd suggest:

• Keep this file as the design doc (explain the flow, the security model, the decisions, light on copy-paste code, or clearly mark snippets as illustrative).
• Spin out a separate working TODO doc (e.g. a checklist of implementation steps) that we tick off as the real, tested code lands in follow-up PRs.

That way the design can merge now and be useful, while the implementation gets verified piece by piece instead of being asserted as done.

Technical issues in the implementation snippets

These matter for the TODO/implementation side. Even in a design doc, worth correcting so the intent is accurate.

1. Auth routes get wired twice, in two incompatible ways.
Step 6 adds mountAuthRoutes(app) in app.js, and Step 8 also adds app.use('/api/auth', makeAuthRouter()) in routes/index.js, which registers the routes twice. The export is inconsistent too: routes/auth.js does module.exports = router (an instance), but it's called both as a function mountAuthRoutes(app) and as a factory makeAuthRouter(). Our convention is the factory style, see makeScanRouter(scanController) in routes/index.js. Pick that, wire it in one place, and match the export.

flowchart TD
  A["app.js<br/>mountAuthRoutes(app)"] --> R["/api/auth routes"]
  B["routes/index.js<br/>app.use('/api/auth', makeAuthRouter())"] --> R
  R --> D["Registered twice<br/>+ export shape mismatch"]

Loading

2. Frontend calls the wrong paths.
Routes mount under /api/auth/..., but apiClient calls /auth/github, /auth/status, etc., missing /api, so they'll 404. runScan in the same file already does /api/scan correctly; match it.

flowchart LR
  FE["apiClient<br/>GET /auth/github"] -->|missing /api| X["404 Not Found"]
  BE["backend mount<br/>/api/auth/github"]
  REF["runScan -> /api/scan ✅"]

Loading

3. Frontend and backend disagree on the auth model.
Backend uses session cookies (passport + express-session); frontend expects a Bearer token (accessToken from localStorage in an Authorization header). The backend never returns a token, and fetch isn't given credentials: 'include', so the cookie isn't sent either. Neither half can authenticate the other. Pick sessions or tokens and make both sides agree.

sequenceDiagram
  participant FE as Frontend
  participant BE as Backend
  Note over FE: sends Authorization: Bearer (from localStorage)
  Note over BE: expects session cookie
  FE->>BE: request (no cookie, no credentials:'include')
  BE-->>FE: response (sets session cookie, no token in body)
  Note over FE,BE: each side ignores what the other sends

Loading

4. StorageService calls methods that live on AuthService.
getOrCreateStorage/saveScanResults call this.getGitHubClient(...) / this.getGoogleDriveClient(...), but those are defined on AuthService, so this won't find them and it throws a TypeError. Either move the helpers into StorageService or pass the built client in as an argument.

classDiagram
  class AuthService {
    +getGitHubClient()
    +getGoogleDriveClient()
  }
  class StorageService {
    +getOrCreateStorage()
    +saveScanResults()
  }
  StorageService ..> AuthService : calls this.getGitHubClient() but it lives here, not on this

Loading

5. The user loses its tokens after the first request.
deserializeUser is stubbed to done(null, { id }), but it runs on every request to rebuild req.user. After login, req.user only has { id }, with no storage and no access token, so the scan-save path (req.user.storage) fails. Fine to leave the stub, but flag it with a // TODO: load full user from DB.

sequenceDiagram
  participant FE as Frontend
  participant BE as Backend
  FE->>BE: login callback
  Note over BE: req.user = full user (tokens, storage)
  FE->>BE: next request (POST /api/scan)
  Note over BE: deserializeUser -> req.user = { id } only
  BE-->>FE: fails: req.user.storage is undefined

Loading

6. Backend imports a frontend ESM module.
routes/auth.js does require('../../frontend/src/data/placeholders').PROVIDERS. That file is ESM (export const), the backend is CommonJS (require), so the import throws, and reaching into the frontend tree crosses the layer boundary. PROVIDERS isn't even used there; drop the import.

flowchart LR
  subgraph BE["backend (CommonJS)"]
    A["routes/auth.js<br/>require(...)"]
  end
  subgraph FE["frontend (ESM)"]
    B["placeholders.js<br/>export const PROVIDERS"]
  end
  A -->|require an ESM file<br/>across layers| B
  B --> X["throws + breaks layering<br/>(and PROVIDERS unused)"]

Loading

7. Two API calls aren't valid as written.
githubClient.repos.getForAuthenticatedUser({ repo }) isn't a real Octokit method, and the Google client isn't built correctly for a raw access token.

GitHub
  ✗ githubClient.repos.getForAuthenticatedUser({ repo })
  ✓ githubClient.repos.get({ owner, repo })

Google
  ✗ new google.auth.GoogleAuth({ credentials: { access_token } })
  ✓ const c = new google.auth.OAuth2()
    c.setCredentials({ access_token })

8. Minor: the app.js snippet uses require('../services/storageService'), but every other import there is ./services/..., so ../ points one folder too high.

backend/
├─ app.js            <-- you are here
├─ services/
│   └─ storageService.js   ✓ ./services/storageService
└─ ...
../services/storageService -> backend/../services -> WRONG (one level up)

One inconsistency to clean up

The acceptance criteria say "✅ No frontend changes required," but Phase 2 is entirely frontend changes (and references a setAuthed / bare name that aren't defined after switching to setUser). Worth making the doc agree with itself.

How to verify all of this yourself (the fundamentals)

First: writing the docs before the code was a genuinely good call. When you're doing something for the first time, laying out the plan in writing is how you find the gaps before they cost you hours of debugging. That instinct is worth keeping.

And none of the findings above needed anything clever. Every one is just checking whether what's written is actually true. The doc says "call this method", "import from here", "the route is X"; each of those is a claim, and reviewing is nothing more than opening the real file and confirming the claim holds. A simple loop you can run on any PR:

1. Split the screen. Guide on the left, the real file it tells you to edit on the right. Half the bugs (items 1 and 8) are just "snippet says X, the file two inches away says Y".
2. Never trust a name, jump to it. Ctrl+Click / F12 (Go to Definition) on every method, and Shift+F12 (Find References) to see how things are really called. This alone catches item 4 (getGitHubClient lives on AuthService, not StorageService) and item 1 (the real makeScanRouter convention).
3. Global search the strings. Ctrl+Shift+F for /api/auth vs /auth/github exposes item 2; searching PROVIDERS exposes item 6.
4. Check line 1 of each file for its module style. export const = ESM, require/module.exports = CommonJS. Mixing them throws (item 6).
5. Follow one action end to end. Trace "user clicks Sign in with GitHub" through button → apiClient → route → callback → next request. Issues that only show up across the whole chain (item 3 auth model, item 5 the user shrinking to { id }) surface here.
6. Confirm library calls against the official docs. repos.getForAuthenticatedUser isn't real Octokit (item 7). The skill is suspicion: if you've never seen a method, verify it exists instead of assuming.
7. Read the doc against itself. "No frontend changes required" sitting above a frontend phase is a contradiction you catch without touching the code.

The underlying rule: treat every method name, file path, route string, and import as a claim to be confirmed, not trusted.

Bottom line

The thinking is all here. It just needs the snippets lined up with how the codebase is actually wired. My main ask is splitting design from implementation so the good parts can land now. Happy to pair on any of the above.