Reworked page authorization to use locals/hooks #24
Conversation
There was a problem hiding this comment.
I'm sorry for not getting to this earlier. I'm struggling to find a prettier way to say this, so I'm going to be a bit harsh:
The goal of this pull request seems a little pointless. perms is an abstract object in that it lacks meaning unless it's properties are verified. If perms.edit is true or if perms.edit is false, it ultimately makes no difference to the behavior of the program unless the state of perms is checked.
The issue this pull request attempts to solve is that the SvelteKit router can skip some code in favor of speed. If the router skips code that handles whether a user is able to access a page, and the state of their permission has changed since the last time it ran, the router continues to use the old state.
The changes you made now determine a users access based on an external variable perms which is updated for every request, rather than pulled straight from the database. That's a good change and it increases readability, but since perms itself is abstract and does not determine the user's access to the site, this change is, as I said it earlier, pointless. That being said, you already wrote it, and it's a step in the right direction so I think after you fix a few of my comments it's good to push.
For all I know the sveltekit router is basically a magic box that does whatever it wants, so relying on it for something vital like authorization can cause some edge cases. The edge cases it creates aren't a huge deal when 2-3 users are running tournaments. So I'd say there's no point in rushing it before the 'Run a Tournament' milestone.
When the ability to create your own tournaments is made public, we should look into entirely reworking how authorization is done. I have some ideas for how to do that, but we should collaborate more on that when we come to it.
| import EditPageSetting from '$lib/components/tournament-page/edit-page/EditPageSetting.svelte'; | ||
|
|
||
| export let data: PageServerData & LayoutServerData; | ||
| export let data: PageServerData & LayoutServerData & LayoutData; |
There was a problem hiding this comment.
If you use LayoutData, the LayoutServerData type is redundant, as it's fully covered by LayoutData
There was a problem hiding this comment.
Didn't know that, thanks.
|
|
||
| if (!editPerms) { | ||
| export const load: LayoutServerLoad = async ({ locals }) => { | ||
| if (!locals.perms?.edit) { |
There was a problem hiding this comment.
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.
| @@ -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[]) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Clearly I misintepreted the auth in hooks idea as "reducing the re-use of code in routes" rather than removing the auth code completely. I agree removing the auth from the route entirely would be better. I think last time we discussed this neither of us were sure how exactly to implement purely hook based authentication, and |
Note that this is ONLY for page authorization, so no authentication for actions for now. (I've been looking into whether Sveltekit actions can be invoked externally with custom POST requests, but haven't found anything yet. If I do find something, then authentication for actions would be necessary)
I did some testing to see if the permissions could accidentally carry over between pages where the permissions are supposed to change; there were no problems. Also while doing so I did confirm that
+layout.server.sveltefiles do run every time a page is loaded (which includes all parent layouts, from top to bottom, even ifparent()is not called).