Skip to content

Reworked page authorization to use locals/hooks #24

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.

Sign up for GitHub

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
JPW03 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Reworked page authorization to use locals/hooks #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib/components/tournament-page/TournamentTeams.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

export let tournament: db.FullyPopulatedTournament;
export let editPerms: boolean;
export let sessionUserTeam: db.TeamWithMembers | null;
export let sessionUserTeam: db.TeamWithMembers | undefined;
let { team_size, Teams } = tournament;
</script>

Expand Down
25 changes: 8 additions & 17 deletions src/routes/(main)/tournaments/[id]/+layout.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { LayoutServerLoad } from './$types';
import { StatusCodes } from '$lib/StatusCodes';
import prisma from '../../../../lib/prisma';
import prisma from '$lib/prisma';
import { error } from '@sveltejs/kit';

export const prerender = 'auto';
Expand Down Expand Up @@ -139,24 +139,15 @@ export const load: LayoutServerLoad = async ({ params, locals }) => {

const tournament: db.FullyPopulatedTournament = tournamentRaw;

let editPerms = false;
const user = locals.user;

let sessionUserTeam = null;

if (!user) {
return { tournament, editPerms, sessionUserTeam };
}

// Get the session user's team
sessionUserTeam = tournament.Teams?.find((team) =>
team.Members.find((member) => member.osuId === user.id)
);

const hosts = tournament.Hosts.map((x) => x.userId);
if (hosts.includes(user.id)) {
editPerms = true;
// Get the session user's team (only if logged in and playing in tournament)
let sessionUserTeam = undefined;
if (user && locals.perms.playing) {
sessionUserTeam = tournament.Teams?.find((team) =>
team.Members.find((member) => member.osuId === user.id)
);
}

return { tournament, editPerms, sessionUserTeam };
return { tournament, perms: locals.perms, sessionUserTeam };
}
10 changes: 4 additions & 6 deletions src/routes/(main)/tournaments/[id]/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@
import Teams from '$lib/components/tournament-page/TournamentTeams.svelte';
import Matches from '$lib/components/tournament-page/TournamentMatches.svelte';
import Button from '$lib/components/common/LargeButton.svelte';
import type { LayoutServerData } from './$types';

export let data: {
tournament: db.FullyPopulatedTournament;
editPerms: boolean;
sessionUserTeam: db.TeamWithMembers | null;
};
let { tournament, editPerms, sessionUserTeam } = data;
export let data: LayoutServerData;
let { tournament, perms, sessionUserTeam } = data;
let editPerms = perms.edit;
let { name, acronym, id, color, team_size } = tournament;
</script>

Expand Down
26 changes: 5 additions & 21 deletions src/routes/(main)/tournaments/[id]/edit/+page.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ import vine, { errors } from '@vinejs/vine';
import { parseFormData } from 'parse-nested-form-data';
import type { PageServerLoad } from './$types';

export const load: PageServerLoad = async ({ parent }) => {
const { tournament, user } = await parent();
export const load: PageServerLoad = async ({ parent, locals }) => {
const { user, perms } = locals;
const { tournament } = await parent();

if (!user) {
throw error(StatusCodes.UNAUTHORIZED, 'You are not signed in.');
}

const hosts = tournament.Hosts.map((x) => x.userId);
if (!hosts.includes(user.id)) {
if (!perms.edit) {
throw error(StatusCodes.UNAUTHORIZED, 'You do not have permission.');
}

Expand All @@ -23,7 +23,7 @@ export const load: PageServerLoad = async ({ parent }) => {
export const actions: Actions = {
save: async ({ locals, request, params }) => {
const tournamentId = parseInt(params.id ?? '-1');
if (!hasEditPermission(tournamentId, locals.user.id)) throw error(StatusCodes.UNAUTHORIZED);
if (locals.perms.edit) throw error(StatusCodes.UNAUTHORIZED);

const data = parseFormData(await request.formData());

Expand Down Expand Up @@ -69,20 +69,4 @@ export const actions: Actions = {
}
};

const hasEditPermission = async (tournament: number, user: number) => {
// Check edit permissions
const permissionCheck = await prisma.tournament.findFirst({
where: {
id: tournament,
Hosts: {
some: {
userId: user
}
}
}
});

return permissionCheck != null;
};

export const prerender = false;
8 changes: 2 additions & 6 deletions src/routes/(main)/tournaments/[id]/staff-dashboard/+layout.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,8 @@ import { error } from "@sveltejs/kit";
import type { LayoutServerLoad } from "./$types";
import { StatusCodes } from "$lib/StatusCodes";

export const load: LayoutServerLoad = async ({ parent }) => {
const { tournament, editPerms, user } = await parent();

if (!editPerms) {
export const load: LayoutServerLoad = async ({ locals }) => {
if (!locals.perms?.edit) {
Copy link
Collaborator

@clxxiii clxxiii Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of what I was talking about in the initial review comment:
This does not fix the problem of "authorization being in layout files", because the if statement that determines whether the user is able to access the site or not still exists in the layout.

throw error(StatusCodes.UNAUTHORIZED, 'You do not have permission to access this page.');
}

return { tournament, user };
}
2 changes: 1 addition & 1 deletion src/routes/(main)/tournaments/[id]/staff-dashboard/matches/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
let { tournamentName, teams, rounds } = data;
let selectedMatch: db.MatchWithTeams | undefined = undefined;

function onDrop(updatedTeams: db.TeamWithMembers[]) {
function onDrop(updatedTeams: db.TeamWithMembersAndMatches[]) {
Copy link
Collaborator

@clxxiii clxxiii Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, but in the future please isolate any changes like this to direct commits to the main branch, or a "General Improvements" branch with a list of the improvements you made.
Otherwise it's hard to tell when I'm reviewing whether the code you wrote is relevant to the pull request or not.

Copy link
Collaborator Author

@JPW03 JPW03 Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's difficult to tell but this is just a random type safety error fix, and I agree a general improvements branch would be a good idea.

teams = updatedTeams;
}

Expand Down
8 changes: 4 additions & 4 deletions src/routes/(main)/tournaments/[id]/teams/[team_id]/+layout.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { error } from '@sveltejs/kit';
import type { LayoutServerLoad } from './$types';
import { StatusCodes } from '$lib/StatusCodes';

export const load: LayoutServerLoad = async ({ params, parent }) => {
const { tournament, user } = await parent();
export const load: LayoutServerLoad = async ({ params, parent, locals }) => {
const { tournament } = await parent();

// Retrieve team from tournament and params
const team = tournament.Teams.find((team) => team.id === parseInt(params.team_id));
Expand All @@ -17,8 +17,8 @@ export const load: LayoutServerLoad = async ({ params, parent }) => {

// Team captains have a member_order of 0
const isTeamCaptain = team.Members.some(
(member) => member.osuId === user?.id && member.member_order === 0
(member) => member.osuId === locals.user?.id && member.member_order === 0
Copy link
Collaborator

@clxxiii clxxiii Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit actually faces the opposite problem:
If code in +page.server.ts is run after every request (which I'm not confident it is, but for this example it does) and you have an isTeamCaptain check in that file, the variable isTeamCaptain will only be up-to-date when the layout file is run again, which if we're not to trust the router, is not after every request.

Copy link
Collaborator Author

@JPW03 JPW03 Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still can't find explicit documentation saying it runs on every request but all my testing shows that bost +page.server.ts and +layout.server.ts run on every request, even if the contents of the page itself don't update due to sveltekit's smooth page transitions like when use:enhance is used (this is a bug I'm currently dealing with for the mappool creator). The files that don't update on subsequent requests are +page.ts and +layout.ts.

Copy link
Collaborator

@clxxiii clxxiii Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function runs every time the SvelteKit server receives a request — whether that happens while the app is running, or during prerendering — and determines the response.

https://kit.svelte.dev/docs/hooks#server-hooks-handle

);

return { tournament, user, team, isTeamCaptain };
return { team, isTeamCaptain };
};
41 changes: 19 additions & 22 deletions src/routes/(main)/tournaments/[id]/teams/[team_id]/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,17 @@
import InvitePlayer from '$lib/components/tournament-page/team-page/InvitePlayer.svelte';
import TournamentPageTemplate from '$lib/components/tournament-page/TournamentPageTemplate.svelte';
import MatchList from '$lib/components/common/MatchList.svelte';
import type { PageServerData, ActionData, LayoutServerData } from './$types';
import type { PageServerData, ActionData, LayoutServerData, LayoutData } from './$types';
import EditPageSetting from '$lib/components/tournament-page/edit-page/EditPageSetting.svelte';

export let data: PageServerData & LayoutServerData;
export let data: PageServerData & LayoutServerData & LayoutData;
Copy link
Collaborator

@clxxiii clxxiii Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use LayoutData, the LayoutServerData type is redundant, as it's fully covered by LayoutData

Copy link
Collaborator Author

@JPW03 JPW03 Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know that, thanks.

export let form: ActionData;
let { tournament, team, isTeamCaptain } = data;
let { tournament, team, isTeamCaptain, sessionUserTeam } = data;
let { team_size: maxTeamSize } = tournament;
let { name, Members, color, InBracketMatches } = team;

let inTeam: boolean = false;
function updateInTeam(memberId: number) {
if (memberId == data.user?.id) {
inTeam = true;
}
return '';
}
// Check if this team is the current user's team
let inTeam: boolean = (team.id == sessionUserTeam?.id);
</script>

<svelte:head>
Expand All @@ -32,19 +28,18 @@

<section class="team_header">
<h1>
{tournament.team_size == 1 ? 'Player: ' : 'Team: '}
{maxTeamSize == 1 ? 'Player: ' : 'Team: '}
{team.name}
</h1>
</section>

<section class="players">
{#if tournament.team_size != 1}
{#if maxTeamSize != 1}
<h1>Players</h1>
{/if}
<div class="player_cards">
{#each Members as member}
<User user={member.User} {color} />
{updateInTeam(member.User.id)}
{/each}
</div>
{#if inTeam && !isTeamCaptain}
Expand All @@ -62,13 +57,15 @@
<h1>Team Settings</h1>

<form id="team_settings" method="POST" action="?/update_team">
<EditPageSetting
name="name"
label="Team Name"
value={name}
errors={form?.messages}
type="text"
/>
{#if maxTeamSize != 1}
<EditPageSetting
name="name"
label="Team Name"
value={name}
errors={form?.messages}
type="text"
/>
{/if}
<EditPageSetting
name="color"
label="Team Color"
Expand All @@ -92,11 +89,11 @@
{/if}
</section>

{#if tournament.team_size != 1 && tournament.allow_registrations}
{#if maxTeamSize != 1 && tournament.allow_registrations}
<section class="invites">
<h1>Team Invites</h1>

{#if team.Members.length < tournament.team_size}
{#if team.Members.length < maxTeamSize}
<InvitePlayer {data} {form} />
{:else}
<p>Your team is full. You can't invite anymore players.</p>
Expand Down
13 changes: 6 additions & 7 deletions src/routes/(main)/tournaments/[id]/teams/new/+page.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,22 @@ import vine, { errors } from '@vinejs/vine';
import { parseFormData } from 'parse-nested-form-data';
import { StatusCodes } from '$lib/StatusCodes';

export const load: PageServerLoad = async ({ parent }) => {
const { tournament, user, editPerms } = await parent();
export const load: PageServerLoad = async ({ parent, locals }) => {
const { user, perms } = locals;
const { tournament } = await parent();

// If user isn't logged in
if (!user) {
throw error(StatusCodes.UNAUTHORIZED, 'You must log in with osu! to register.');
}

// Check if the user has staff permissions for this tournament
if (editPerms) {
if (perms.edit) {
throw error(StatusCodes.BAD_REQUEST, 'You can\'t sign up for your own tournament.');
}

// Check if this user is already in a team in this tournament
const isInTeam = tournament?.Teams?.find((team) =>
team.Members.some((member) => member.osuId === user?.id)
);
if (isInTeam) {
if (perms.playing) {
throw error(StatusCodes.BAD_REQUEST, 'You are already registered in this tournament.');
}

Expand Down