Skip to content

Commit

Permalink
Merge pull request #135 from cellajs/development
Browse files Browse the repository at this point in the history
fixes
  • Loading branch information
flipvh authored Jun 12, 2024
2 parents 671a6f1 + d2a1d80 commit b349c09
Show file tree
Hide file tree
Showing 23 changed files with 93 additions and 92 deletions.
2 changes: 1 addition & 1 deletion backend/src/lib/docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const docs = (app: CustomHono) => {
description: `
(ATTENTION: PRERELEASE!) This API documentation is split in modules. Each module relates to a module in the backend codebase. Each module should be at least loosely-coupled, but ideally entirely decoupled. The documentation is based upon zod schemas that are converted to openapi specs using hono middleware: zod-openapi.
API design differentiates between three types of resources:
API differentiates between three types of resources:
1) page-related resources are called an 'entity' (ie ORGANIZATION or USER)
2) a subclass are 'contextual entities' (ie ORGANIZATION, not USER)
Expand Down
2 changes: 1 addition & 1 deletion backend/src/middlewares/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
A short introduction into the most important middleware as they are crucial for fast and secure development.

### Guard
Guard is where cella protects and secures endpoints with middleware. There are currently four options. Except for isPublicAccess, the other three can and should be combined and isAuthenticated always needs to be the first one. For example a `middleware` for an endpoint could be: `[isAuthenticated, isAllowedTo('delete', 'organization'), isSystemAdmin]`
Guard is where cella protects and secures endpoints with middleware. There are currently four options. Except for isPublicAccess, the other three can and should be combined and isAuthenticated always needs to be the first one. For example a `middleware` for an endpoint could be: `[isAuthenticated, isAllowedTo('delete', 'ORGANIZATION'), isSystemAdmin]`

* isPublicAccess
* isAuthenticated
Expand Down
30 changes: 16 additions & 14 deletions backend/src/middlewares/guard/is-allowed-to.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import { membershipsTable } from '../../db/schema/memberships';
import { resolveEntity } from '../../lib/entity';
import { errorResponse } from '../../lib/errors';
import permissionManager, { HierarchicalEntity } from '../../lib/permission-manager';
import type { EntityType, Env } from '../../types/common';
import type { Env, ContextEntity } from '../../types/common';
import { logEvent } from '../logger/log-event';

export type PermissionAction = 'create' | 'update' | 'read' | 'write';

/**
* Middleware to protect routes by checking user permissions.
* @param action - The action to be performed (e.g., 'read', 'write').
Expand All @@ -16,39 +18,39 @@ import { logEvent } from '../logger/log-event';
*/
const isAllowedTo =
// biome-ignore lint/suspicious/noExplicitAny: it's required to use `any` here
(action: string, entityType: string): MiddlewareHandler<Env, any> =>
(action: PermissionAction, entityType: ContextEntity): MiddlewareHandler<Env, any> =>
async (ctx: Context, next) => {
// Extract user
const user = ctx.get('user');

// Retrieve the context of the entity to be authorized (e.g., 'organization', 'workspace')
const context = await getEntityContext(ctx, entityType);
const contextEntity = await getEntityContext(ctx, entityType);

// Check if user or context is missing
if (!context || !user) {
return errorResponse(ctx, 404, 'not_found', 'warn', entityType.toUpperCase() as EntityType, {
if (!contextEntity || !user) {
return errorResponse(ctx, 404, 'not_found', 'warn', entityType, {
user: user?.id,
id: context?.id || '',
id: contextEntity?.id || '',
});
}

// Fetch user's memberships from the database
const memberships = await db.select().from(membershipsTable).where(eq(membershipsTable.userId, user.id));

// Check if the user is allowed to perform the action in the given context
const isAllowed = permissionManager.isPermissionAllowed(memberships, action, context);
const isAllowed = permissionManager.isPermissionAllowed(memberships, action, contextEntity);

// If user is not allowed and not an admin, return a forbidden error
// If not allowed and not admin, return forbidden
if (!isAllowed && user.role !== 'ADMIN') {
return errorResponse(ctx, 403, 'forbidden', 'warn', entityType.toUpperCase() as EntityType, { user: user.id, id: context.id });
return errorResponse(ctx, 403, 'forbidden', 'warn', entityType, { user: user.id, id: contextEntity.id });
}

// Store the user memberships and authorized entity context in the context
ctx.set('memberships', memberships);
ctx.set(entityType, context);
// Store user memberships and authorized context entity in hono ctx
ctx.set('userMemberships', memberships);
ctx.set('contextEntity', contextEntity);

// Log user allowance in the context
logEvent(`User is allowed to ${action} ${context.entity}`, { user: user.id, id: context.id });
logEvent(`User is allowed to ${action} ${contextEntity.entity}`, { user: user.id, id: contextEntity.id });

await next();
};
Expand All @@ -57,7 +59,7 @@ const isAllowedTo =
* Get the context based on the entity type.
* Handles resolve for both direct entity operations (retrieval, update, deletion) and contextual operations (fetching child entities).
* @param ctx - The context object containing request and response details.
* @param entityType - The type of the entity (e.g., 'organization', 'workspace').
* @param entityType - The type of the entity (e.g., 'ORGANIZATION', 'WORKSPACE').
*/

// biome-ignore lint/suspicious/noExplicitAny: Prevent assignable errors
Expand Down
28 changes: 14 additions & 14 deletions backend/src/modules/auth/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { checkEmailJsonSchema, signInJsonSchema, signUpJsonSchema } from './sche

export const checkEmailRouteConfig = createRouteConfig({
method: 'post',
path: '/authenticate/check-email',
path: '/check-email',
guard: isPublicAccess,
middleware: [authRateLimiter],
tags: ['auth'],
Expand Down Expand Up @@ -42,7 +42,7 @@ export const checkEmailRouteConfig = createRouteConfig({

export const signUpRouteConfig = createRouteConfig({
method: 'post',
path: '/authenticate/sign-up',
path: '/sign-up',
guard: isPublicAccess,
tags: ['auth'],
summary: 'Sign up with password',
Expand Down Expand Up @@ -76,7 +76,7 @@ export const signUpRouteConfig = createRouteConfig({

export const sendVerificationEmailRouteConfig = createRouteConfig({
method: 'post',
path: '/authenticate/verify-email',
path: '/verify-email',
guard: isPublicAccess,
middleware: [authRateLimiter],
tags: ['auth'],
Expand Down Expand Up @@ -109,7 +109,7 @@ export const sendVerificationEmailRouteConfig = createRouteConfig({

export const verifyEmailRouteConfig = createRouteConfig({
method: 'get',
path: '/authenticate/verify-email/{token}',
path: '/verify-email/{token}',
guard: isPublicAccess,
middleware: [authRateLimiter],
tags: ['auth'],
Expand Down Expand Up @@ -139,7 +139,7 @@ export const verifyEmailRouteConfig = createRouteConfig({

export const resetPasswordRouteConfig = createRouteConfig({
method: 'post',
path: '/authenticate/reset-password',
path: '/reset-password',
guard: isPublicAccess,
middleware: [authRateLimiter],
tags: ['auth'],
Expand Down Expand Up @@ -172,7 +172,7 @@ export const resetPasswordRouteConfig = createRouteConfig({

export const resetPasswordCallbackRouteConfig = createRouteConfig({
method: 'post',
path: '/authenticate/reset-password/{token}',
path: '/reset-password/{token}',
guard: isPublicAccess,
middleware: [authRateLimiter],
tags: ['auth'],
Expand Down Expand Up @@ -204,7 +204,7 @@ export const resetPasswordCallbackRouteConfig = createRouteConfig({

export const signInRouteConfig = createRouteConfig({
method: 'post',
path: '/authenticate/sign-in',
path: '/sign-in',
guard: isPublicAccess,
middleware: [signInRateLimiter()],
tags: ['auth'],
Expand Down Expand Up @@ -242,7 +242,7 @@ export const signInRouteConfig = createRouteConfig({

export const githubSignInRouteConfig = createRouteConfig({
method: 'get',
path: '/authenticate/github',
path: '/github',
guard: isPublicAccess,
tags: ['auth'],
summary: 'Authenticate with GitHub',
Expand All @@ -262,7 +262,7 @@ export const githubSignInRouteConfig = createRouteConfig({

export const githubSignInCallbackRouteConfig = createRouteConfig({
method: 'get',
path: '/authenticate/github/callback',
path: '/github/callback',
guard: isPublicAccess,
middleware: [authRateLimiter],
tags: ['auth'],
Expand All @@ -288,7 +288,7 @@ export const githubSignInCallbackRouteConfig = createRouteConfig({

export const googleSignInRouteConfig = createRouteConfig({
method: 'get',
path: '/authenticate/google',
path: '/google',
guard: isPublicAccess,
tags: ['auth'],
summary: 'Authenticate with Google',
Expand All @@ -308,7 +308,7 @@ export const googleSignInRouteConfig = createRouteConfig({

export const googleSignInCallbackRouteConfig = createRouteConfig({
method: 'get',
path: '/authenticate/google/callback',
path: '/google/callback',
guard: isPublicAccess,
middleware: [authRateLimiter],
tags: ['auth'],
Expand All @@ -334,7 +334,7 @@ export const googleSignInCallbackRouteConfig = createRouteConfig({

export const microsoftSignInRouteConfig = createRouteConfig({
method: 'get',
path: '/authenticate/microsoft',
path: '/microsoft',
guard: isPublicAccess,
tags: ['auth'],
summary: 'Authenticate with Microsoft',
Expand All @@ -358,7 +358,7 @@ export const microsoftSignInRouteConfig = createRouteConfig({

export const microsoftSignInCallbackRouteConfig = createRouteConfig({
method: 'get',
path: '/authenticate/microsoft/callback',
path: '/microsoft/callback',
guard: isPublicAccess,
middleware: [authRateLimiter],
tags: ['auth'],
Expand All @@ -384,7 +384,7 @@ export const microsoftSignInCallbackRouteConfig = createRouteConfig({

export const signOutRouteConfig = createRouteConfig({
method: 'get',
path: '/authenticate/sign-out',
path: '/sign-out',
guard: isPublicAccess,
tags: ['auth'],
summary: 'Sign out',
Expand Down
8 changes: 7 additions & 1 deletion backend/src/modules/general/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -389,11 +389,16 @@ const generalRoutes = app
const { idOrSlug, entityType, q, sort, order, offset, limit, role } = ctx.req.valid('query');
const entity = await resolveEntity(entityType, idOrSlug);

// TODO use filter query helper to avoid code duplication. Also, this specific filter is missing name search?
const filter: SQL | undefined = q ? ilike(usersTable.email, `%${q}%`) : undefined;

const usersQuery = db.select().from(usersTable).where(filter).as('users');

const membersFilters = [eq(membershipsTable.organizationId, entity.id), eq(membershipsTable.type, entityType)];
// TODO refactor this to use agnostic entity mapping to use 'entityType'+Id in a clean way
const membersFilters = [
eq(entityType === 'ORGANIZATION' ? membershipsTable.organizationId : membershipsTable.projectId, entity.id),
eq(membershipsTable.type, entityType),
];

if (role) {
membersFilters.push(eq(membershipsTable.role, role.toUpperCase() as MembershipModel['role']));
Expand All @@ -409,6 +414,7 @@ const generalRoutes = app
.where(and(...membersFilters))
.as('roles');

// TODO: use count helper?
const membershipCount = db
.select({
userId: membershipsTable.userId,
Expand Down
6 changes: 3 additions & 3 deletions backend/src/modules/general/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
} from '../../lib/common-responses';
import { entityTypeSchema } from '../../lib/common-schemas';
import { createRouteConfig } from '../../lib/route-config';
import { isAllowedTo, isAuthenticated, isPublicAccess, isSystemAdmin } from '../../middlewares/guard';
import { isAuthenticated, isPublicAccess, isSystemAdmin } from '../../middlewares/guard';
import { authRateLimiter, rateLimiter } from '../../middlewares/rate-limiter';
import {
acceptInviteJsonSchema,
Expand Down Expand Up @@ -224,7 +224,7 @@ export const suggestionsConfig = createRouteConfig({
path: '/suggestions',
guard: isAuthenticated,
tags: ['general'],
summary: 'Get search suggestions',
summary: 'Get list of suggestions',
description: 'Get search suggestions for all entities, such as users and organizations.',
request: {
query: z.object({
Expand All @@ -248,7 +248,7 @@ export const suggestionsConfig = createRouteConfig({
export const getMembersRouteConfig = createRouteConfig({
method: 'get',
path: '/members',
guard: [isAuthenticated, isAllowedTo('read', 'organization')],
guard: [isAuthenticated],
tags: ['general'],
summary: 'Get list of members',
description: 'Get members of an entity by id or slug. It returns members (users) with their role.',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import type { membershipInfoType } from "../schema";
export const toMembershipInfo = (membership: MembershipModel | undefined | null): membershipInfoType | null => {
return membership ? {
id: membership.id,
createdAt: membership.createdAt.toString(),
role: membership.role,
archived: membership.inactive || false,
} : null;
Expand Down
1 change: 0 additions & 1 deletion backend/src/modules/memberships/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ export const deleteMembersQuerySchema = z.object({
export const membershipInfoSchema = z.object({
id: apiMembershipSchema.shape.id,
role: apiMembershipSchema.shape.role,
createdAt: apiMembershipSchema.shape.createdAt,
archived: apiMembershipSchema.shape.inactive,
}).nullable();

Expand Down
4 changes: 2 additions & 2 deletions backend/src/modules/organizations/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export const getOrganizationsRouteConfig = createRouteConfig({
export const updateOrganizationRouteConfig = createRouteConfig({
method: 'put',
path: '/{idOrSlug}',
guard: [isAuthenticated, isAllowedTo('update', 'organization')],
guard: [isAuthenticated, isAllowedTo('update', 'ORGANIZATION')],
tags: ['organizations'],
summary: 'Update organization',
description: 'Update organization by id or slug.',
Expand Down Expand Up @@ -95,7 +95,7 @@ export const updateOrganizationRouteConfig = createRouteConfig({
export const getOrganizationRouteConfig = createRouteConfig({
method: 'get',
path: '/{idOrSlug}',
guard: [isAuthenticated, isAllowedTo('read', 'organization')],
guard: [isAuthenticated, isAllowedTo('read', 'ORGANIZATION')],
tags: ['organizations'],
summary: 'Get organization',
description: 'Get an organization by id or slug.',
Expand Down
3 changes: 2 additions & 1 deletion backend/src/modules/projects/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ const projectsRoutes = app
* Get list of projects
*/
.openapi(getProjectsRouteConfig, async (ctx) => {
// also be able to filter on organizationId
// TODO: also be able to filter on organizationId
const { q, sort, order, offset, limit, workspaceId, requestedUserId } = ctx.req.valid('query');
const user = ctx.get('user');

Expand All @@ -125,6 +125,7 @@ const projectsRoutes = app
.as('counts');

// @TODO: Permission check which projects a user is allowed to see? (this will skip when requestedUserId is used in query!)
// It should check organization permissions, project permissions and system admin permission
const memberships = db
.select()
.from(membershipsTable)
Expand Down
8 changes: 4 additions & 4 deletions backend/src/modules/projects/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { apiProjectListSchema, apiProjectSchema, createProjectJsonSchema, getPro
export const createProjectRouteConfig = createRouteConfig({
method: 'post',
path: '/',
guard: [isAuthenticated, isAllowedTo('create', 'project')],
guard: [isAuthenticated, isAllowedTo('create', 'PROJECT')],
tags: ['projects'],
summary: 'Create new project',
description: 'Create a new project in an organization. Creator will become admin and can invite other members.',
Expand Down Expand Up @@ -44,7 +44,7 @@ export const createProjectRouteConfig = createRouteConfig({
export const getProjectRouteConfig = createRouteConfig({
method: 'get',
path: '/{idOrSlug}',
guard: [isAuthenticated, isAllowedTo('read', 'project')],
guard: [isAuthenticated, isAllowedTo('read', 'PROJECT')],
tags: ['projects'],
summary: 'Get project',
description: 'Get project by id or slug.',
Expand All @@ -70,7 +70,7 @@ export const getProjectsRouteConfig = createRouteConfig({
guard: [isAuthenticated],
tags: ['projects'],
summary: 'Get list of projects',
description: 'Get list of projects in which you have a membership or by requestedId.',
description: 'Get list of projects in which you have a membership or - if a `requestedUserId` is provided - the projects of this user.',
request: {
query: getProjectsQuerySchema,
},
Expand All @@ -90,7 +90,7 @@ export const getProjectsRouteConfig = createRouteConfig({
export const updateProjectRouteConfig = createRouteConfig({
method: 'put',
path: '/{idOrSlug}',
guard: [isAuthenticated, isAllowedTo('update', 'project')],
guard: [isAuthenticated, isAllowedTo('update', 'PROJECT')],
tags: ['projects'],
summary: 'Update project',
description: 'Update project by id or slug.',
Expand Down
6 changes: 3 additions & 3 deletions backend/src/modules/workspaces/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { apiWorkspaceSchema, createWorkspaceJsonSchema, updateWorkspaceJsonSchem
export const createWorkspaceRouteConfig = createRouteConfig({
method: 'post',
path: '/',
guard: [isAuthenticated, isAllowedTo('create', 'workspace')],
guard: [isAuthenticated, isAllowedTo('create', 'WORKSPACE')],
tags: ['workspaces'],
summary: 'Create new workspace',
description: 'Create personal workspace to organize projects and tasks.',
Expand Down Expand Up @@ -38,7 +38,7 @@ export const createWorkspaceRouteConfig = createRouteConfig({
export const getWorkspaceRouteConfig = createRouteConfig({
method: 'get',
path: '/{idOrSlug}',
guard: [isAuthenticated, isAllowedTo('read', 'workspace')],
guard: [isAuthenticated, isAllowedTo('read', 'WORKSPACE')],
tags: ['workspaces'],
summary: 'Get workspace',
description: 'Get workspace by id or slug.',
Expand All @@ -61,7 +61,7 @@ export const getWorkspaceRouteConfig = createRouteConfig({
export const updateWorkspaceRouteConfig = createRouteConfig({
method: 'put',
path: '/{idOrSlug}',
guard: [isAuthenticated, isAllowedTo('update', 'workspace')],
guard: [isAuthenticated, isAllowedTo('update', 'WORKSPACE')],
tags: ['workspaces'],
summary: 'Update workspace',
description: 'Update workspace by id or slug.',
Expand Down
2 changes: 1 addition & 1 deletion backend/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ app.onError((err, ctx) => {

// Add routes for each module
const routes = app
.route('/', authRoutes)
.route('/auth', authRoutes)
.route('/me', meRoutes)
.route('/users', usersRoutes)
.route('/organizations', organizationsRoutes)
Expand Down
Loading

0 comments on commit b349c09

Please sign in to comment.