feat: Add new api for generating invite links #23929
Conversation
n8n-assistant
bot
added
core
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
|
E2E Tests: n8n tests passed after 9m 30.8s Run Details
Groups
This message was posted automatically by
currents.dev | Integration Settings
|
BGZStephen
requested review from
a team and
geemanjs
and removed request for
a team
There was a problem hiding this comment.
This a clever solution to that bug. I quite like it!
- Please tag me on the other side of the PR where we validate and check the token 😄...
From a security point of view I have some more questions:
- Should it be possible to revoke an invitation? This feels like something we should be able to support
- Is this safe from a replay attack? i.e. someone reusing the same token multiple times? JWT's are stateless after-all.
If we wanted some added protection (this prevents both above) an option might be to create a unique inviteId, drop that into the db with a state pending and put it on the JWT also.
| id | state | createdAt | lastUpdatedAt |
| <inviteId> | pending | | |
| <inviteId> | complete | | |
| <inviteId> | revoked | | |
When they signup we extract the inviteId from the JWT and check it's state. If we want to revoke the invite we can manage that on the inviteId table. Alternatively, we can delete the inviteId from the table once it's complete/revoked - that probably scales better (and we can extend to emit events if someone wants the extra information in a SIEM)
- Should we maybe pass the
expiresInthrough the body of the controller to let the user configure how long the link is valid for when they create the link. Default it to a sensible 7 days in an input box. Otherwise, I fear in 6 months time we'll get a request to reduce from 90d to something shorter and/or be able to configure it via an env var or something 👀
guillaumejacquart
left a comment
guillaumejacquart
left a comment
There was a problem hiding this comment.
Looks good ! 2 small comments. Also I'm wondering, we will replace the invitation controller route with this one eventually ? sending the invite by email as well (eventually) ?
| 'user:delete', | ||
| 'user:list', | ||
| 'user:resetPassword', | ||
| 'user:generateInviteLink', |
There was a problem hiding this comment.
Should we also add this scope to all custom roles with user:create scope (the current role used for invitation) in a migration file ?
There was a problem hiding this comment.
Let me add this to the list of tickets, will do this in a follow up once the rest of the code is in place
| const inviterId = req.user.id; | ||
| const inviteeId = req.params.id; | ||
|
|
||
| const targetUser = await this.userRepository.findOne({ where: { id: inviteeId } }); |
There was a problem hiding this comment.
Why not do all that in the users service ? It already has urlService dependency, so would only need the new jwtService dependency.
There was a problem hiding this comment.
Makes sense, I'll move this as part of the other tickets in this feature
Currently no, it's not functionality that exists at the moment, but might be nice to add in the future.
Yes, as once the user has used this token and provided a password, the signup flow will no longer be accessible, we currently check if the user has completed the sign up flow to avoid this.
We had preferred not adding or modifying the DB mostly due to the hassle of migrations, in an ideal world we would have a table for invites, but it's not strictly necessary at the moment
I think it's OK to just have a default (at the moment there is no expiry, so this moves us in the right direction) if there is appetite from a customer perspective, we can always add this later |
|
Got released with |
Summary
This PR creates a new POST API endpoint
/rest/users/:id/invite-linkto support generating signed invite links with expirations to provide a tamper proof mechanism for inviting and accepting invitesRelated Linear tickets, Github issues, and Community forum posts
PAY-4390
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)