Skip to content
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

rfc: add single place for defining page authorization checks #908

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tefkah
Copy link
Member

@tefkah tefkah commented Jan 21, 2025

  • feat: add helper for doing auth checks on pages
  • feat: replace a number of auth checks with new helper

Issue(s) Resolved

High-level Explanation of PR

Test Plan

Screenshots (if applicable)

Notes

@tefkah tefkah mentioned this pull request Jan 21, 2025
Target extends Targeted<Capability> = Targeted<Capability>,
>(
page: T,
userId: UsersId,
Copy link
Member

Choose a reason for hiding this comment

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

Since this is always called in a context where we've already retrieved the full user row, it would be nice to make this function accept a User here and then check user.isSuperAdmin before making the capabilities query. I miss my superadmin privileges in communities I'm not a member of 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea!

apiTokens: Capabilities.editCommunity,
docs: Capabilities.editCommunity,
apiDocs: [Capabilities.editCommunity, Capabilities.viewPub],
} as const satisfies PageAuthorizationCheckMap;
Copy link
Member

Choose a reason for hiding this comment

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

I agree, this is much nicer than what we've been doing!

@3mcd
Copy link
Member

3mcd commented Feb 12, 2025

I'm not sure how I feel about this approach. On one hand, it does keep the page checks very concise, but it adds ~200 lines of code to do so. userCanViewPage also hides away the capability (or capabilities) needed to see a page, whereas currently that information is colocated with the page itself, which I personally prefer.

What about something like this?

await pageCheck(
  Capabilities.editCommunity,
  community.id, // infers that this must be a community id from the provided capability
  user.id, // or a user object
)

And just like userCanViewPage, if the check fails, pageCheck would redirect to /c/${communitySlug}/unauthorized

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants