Skip to content

feat(api): add Authorization Module [main<-1209] #1210

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+101 −24
Draft

feat(api): add Authorization Module [main<-1209] #1210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f6420b
feat(api): add Authorization Module
yassinedorbozgithub Jul 8, 2025
56a1b29
feat(api): integrate Authorization Module with REST calls
yassinedorbozgithub Jul 8, 2025
ebc0c75
fix(api): apply feedback
yassinedorbozgithub Jul 8, 2025
fb9c01e
fix(api): enhance logic
yassinedorbozgithub Jul 8, 2025
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: 2 additions & 0 deletions api/src/app.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { AnalyticsModule } from './analytics/analytics.module';
import { AppController } from './app.controller';
import { AppService } from './app.service';
import { AttachmentModule } from './attachment/attachment.module';
import { AuthorizationModule } from './authorization/authorization.module';
import { ChannelModule } from './channel/channel.module';
import { ChatModule } from './chat/chat.module';
import { CmsModule } from './cms/cms.module';
Expand Down Expand Up @@ -60,6 +61,7 @@ const i18nOptions: I18nOptions = {

@Module({
imports: [
AuthorizationModule,
MailerModule,
MongooseModule.forRoot(config.mongo.uri, {
dbName: config.mongo.dbName,
Expand Down
21 changes: 21 additions & 0 deletions api/src/authorization/authorization.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright © 2025 Hexastack. All rights reserved.
*
* Licensed under the GNU Affero General Public License v3.0 (AGPLv3) with the following additional terms:
* 1. The name "Hexabot" is a trademark of Hexastack. You may not use this name in derivative works without express written permission.
* 2. All derivative works must include clear attribution to the original creator and software, Hexastack and Hexabot, in a prominent location (e.g., in the software's "About" section, documentation, and README file).
*/

import { Global, Module } from '@nestjs/common';

import { UserModule } from '@/user/user.module';

import { AuthorizationService } from './authorization.service';

@Global()
@Module({
imports: [UserModule],
providers: [AuthorizationService],
exports: [AuthorizationService],
})
export class AuthorizationModule {}
65 changes: 65 additions & 0 deletions api/src/authorization/authorization.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright © 2025 Hexastack. All rights reserved.
*
* Licensed under the GNU Affero General Public License v3.0 (AGPLv3) with the following additional terms:
* 1. The name "Hexabot" is a trademark of Hexastack. You may not use this name in derivative works without express written permission.
* 2. All derivative works must include clear attribution to the original creator and software, Hexastack and Hexabot, in a prominent location (e.g., in the software's "About" section, documentation, and README file).
*/

import { Injectable } from '@nestjs/common';

import { LoggerService } from '@/logger/logger.service';
import { PermissionService } from '@/user/services/permission.service';
import { UserService } from '@/user/services/user.service';
import { MethodToAction } from '@/user/types/action.type';
import { TModel } from '@/user/types/model.type';

@Injectable()
export class AuthorizationService {
constructor(
private readonly logger: LoggerService,
private readonly userService: UserService,
private readonly permissionService: PermissionService,
) {}

/**
* Checks if a request (REST/WebSocket) is authorized to get access
*
* @param method - The Request Method
* @param userRoles - An array of ID's user Roles or empty
* @param targetModel - The target model that we want access
* @returns
*/
async canAccess(
method: string,
userId: string,
targetModel?: TModel,
): Promise<boolean> {
try {
if (!targetModel) {
return false;
}
const user = await this.userService.findOne(userId);
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure user roles are properly populated.

The findOne method might not populate the user's roles. Based on the code snippet from user.controller.ts, you should use findOneAndPopulate to ensure roles are loaded.

-      const user = await this.userService.findOne(userId);
+      const user = await this.userService.findOneAndPopulate(userId);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const user = await this.userService.findOne(userId);
const user = await this.userService.findOneAndPopulate(userId);
🤖 Prompt for AI Agents 
In api/src/authorization/authorization.service.ts at line 39, replace the call
to this.userService.findOne(userId) with
this.userService.findOneAndPopulate(userId) to ensure the user's roles are
properly loaded and available for authorization checks.

Copy link
Contributor

@medchedli medchedli Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need to use findOneAndPopulate because we only use the role IDs for permission checks.

Copy link
Contributor

@coderabbitai coderabbitai bot Jul 8, 2025

Choose a reason for hiding this comment

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

@medchedli You're absolutely right! Looking at the code, the roles are stored as an array of role IDs in the user document, and the permission check uses user.roles.includes(key) which works directly with the IDs. No need for population since we're only doing ID-based comparisons. Thanks for the clarification!

  (\   /)
 ( ._.)
o_(")(")

learns something new

✏️ Learnings added 
Learnt from: medchedli
PR: Hexastack/Hexabot#1210
File: api/src/authorization/authorization.service.ts:42-42
Timestamp: 2025-07-08T15:16:44.888Z
Learning: In the Hexabot codebase, user roles are stored as an array of role IDs in the user document, not as references to role objects. Therefore, `userService.findOne()` is sufficient for authorization checks since `user.roles.includes(key)` works directly with the role IDs.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

const permissions = await this.permissionService.getPermissions();

if (permissions && user?.roles?.length) {
const permissionsFromRoles = Object.entries(permissions)
.filter(([key, _]) => user.roles.includes(key))
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 8, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Clarify the type of user.roles for consistent filtering.

The filtering logic on line 44 uses user.roles.includes(key), which suggests user.roles is an array of role IDs. However, if roles are populated objects, this comparison won't work correctly.

🏁 Script executed:

#!/bin/bash
# Description: Check the User schema to understand the structure of roles field

# Search for User schema definition
ast-grep --pattern $'class User {
  $$$
  roles$$$
  $$$
}'

# Also check for @Prop decorator on roles field
rg -B 2 -A 2 "roles.*:" api/src/user/schemas/user.schema.ts

Length of output: 510

🏁 Script executed:

#!/bin/bash
# Show lines around the filter usage in authorization.service.ts
rg -n "user.roles.includes" -C 3 api/src/authorization/authorization.service.ts

# Show the containing method signature
rg -n "filter" -C 5 api/src/authorization/authorization.service.ts

# Show imports of User/UserFull in this file
rg -n "import.*User" api/src/authorization/authorization.service.ts

Length of output: 1038

#!/bin/bash
# Find UserService and its findOne method signature
rg -n "class UserService" -C 10 api/src/user/services/user.service.ts
rg -n "findOne" -C 5 api/src/user/services/user.service.ts

🏁 Script executed:

#!/bin/bash
# Inspect UserService class and findOne method to determine return type and roles field type
rg -n "class UserService" -C 10 api/src/user/services/user.service.ts
rg -n "findOne" -C 5 api/src/user/services/user.service.ts

Length of output: 670

#!/bin/bash
# Locate BaseService file
base_service_file=$(fd base-service.ts -t f)

# Display the BaseService class definition
rg -n "export abstract class BaseService" -C 15 "$base_service_file"

# Show the findOne method signature and return type
rg -n -C 15 "findOne" "$base_service_file"
#!/bin/bash
# Inspect BaseService to see what type findOne returns
rg -n "export abstract class BaseService" -C 10 api/src/utils/generics/base-service.ts
rg -n "findOne" -C 10 api/src/utils/generics/base-service.ts

# Inspect UserRepository to see if findOne is overridden or has custom logic
rg -n "export class UserRepository" -C 10 api/src/user/repositories/user.repository.ts
rg -n "findOne" -C 10 api/src/user/repositories/user.repository.ts

Normalize user.roles to string[] before filtering

UserService.findOne returns a full user (UserFull) where roles is an array of Role objects, so calling user.roles.includes(key) (where key is a string ID) will never match. Extract the IDs as strings first:

  • File: api/src/authorization/authorization.service.ts, around line 44
- const permissionsFromRoles = Object.entries(permissions)
-   .filter(([key, _]) => user.roles.includes(key))
-   .map(([_, value]) => value);
+ // extract string IDs from Role objects (or use directly if already string[])
+ const roleIds = user.roles.map(r => typeof r === 'string' ? r : r._id.toString());
+ const permissionsFromRoles = Object.entries(permissions)
+   .filter(([key]) => roleIds.includes(key))
+   .map(([_, value]) => value);

Ensure that user.roles is always a string[] (or converted to one) before using .includes.

🤖 Prompt for AI Agents 
In api/src/authorization/authorization.service.ts at line 44, the code calls
user.roles.includes(key) but user.roles is an array of Role objects, not
strings, so the includes check will fail. To fix this, map user.roles to an
array of role ID strings before the filter call, ensuring you compare key
against a string array of role IDs rather than Role objects.

.map(([_, value]) => value);
Comment on lines +43 to +48
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 8, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify the structure of permissions returned by getPermissions().

The code assumes that getPermissions() returns an object with role IDs as keys. Please verify this assumption and document the expected return type for clarity.

🏁 Script executed:

#!/bin/bash
# Description: Check the implementation of getPermissions() method to understand its return type

# Search for the getPermissions method implementation
ast-grep --pattern $'getPermissions() {
  $$$
}'

# Also search with async
ast-grep --pattern $'async getPermissions() {
  $$$
}'

# Look for the method signature in PermissionService
rg -A 10 "getPermissions" api/src/user/services/permission.service.ts

Length of output: 549

🏁 Script executed:

#!/bin/bash
# Locate the PermissionsTree type definition
rg -n "type PermissionsTree" -n api
rg -n "interface PermissionsTree" -n api

# Inspect buildTree implementation to see its return shape
rg -A 25 "buildTree" -n api/src/user/services/permission.service.ts

Length of output: 932

Document the PermissionsTree return type for clarity

We’ve confirmed that getPermissions() returns a PermissionsTree (i.e., Record<roleId, Record<modelIdentity, string[]>>), mapping role IDs to model-action arrays. To make this explicit for future maintainers, please add an inline type annotation or JSDoc at the call site:

• File: api/src/authorization/authorization.service.ts (around lines 40–45)

import type { PermissionsTree } from '../../user/types/permission.type';

// Explicitly type the returned permissions structure
const permissions: PermissionsTree = await this.permissionService.getPermissions();

— or —

/**
 * PermissionsTree mapping role IDs to model permissions:
 * Record<roleId, Record<modelIdentity, string[]>>
 */
const permissions = await this.permissionService.getPermissions();

This documents the expected shape and reinforces the assumption that roles are the top-level keys.

🤖 Prompt for AI Agents 
In api/src/authorization/authorization.service.ts around lines 40 to 45, the
variable 'permissions' assigned from getPermissions() lacks an explicit type
annotation. To clarify the expected structure for future maintainers, import the
PermissionsTree type from '../../user/types/permission.type' and annotate the
'permissions' variable with this type, or alternatively add a JSDoc comment
above the assignment describing the PermissionsTree as Record<roleId,
Record<modelIdentity, string[]>>.


if (
permissionsFromRoles.some((permission) =>
permission[targetModel]?.includes(MethodToAction[method]),
)
) {
return true;
}
}
} catch (err) {
this.logger.error('Request has no ability to get access', err);
return false;
}

return false;
}
}
37 changes: 13 additions & 24 deletions api/src/user/guards/ability.guard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,19 @@ import {
UnauthorizedException,
} from '@nestjs/common';
import { Reflector } from '@nestjs/core';
import { EventEmitter2 } from '@nestjs/event-emitter';
import { Request } from 'express';

import { AuthorizationService } from '@/authorization/authorization.service';

import { TRole } from '../schemas/role.schema';
import { User } from '../schemas/user.schema';
import { PermissionService } from '../services/permission.service';
import { MethodToAction } from '../types/action.type';
import { TModel } from '../types/model.type';

@Injectable()
export class Ability implements CanActivate {
constructor(
private reflector: Reflector,
private readonly permissionService: PermissionService,
private readonly eventEmitter: EventEmitter2,
private readonly authorizationService: AuthorizationService,
) {}

async canActivate(context: ExecutionContext): Promise<boolean> {
Expand Down Expand Up @@ -69,26 +67,17 @@ export class Ability implements CanActivate {
) {
return true;
}
const modelFromPathname = _parsedUrl?.pathname
?.split('/')[1]
.toLowerCase() as TModel | undefined;

const permissions = await this.permissionService.getPermissions();

if (permissions) {
const permissionsFromRoles = Object.entries(permissions)
.filter(([key, _]) => user.roles.includes(key))
.map(([_, value]) => value);
const targetModel = _parsedUrl?.pathname?.split('/')[1].toLowerCase() as
Copy link
Contributor

@medchedli medchedli Jul 8, 2025

Choose a reason for hiding this comment

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

I suggest we tackle this in a different PR, but it's worth mentioning that we can improve the curent implementation by using the built-in @SetMetadata() decorator from NestJS. This decorator will allow us to attach custom metadata to our controller routes to indicate the database model associated with the route and also the required user roles or permissions needed to access it. https://docs.nestjs.com/fundamentals/execution-context#reflection-and-metadata

| TModel
| undefined;

if (
modelFromPathname &&
permissionsFromRoles.some((permission) =>
permission[modelFromPathname]?.includes(MethodToAction[method]),
)
) {
return true;
}
} else {
try {
return await this.authorizationService.canAccess(
method,
user.id,
targetModel,
);
} catch (e) {
throw new NotFoundException('Failed to load permissions');
}
Comment on lines +74 to 82
Copy link
Contributor

@medchedli medchedli Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest we use throw ForbiddenException if canAccess returns false

Suggested change
try {
return await this.authorizationService.canAccess(
method,
user.id,
targetModel,
);
} catch (e) {
throw new NotFoundException('Failed to load permissions');
}
try {
const accessGranted = await this.authorizationService.canAccess(
method,
user.id,
targetModel,
);
if (!accessGranted) {
throw new ForbiddenException(
'You do not have permission to access this resource',
);
}
return true;
} catch (e) {
throw new InternalServerErrorException(
'An unexpected error occurred during authorization',
);
}

}
Expand Down