-
-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/tos #777
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jona159
wants to merge
39
commits into
dev
Choose a base branch
from
feat/tos
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Feat/tos #777
Changes from all commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
b9df12b
feat: tos schemas
jona159 922694a
feat: translations
jona159 0e7647e
feat: add tos to user schema
jona159 029aa43
feat: enable middleware
jona159 a4e472f
feat: tos server
jona159 c8b2822
feat: add tos acceptance to user creation
jona159 0246ff1
feat: add terms to menu
jona159 6ff6b88
feat: dialog component for tos modal
jona159 3ea6b96
feat: require tos acceptance on register
jona159 6a0a883
feat: ui and api middlewares
jona159 4c90204
feat: adjust register via api for tos requirement
jona159 698fbbc
feat: tos validation
jona159 0bbbe97
feat: terms component
jona159 6f57d7c
feat: allow to accept tos via api
jona159 14144b9
fix: typo, add device deletion page
jona159 34c49f3
feat: delete device component
jona159 6fea65a
feat: translations
jona159 2f6403f
feat: translations
jona159 53e2da8
feat: get user device
jona159 49fd12b
fix: rm device deletion from general edit
jona159 82327d9
fix: style
jona159 5af63d9
feat: allow profile and delete device in ui
jona159 61f509f
feat: add delete action
jona159 a3ed19a
fix: tests
jona159 4d21408
fix: tests
jona159 d9fef16
Revert "fix: rm device deletion from general edit"
jona159 e3a153c
fix: rename to effective from
jona159 5ff6d7e
fix: rm delete from menu
jona159 77eedc2
fix: rm delete from sidebar
jona159 3abf816
feat: delete devices of users who delete their account
jona159 ce674cb
feat: improve api middleware allowlist
jona159 a30a5c8
fix: more tests
jona159 510f27d
fix: remove device deletion from allowlist
jona159 8356bfd
fix: remove device deletion from allowlist
jona159 9ec9572
Merge branch 'dev' into feat/tos
jona159 fc3a5c3
fix: migrations
jona159 a7e96ae
fix: minor
jona159 0869dea
fix: migrations
jona159 24a1b4c
feat: adjust tos schema
jona159 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import { getUserFromJwt } from "~/lib/jwt"; | ||
| import { getTosRequirementForUser } from "~/models/tos.server"; | ||
|
|
||
| function json(body: unknown, status = 200) { | ||
| return new Response(JSON.stringify(body), { | ||
| status, | ||
| headers: { "content-type": "application/json; charset=utf-8" }, | ||
| }); | ||
| } | ||
|
|
||
| type HttpMethod = 'GET' | 'POST' | 'PUT' | 'DELETE' | 'PATCH' | ||
| type AllowRule = { | ||
| method: HttpMethod | '*' | ||
| pathname: string | RegExp | ||
| } | ||
|
|
||
| const API_TOS_ALLOWLIST: AllowRule[] = [ | ||
| { method: 'POST', pathname: '/api/users/refresh-auth' }, | ||
| { method: 'POST', pathname: '/api/users/sign-out' }, | ||
|
|
||
| { method: 'DELETE', pathname: '/api/users/me' }, | ||
|
|
||
| { method: 'POST', pathname: '/api/users/me/accept-tos' }, | ||
| ] | ||
|
|
||
| function isAllowedApi(request: Request, pathname: string) { | ||
| const method = request.method as HttpMethod | ||
|
|
||
| return API_TOS_ALLOWLIST.some((rule) => { | ||
| if (rule.method !== '*' && rule.method !== method) return false | ||
|
|
||
| if (rule.pathname instanceof RegExp) { | ||
| return rule.pathname.test(pathname) | ||
| } | ||
|
|
||
| return rule.pathname === pathname | ||
| }) | ||
| } | ||
|
|
||
| export async function tosApiMiddleware( | ||
| { request }: { request: Request }, | ||
| next: () => Promise<Response>, | ||
| ) { | ||
| const url = new URL(request.url); | ||
|
|
||
| const jwtUser = await getUserFromJwt(request); | ||
| if (typeof jwtUser !== "object") { | ||
| return next(); | ||
| } | ||
|
|
||
| if (isAllowedApi(request, url.pathname)) { | ||
| return next(); | ||
| } | ||
|
|
||
| const req = await getTosRequirementForUser(jwtUser.id); | ||
| if (req.required && req.tos) { | ||
| return json( | ||
| { | ||
| code: "tos_required", | ||
| tosVersionId: req.tos.id, | ||
| effectiveFrom: req.tos.effectiveFrom, | ||
| }, | ||
| 428, | ||
| ); | ||
| } | ||
|
|
||
| return next(); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import { redirect } from "react-router"; | ||
| import { getTosRequirementForUser } from "~/models/tos.server"; | ||
| import { getUserId } from "~/utils/session.server"; | ||
|
|
||
| function isAllowedUiPath(pathname: string) { | ||
| if (pathname.startsWith("/explore")) return true; | ||
| if (pathname === "/terms") return true; | ||
| if (pathname === "/settings/delete") return true; | ||
| if (pathname === "/logout") return true; | ||
| if (pathname === '/tos-required') return true; | ||
| if (pathname.startsWith("/profile")) return true; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export async function tosUiMiddleware( | ||
| { request }: { request: Request }, | ||
| next: () => Promise<Response>, | ||
| ) { | ||
| const url = new URL(request.url); | ||
|
|
||
| if (url.pathname.startsWith("/api")) { | ||
| // handled by tos-api middleware | ||
| return next(); | ||
| } | ||
|
|
||
| if (isAllowedUiPath(url.pathname)) { | ||
| return next(); | ||
| } | ||
|
|
||
| const userId = await getUserId(request); | ||
| if (!userId) return next(); | ||
|
|
||
| const req = await getTosRequirementForUser(userId); | ||
| if (req.required && req.tos) { | ||
| const redirectTo = url.pathname + url.search; | ||
| throw redirect(`/tos-required?redirectTo=${encodeURIComponent(redirectTo)}`) | ||
| } | ||
|
|
||
| return next(); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import { drizzleClient } from '~/db.server' | ||
| import { tosUserState } from '~/schema/tos' | ||
|
|
||
| export const ONE_DAY_MS = 24 * 60 * 60 * 1000 | ||
| export const TOS_GRACE_DAYS = 7 | ||
|
|
||
| export async function getCurrentEffectiveTos(now = new Date()) { | ||
| return drizzleClient.query.tosVersion.findFirst({ | ||
| where: (t, { lte }) => lte(t.effectiveFrom, now), | ||
| orderBy: (t, { desc }) => [desc(t.effectiveFrom)], | ||
| }) | ||
| } | ||
|
|
||
| async function getUserTosState(userId: string, tosVersionId: string) { | ||
| return drizzleClient.query.tosUserState.findFirst({ | ||
| where: (s, { and, eq }) => and(eq(s.userId, userId), eq(s.tosVersionId, tosVersionId)), | ||
| }) | ||
| } | ||
|
|
||
| async function ensureUserTosState({ | ||
| userId, | ||
| tos, | ||
| now = new Date(), | ||
| }: { | ||
| userId: string | ||
| tos: { id: string; graceDays?: number | null } | ||
| now?: Date | ||
| }) { | ||
| const existing = await getUserTosState(userId, tos.id) | ||
| if (existing) return existing | ||
|
|
||
| const graceDays = tos.graceDays ?? TOS_GRACE_DAYS | ||
| const graceUntil = new Date(now.getTime() + graceDays * ONE_DAY_MS) | ||
|
|
||
| const inserted = await drizzleClient | ||
| .insert(tosUserState) | ||
| .values({ | ||
| userId, | ||
| tosVersionId: tos.id, | ||
| firstSeenAt: now, | ||
| graceUntil, | ||
| }) | ||
| .onConflictDoNothing() | ||
| .returning() | ||
|
|
||
| return inserted[0] ?? (await getUserTosState(userId, tos.id)) | ||
| } | ||
|
|
||
| export async function markTosAccepted({ | ||
| userId, | ||
| tosId, | ||
| now = new Date(), | ||
| graceDays = TOS_GRACE_DAYS, | ||
| }: { | ||
| userId: string | ||
| tosId: string | ||
| now?: Date | ||
| graceDays?: number | ||
| }) { | ||
| const graceUntil = new Date(now.getTime() + graceDays * ONE_DAY_MS) | ||
|
|
||
| await drizzleClient | ||
| .insert(tosUserState) | ||
| .values({ | ||
| userId, | ||
| tosVersionId: tosId, | ||
| firstSeenAt: now, | ||
| graceUntil, | ||
| acceptedAt: now, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: [tosUserState.userId, tosUserState.tosVersionId], | ||
| set: { acceptedAt: now }, | ||
| }) | ||
| } | ||
|
|
||
| export async function getTosRequirementForUser(userId: string, now = new Date()) { | ||
| const current = await getCurrentEffectiveTos(now) | ||
| if (!current) { | ||
| return { tos: null, accepted: true, inGrace: false, mustBlock: false, graceUntil: null } | ||
| } | ||
|
|
||
| const state = await ensureUserTosState({ | ||
| userId, | ||
| tos: { id: current.id, graceDays: (current as any).graceDays }, | ||
| now, | ||
| }) | ||
|
|
||
| const accepted = !!state?.acceptedAt | ||
| const graceUntil = state?.graceUntil ?? null | ||
| const inGrace = !accepted && !!graceUntil && now <= new Date(graceUntil) | ||
|
|
||
| const mustBlock = !accepted && !inGrace | ||
|
|
||
| return { tos: current, accepted, inGrace, mustBlock, graceUntil } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we couldn't handle both ui and server in one middleware?
They seem very related to each other and technically we are always handling just "requests", right?
I do understand the response types are different, but this way there also seems to be quite a bit of duplication?
Maybe I am just too picky though :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes i thought about it as well, i initially thought that maybe in a single file it could get a bit messy with potentially more branches over time, but thinking about it again i guess this would still be quite manageable. In terms of code duplication i think the way it is at the moment is still somewhat justifiable, but i think i could still add a small helper to further reduce it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also thinking a bit about "mental load" here. People have to remember to put the right middleware for the right kind of route. It's not like its hugely complicated though, so keeping it separate is probably perfectly fine.
Having more time comparing the two, I am realizing where the differences are. It does make it more complicated to unify the two, so overall we would be trading that for complexity of the code.
I'd rather create a base class/ function for each type of route to make sure the right middleware is called automatically and people don't have to bear the mental load of differentiating ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The right middleware should already be called automatically due to where they are attached (ui middlware in
root.tsxand api inroutes.api). Is this what you meant?