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

feat: Adds route-level role filtering. #3734

Merged
merged 2 commits into from Jul 5, 2022
Merged

Conversation

tommoor
Copy link
Member

@tommoor tommoor commented Jul 5, 2022

Another layer in the onion of security and performance.

documents.fetchDrafts();
documents.fetchTemplates();
}, [documents]);
if (!user.isViewer) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Viewers do not need to load this data

@@ -71,14 +77,24 @@ export default function AuthenticatedRoutes() {
}
>
<Switch>
{can.createDocument && (
Copy link
Member Author

Choose a reason for hiding this comment

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

Viewers don't need these routes mounted

// not awaiting the promise here so that the request is not blocked
user.updateActiveAt(ctx.request.ip);
user.updateActiveAt(ctx.request.ip).catch((err) => {
Copy link
Member Author

@tommoor tommoor Jul 5, 2022

Choose a reason for hiding this comment

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

I hanve't seen any issues here, but good practice to catch all non-awaited promises

return ctx.redirect(
`${team!.url}/auth${ctx.request.path}?${ctx.request.querystring}`
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing ! where possible

return ctx.redirect(
`${team!.url}/auth${ctx.request.path}?${ctx.request.querystring}`
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing ! where possible

@@ -13,7 +13,7 @@ import pagination from "./middlewares/pagination";

const router = new Router();

router.post("fileOperations.info", auth(), async (ctx) => {
router.post("fileOperations.info", auth({ admin: true }), async (ctx) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: These routes are already authorized via policies, but this is another "layer" of security by restricting the role so early on.

@@ -1432,12 +1433,76 @@ describe("#documents.archived", () => {
expect(body.data.length).toEqual(0);
});

it("should require member", async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes a bug where viewers could load archived docs via API but not see them in the UI

expect(body.data.length).toEqual(0);
});

it("should require member", async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes a bug where viewers could load trashed docs via API but not see them in the UI

@tommoor tommoor marked this pull request as ready for review July 5, 2022 07:18
@tommoor tommoor merged commit 831df67 into main Jul 5, 2022
@delete-merged-branch delete-merged-branch bot deleted the tom/feat-route-role-filtering branch July 5, 2022 19:26
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.

None yet

1 participant