-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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: Introduce HooksService
#8962
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
7c4ff27
fix: Introduce `HooksService`
RicardoE105 656002d
fix description
RicardoE105 b64f340
add test to make sure downstream service is called.
RicardoE105 d0d8d44
improve function documentation
RicardoE105 dddbe20
Sync master
RicardoE105 d793d48
Add more methods used in the hooks to the service
RicardoE105 d7b201c
Add DB access in BE hooks to hooks service
RicardoE105 87e2e62
Make all properties read only
RicardoE105 88622c1
renames and add authMiddleware method
RicardoE105 e78467e
expose db collections
RicardoE105 e00a64f
add comments to tests
RicardoE105 1e3db1e
fix typing errors
netroy 51c11f7
revert the accidental change
netroy a4bbdba
Use the Auth repo when looking for user
RicardoE105 1d8084d
Merge remote-tracking branch 'origin/master' into ado-1912-bug-users-…
netroy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
import { Service } from 'typedi'; | ||
import type { NextFunction, Response } from 'express'; | ||
import type { QueryDeepPartialEntity } from '@n8n/typeorm/query-builder/QueryPartialEntity'; | ||
import type { FindManyOptions, FindOneOptions, FindOptionsWhere } from '@n8n/typeorm'; | ||
|
||
import { AuthService } from '@/auth/auth.service'; | ||
import type { AuthUser } from '@db/entities/AuthUser'; | ||
import type { User } from '@db/entities/User'; | ||
import { UserRepository } from '@db/repositories/user.repository'; | ||
import { SettingsRepository } from '@db/repositories/settings.repository'; | ||
import { WorkflowRepository } from '@db/repositories/workflow.repository'; | ||
import { CredentialsRepository } from '@db/repositories/credentials.repository'; | ||
import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; | ||
import type { CredentialsEntity } from '@db/entities/CredentialsEntity'; | ||
import { AuthUserRepository } from '@db/repositories/authUser.repository'; | ||
import type { Settings } from '@db/entities/Settings'; | ||
import { UserService } from '@/services/user.service'; | ||
import type { AuthenticatedRequest } from '@/requests'; | ||
import type { Invitation } from '@/Interfaces'; | ||
|
||
/** | ||
* Exposes functionality to be used by the cloud BE hooks. | ||
* DO NOT DELETE or RENAME any of the methods without making sure this is not used in cloud BE hooks. | ||
*/ | ||
@Service() | ||
export class HooksService { | ||
constructor( | ||
private readonly userService: UserService, | ||
private readonly authService: AuthService, | ||
private readonly userRepository: UserRepository, | ||
private readonly settingsRepository: SettingsRepository, | ||
private readonly workflowRepository: WorkflowRepository, | ||
private readonly credentialsRepository: CredentialsRepository, | ||
private readonly authUserRepository: AuthUserRepository, | ||
) {} | ||
|
||
/** | ||
* Invite users to instance during signup | ||
*/ | ||
async inviteUsers(owner: User, attributes: Invitation[]) { | ||
return await this.userService.inviteUsers(owner, attributes); | ||
RicardoE105 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* Set the n8n-auth cookie in the response to auto-login | ||
* the user after instance is provisioned | ||
*/ | ||
issueCookie(res: Response, user: AuthUser) { | ||
return this.authService.issueCookie(res, user); | ||
} | ||
|
||
/** | ||
* Find user in the instance | ||
* 1. To know whether the instance owner is already setup | ||
* 2. To know when to update the user's profile also in cloud | ||
*/ | ||
async findOneUser(filter: FindOneOptions<AuthUser>) { | ||
return await this.authUserRepository.findOne(filter); | ||
} | ||
|
||
/** | ||
* Save instance owner with the cloud signup data | ||
*/ | ||
async saveUser(user: User) { | ||
return await this.userRepository.save(user); | ||
} | ||
|
||
/** | ||
* Update instance's settings | ||
* 1. To keep the state when users are invited to the instance | ||
*/ | ||
async updateSettings(filter: FindOptionsWhere<Settings>, set: QueryDeepPartialEntity<Settings>) { | ||
return await this.settingsRepository.update(filter, set); | ||
} | ||
|
||
/** | ||
* Count the number of workflows | ||
* 1. To enforce the active workflow limits in cloud | ||
*/ | ||
async workflowsCount(filter: FindManyOptions<WorkflowEntity>) { | ||
return await this.workflowRepository.count(filter); | ||
} | ||
|
||
/** | ||
* Count the number of credentials | ||
* 1. To enforce the max credential limits in cloud | ||
*/ | ||
async credentialsCount(filter: FindManyOptions<CredentialsEntity>) { | ||
return await this.credentialsRepository.count(filter); | ||
} | ||
|
||
/** | ||
* Count the number of occurrences of a specific key | ||
* 1. To know when to stop attempting to invite users | ||
*/ | ||
async settingsCount(filter: FindManyOptions<Settings>) { | ||
return await this.settingsRepository.count(filter); | ||
} | ||
|
||
/** | ||
* Add auth middleware to routes injected via the hooks | ||
* 1. To authenticate the /proxy routes in the hooks | ||
*/ | ||
async authMiddleware(req: AuthenticatedRequest, res: Response, next: NextFunction) { | ||
return await this.authService.authMiddleware(req, res, next); | ||
} | ||
|
||
/** | ||
* Return repositories to be used in the hooks | ||
* 1. Some self-hosted users rely in the repositories to interact with the DB directly | ||
*/ | ||
dbCollections() { | ||
return { | ||
User: this.userRepository, | ||
Settings: this.settingsRepository, | ||
Credentials: this.credentialsRepository, | ||
Workflow: this.workflowRepository, | ||
}; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,151 @@ | ||
import type { Response } from 'express'; | ||
import { mock } from 'jest-mock-extended'; | ||
|
||
import type { AuthUser } from '@db/entities/AuthUser'; | ||
import type { CredentialsRepository } from '@db/repositories/credentials.repository'; | ||
import type { SettingsRepository } from '@db/repositories/settings.repository'; | ||
import type { UserRepository } from '@db/repositories/user.repository'; | ||
import type { WorkflowRepository } from '@db/repositories/workflow.repository'; | ||
import type { AuthService } from '@/auth/auth.service'; | ||
import type { UserService } from '@/services/user.service'; | ||
import { HooksService } from '@/services/hooks.service'; | ||
import type { Invitation } from '@/Interfaces'; | ||
import type { AuthenticatedRequest } from '@/requests'; | ||
import type { AuthUserRepository } from '@/databases/repositories/authUser.repository'; | ||
|
||
describe('HooksService', () => { | ||
const mockedUser = mock<AuthUser>(); | ||
const userService = mock<UserService>(); | ||
const authService = mock<AuthService>(); | ||
const userRepository = mock<UserRepository>(); | ||
const settingsRepository = mock<SettingsRepository>(); | ||
const workflowRepository = mock<WorkflowRepository>(); | ||
const credentialsRepository = mock<CredentialsRepository>(); | ||
const authUserRepository = mock<AuthUserRepository>(); | ||
const hooksService = new HooksService( | ||
userService, | ||
authService, | ||
userRepository, | ||
settingsRepository, | ||
workflowRepository, | ||
credentialsRepository, | ||
authUserRepository, | ||
); | ||
|
||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
it('hooksService.inviteUsers should call userService.inviteUsers', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you for adding unit tests here ✅ |
||
// ARRANGE | ||
const usersToInvite: Invitation[] = [{ email: '[email protected]', role: 'global:member' }]; | ||
|
||
// ACT | ||
await hooksService.inviteUsers(mockedUser, usersToInvite); | ||
|
||
// ASSERT | ||
expect(userService.inviteUsers).toHaveBeenCalledWith(mockedUser, usersToInvite); | ||
}); | ||
|
||
it('hooksService.issueCookie should call authService.issueCookie', async () => { | ||
// ARRANGE | ||
const res = mock<Response>(); | ||
|
||
// ACT | ||
hooksService.issueCookie(res, mockedUser); | ||
|
||
// ASSERT | ||
expect(authService.issueCookie).toHaveBeenCalledWith(res, mockedUser); | ||
}); | ||
|
||
it('hooksService.findOneUser should call authUserRepository.findOne', async () => { | ||
// ARRANGE | ||
const filter = { where: { id: '1' } }; | ||
|
||
// ACT | ||
await hooksService.findOneUser(filter); | ||
|
||
// ASSERT | ||
expect(authUserRepository.findOne).toHaveBeenCalledWith(filter); | ||
}); | ||
|
||
it('hooksService.saveUser should call userRepository.save', async () => { | ||
// ACT | ||
await hooksService.saveUser(mockedUser); | ||
|
||
// ASSERT | ||
|
||
expect(userRepository.save).toHaveBeenCalledWith(mockedUser); | ||
}); | ||
|
||
it('hooksService.updateSettings should call settingRepository.update', async () => { | ||
// ARRANGE | ||
const filter = { key: 'test' }; | ||
const set = { value: 'true' }; | ||
|
||
// ACT | ||
await hooksService.updateSettings(filter, set); | ||
|
||
// ASSERT | ||
expect(settingsRepository.update).toHaveBeenCalledWith(filter, set); | ||
}); | ||
|
||
it('hooksService.workflowsCount should call workflowRepository.count', async () => { | ||
// ARRANGE | ||
const filter = { where: { active: true } }; | ||
|
||
// ACT | ||
await hooksService.workflowsCount(filter); | ||
|
||
// ASSERT | ||
expect(workflowRepository.count).toHaveBeenCalledWith(filter); | ||
}); | ||
|
||
it('hooksService.credentialsCount should call credentialRepository.count', async () => { | ||
// ARRANGE | ||
const filter = { where: {} }; | ||
|
||
// ACT | ||
await hooksService.credentialsCount(filter); | ||
|
||
// ASSERT | ||
expect(credentialsRepository.count).toHaveBeenCalledWith(filter); | ||
}); | ||
|
||
it('hooksService.settingsCount should call settingsRepository.count', async () => { | ||
// ARRANGE | ||
const filter = { where: { key: 'test' } }; | ||
|
||
// ACT | ||
await hooksService.settingsCount(filter); | ||
|
||
// ASSERT | ||
expect(settingsRepository.count).toHaveBeenCalledWith(filter); | ||
}); | ||
|
||
it('hooksService.authMiddleware should call authService.authMiddleware', async () => { | ||
// ARRANGE | ||
const res = mock<Response>(); | ||
|
||
const req = mock<AuthenticatedRequest>(); | ||
|
||
const next = jest.fn(); | ||
|
||
// ACT | ||
await hooksService.authMiddleware(req, res, next); | ||
|
||
// ASSERT | ||
expect(authService.authMiddleware).toHaveBeenCalledWith(req, res, next); | ||
}); | ||
|
||
it('hooksService.dbCollections should return valid repositories', async () => { | ||
// ACT | ||
const collections = hooksService.dbCollections(); | ||
|
||
// ASSERT | ||
expect(collections).toHaveProperty('User'); | ||
expect(collections).toHaveProperty('Settings'); | ||
expect(collections).toHaveProperty('Credentials'); | ||
expect(collections).toHaveProperty('Workflow'); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw should we move the dbCollections to this service as well? At the moment they are available in
this.dbCollections
in the hooks. I would still like us to have quite flexible way to query data without needing to add a separate method for every single queryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any changes to the external hooks interface should be considered a breaking change though, since we do have self-hosting customers who use external hooks.
we can definitely add the repositories here, but we should leave dbCollections as they are for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomi, the main idea is to force us to use only methods we have in the hooks service. If the repositories are also available here, we might have a case where if we change the model for that repo, we will break the hooks and not know. However, if we only use the methods we have in the hooks, we will notice here that we broke them and adjusted the method to use the new format/change. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always going to be balance between flexibility and runtime safety. I think it makes sense that certain things (like operations that change things in the DB or touch external systems like send email) have their own method to do that. But we can't think of every single use case beforehand, so thinking that we would do everything only based on the HooksService seems unrealistic. IMHO a good balance could be to have the methods here that we know we need but also expose certain repositories so we also have the flexibility. We can then put everything we know we need here, and rely on the repositories to implement the functionality for older versions that don't yet have the new methods in the HooksService. So I'd expose the repositories here as well. Then it's a mutual agreement that we don't use those unless absolutely necessary and rely on tests (in the hooks side) to make sure they work for different n8n versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as long as we don't remove
dbCollections
fromExternalHooks
, I agree with @tomi.maybe we can mark it as
@deprecated
and remove it in v2.0.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that specifying everything to be in the hooks is unrealistic, if we needed something, we would add it to the hooks service in n8n, wait for it to be release it, and then use it on the BE hooks via the hooks. So, we add functionality as we discover we need it.
For this we still have the
dbCollections
we inject when loading the hooks. When people upgrade to a n8n version which include the hooks service then we use that instead. Once at instances have the hooks service we can stop using thedbCollections
altogether and as I mentioned before start adding functionality to the hooks as we need then.Conclusion:
In any case, we need some way to keep supporting access to these repositories because we have some self-hosting users relaying on this. So will:
dbCollections
here.dbCollections
in we pass as parameter in the external hooks as @deprecated