Since https://github.com/twentyhq/twenty/pull/12286 we are now capturing
in sentry graphql queries errors in the FE.
We want to exclude InternalServerErrors because they are already
captured in sentry from the BE.
We should capture graphQL exceptions thrown in the FE in Sentry.
All the more so as we have just cleaned back-end errors in sentry,
preventing 4xx errors from being wrongfully sent to sentry.
Those 4xx errors should, except for `Unauthenticated` and `Forbidden`
errors (for now - this list can evolve), trigger a sentry FE error, as
we are not suppose to let users of the product interface trigger queries
that will fail with 4xx errors (for instance a malformed input).
We still miss an efficient way to group those errors together in sentry.
It could be the message but the message may be different for each user
if it contains user-specific data, and we don't always have control on
the message.
This can be done later as we iterate on improving sentry
Fixes https://github.com/twentyhq/twenty/issues/11566 + translates dates
- Date display bug: We had an issue with date (not date time) display
depending on the timezone the user had selected. The date is stored in
the db as yyyy-mm-dd, eg. 2025-05-01 for **May 1st, 2025**. When
returned this date is formatted in a UTC DateTime at midnight, so
2025-05-1 00:00:00. Then when displaying the date we were converting
this date using the timeZone, so 2025-04-30 17:00:00, thus displaying
**April 30th, 2025**. The fix chosen is that we should not take into
account the timezone for date (not date time!) displays as we always
want to show the same date.
- Date translation: dates were not translated, not in their default
display (_May 1st, 2025_) nor in their relative display (_about a month
ago_). The lib we use for date formatting, date-fns, offers a
translation option with pre-built `Locale`s from their lib.
Unfortunately and surprisingly we cannot just use directly string locale
codes (like `fr-FR`), cf [open issue on
date-fns](https://github.com/date-fns/date-fns/issues/3660). A util was
introduced to offset this by dynamically importing the right date-fns
Locale based on the locale code.
Introducing a class of RelationException extending CustomException to
help grouping those exception in sentries by ExceptionCode.
I did not introduce a filter as these are thrown in utils that can be
used in multiple places now or in the future, and filters are to be
added at resolver-level.
We have approvedAccessDomain custom exceptions, but they were never
filtered while some of them reflects 4xx errors which we don't want to
be captured as 5xx errors
Follow-up on https://github.com/twentyhq/twenty/pull/12007
In this PR
- adding a filter on HttpExceptionHandlerService to filter out 4xx
errors from driver handling (as we do for graphQL errors: see
useGraphQLErrorHandler hook - only filteredIssues are sent to`
exceptionHandlerService.captureExceptions()`.)
- grouping together more missing metadata issues
- attempting to use error codes as issues names in sentry to improve UI;
for now it says "Error" all the time
Fixes https://github.com/twentyhq/twenty/issues/11864 and
https://github.com/twentyhq/core-team-issues/issues/908
We should not send `createManyXXX` mutations with FE-forged ids in the
payload if we want to do an upsert, because that 1) prevents records
from being merged 2) triggers optimistic rendering while we can't know
before-hand which records will actually be created and which records
will only be updated
Also noticed createdBy was being overriden even for records we are
updating and not creating, which did not seem right, so fixed that too
This PR attemps at improving sentry grouping and filtering by
- Using the exceptionCode as the fingerprint when the error is a
customException. For this to work in this PR we are now throwing
customExceptions instead of internalServerError deprived of their code.
They will still be converted to Internal server errors when sent back as
response
- Filtering 4xx issues where it was missing (for emailVerification
because errors were not handled, for invalid captcha and billing errors
because they are httpErrors and not graphqlErrors)
---------
Co-authored-by: Félix Malfait <felix@twenty.com>
In this PR we are
1. cleaning typeORM service by removing connectToDataSource method
2. using workspaceDataSource instead of mainDataSource when possible,
and replacing raw SQL with workspaceRepository methods to use
First and main step of
https://github.com/twentyhq/core-team-issues/issues/747
We are implementing a permission check layer in our custom
WorkspaceEntityManager by overriding all the db-executing methods (this
PR only overrides some as a POC, the rest will be done in the next PR).
Our custom repositories call entity managers under the hood to interact
with the db so this solves the repositories case too.
This is still behind the feature flag IsPermissionsV2Enabled.
In the next PR
- finish overriding all the methods required in WorkspaceEntityManager
- add tests
Closes https://github.com/twentyhq/core-team-issues/issues/857
The issue was caused by the fact that the preview chip was accidentally
made clickable while not linked to any record id:
<img width="763" alt="Capture d’écran 2025-04-28 à 15 17 32"
src="https://github.com/user-attachments/assets/c1d9bf61-edcb-442f-a914-eccc627ee190"
/>
this was fixed by [this
PR](https://github.com/twentyhq/twenty/pull/11745) (@etiennejouan)
It was causing the side panel to open while the record id was empty,
while this recordId is used in query filters (as it should be to fetch
record data), leading the queries to fail.
Let's early return with an error instead as it does not make sense to
open the record page with an empty recordId.
---------
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
In this PR we are
- introducing a cached map `{ userworkspaceId: roleId } `to reduce calls
to get a userWorkspace's role (we were having N+1 around that with
combinedFindMany queries and generally having a lot of avoidable
queries)
- using the roles permissions cache (`{ roleId: { objectNameSingular:
{ canRead: bool, canUpdate: bool, ...} } `) in Permissions V1's
userHasObjectPermission, in order to 1) improve performances to avoid
calls to get roles 2) start using our permissions cache
In this PR we are
- (if permissionsV2 is enabled) executing permission checks at query
builder level. To do so we want to override the query builders methods
that are performing db calls (.execute(), .getMany(), ... etc.) For now
I have just overriden some of the query builders methods for the poc. To
do so I created custom query builder classes that extend typeorm's query
builder (selectQueryBuilder and updateQueryBuilder, for now and later I
will tackle softDeleteQueryBuilder, etc.).
- adding a notion of roles permissions version and roles permissions
object to datasources. We will now use one datasource per roleId and
rolePermissionVersion. Both rolesPermissionsVersion and rolesPermissions
objects are stored in redis and recomputed at role update or if queried
and found empty. Unlike for metadata version we don't need to store a
version in the db that stands for the source of truth. We also don't
need to destroy and recreate the datasource if the rolesPermissions
version changes, but only to update the value for rolesPermissions and
rolesPermissionsVersions on the existing datasource.
What this PR misses
- computing of roles permissions should take into account
objectPermissions table (for now it only looks at what's on the roles
table)
- pursue extension of query builder classes and overriding of their db
calling-methods
- what should the behaviour be for calls from twentyOrmGlobalManager
that don't have a roleId?
Part of https://github.com/twentyhq/core-team-issues/issues/526
An active workspace's defaultRoleId should never be null.
We can't rely on a simple postgres NOT NULL constraint as defaultRoleId
will always be initially null when the workspace is first created since
the roles do not exist at that time.
Since a suspended workspace can be active again, we should maintain that
rule during the workspace suspension. The only moment defaultRoleId can
have a null value is during the onboarding. this pr enforces that
Closes https://github.com/twentyhq/core-team-issues/issues/526
(for reminder:
1. Make defaultRoleId non-nullable for an active workspace
2. Remove permissions V1 feature flag
3. Set member role as default role for new workspaces
About 1.:
An active workspace's defaultRoleId should never be null.
We can't rely on a simple postgres NOT NULL constraint as defaultRoleId
will always be initially null when the workspace is first created since
the roles do not exist at that time.
Let's add a more complex rule to ensure that
About 3.:
In the first phase of our deploy of permissions, we chose to assign
admin role to all existing users, not to break any existing behavior
with the introduction of the feature (= existing users have less rights
than before).
As we deploy permissions to all existing and future workspaces, let's
set the member role as default role for future workspaces.
)
In [a previous PR](https://github.com/twentyhq/twenty/pull/11023) I
fixed the issue where users where re-assigned the default role when
signin up using SSO.
The "fix" actually introduced another error, calling
`assignRoleToUserWorkspace` only when a user is signing up to twenty,
forgetting about the case where an existing twenty user signs up to a
different workspace. They should still be assigned the role in that
case.
To fix this and improve clarity, I moved assignRoleToUserWorkspace to
addUserToWorkspace and renamed addUserToWorkspace to
addUserToWorkspaceIfUserNotInWorkspace since this is at each sign in but
does nothing if user is already in the workspace.
I think ideally we should refactor this part to improve readability and
understandability, maybe with different flows for each case: signIn and
signUp, to twenty or to a workspace
When logging using a SSO method, we call signInUp service in which we
were wrongfully assigning a role to the user even if the user is signing
in and not signin up.
This went unnoticed during our QA as a different sign-in method is
called when logging with the credentials.
---------
Co-authored-by: Weiko <corentin@twenty.com>
Closes https://github.com/twentyhq/core-team-issues/issues/469 and
https://github.com/twentyhq/core-team-issues/issues/500
In this PR
1. stop conditioning permission initialization for a workspace to env
variable value. Instead we want to create and assign permissions and
roles in all new workspaces. For now that will be totally silent.
2. temporarily, the default role is set to the admin role for new
workspaces. it will also be the case for existing workspaces through the
backfill command. Member role is still being created though. (when we
will do the final roll-out we will update this so that future workspaces
have the member role as default role. our goal here is not to break any
current behaviour for users, that today have all have the equivalent of
admin rights).
During sign-up, new users setting new workspaces are required to choose
a billing plan (30-days or 7 days). They are then redirected to
billing.twenty.com to confirm and configure their plan choice.
Then they are quickly redirected to /settings/billing before being
redirected to /create-workspace.
The problem is that even though /create-workspace is not a gated path,
/settings/billing is: it is a path gated by WORKSPACE permission which
has not been granted to the user at this stage. therefore the user is
not redirected to /create-workspace since they do not reach
/settings/billing path but is redirected to the profile page instead.
(To see this feature flag must be removed + billing enabled: you can use
this branch [from this closed
PR](https://github.com/twentyhq/twenty/pull/10570)).
The chosen workaround is to bypass, in the FE, the permission check for
WORKSPACE permission if the workspace's activation status is
PENDING_CREATION. There is no need for any BE change.
A user should not be able to delete their account if they are the last
admin of a workspace.
It means that if a user wants to sign out of twenty, they should delete
their workspace, not their account
In this PR
- updateWorkspaceMemberRole api was changed to stop allowing null as a
valid value for roleId. it is not possible anymore to just unassign a
role from a user. instead it is only possible to assign a different role
to a user, which will unassign them from their previous role. For this
reason in the FE the bins icons next to the workspaceMember on a role
page were removed
- updateWorkspaceMemberRole will throw if a user attempts to update
their own role
- tests tests tests!
- Adding permission gates on workspaceMember to only allow user with
admin permissions OR users attempting to update or delete themself to
perform write operations on workspaceMember object
- Reverting some changes to treat workflow objects as regular metadata
objects (any user can interact with them)
- (fix) Block updates on soft deleted records
Adding permission gates on all workspace-invitations endpoints:
sendInvitation, resendInvitation, deleteWorkspaceInvitation,
findWorkspaceInvitations (the latter being from my understanding only
used to list the invitations to then re-send them or detee them).
+ tests on Api & webhooks permission gates
Following a conversation with @etiennejouan and @martmull, we are adding
a permission gate on billing resolver's checkoutSession, which should
only be accessible to entitled users or at workspace creation (when
there are no roles yet), when the subscription is incomplete
Closes https://github.com/twentyhq/core-team-issues/issues/393
- enforcing object-records permission checks in resolvers for now. we
will move the logic to a lower level asap
- add integration tests that will still be useful when we have moved the
logic
- introduce guest seeded role to test limited permissions on
object-records
This was updated a few weeks ago and went unnoticed since 1) integration
tests were broken + 2) we don't have actionnable updateMany mutations in
the product at the moment
It will fix some tests, at least
all-people-resolvers.integration-spec.ts for instance
After introducing userWorkspaceId into JWTs, we were wrongfully
executing
```
const userWorkspace = await this.userWorkspaceRepository.findOne({
where: {
id: payload.userWorkspaceId,
},
});
```
which would return a random userWorkpace if `payload.userWorkspaceId` is
undefined.
All generated JWTs have had a userWorkspaceId for more than a week now,
but in tests we had not modified the accessToken in use, which did not
have a userWorkspaceId, until [this
pr](https://github.com/twentyhq/twenty/pull/10204)
In this PR
- closing https://github.com/twentyhq/core-team-issues/issues/313
- adding permission gates on workspace settings and security settings
- adding integration tests for each of the protected setting and
security
Adding SettingsPermissionsGuard to execute permission check.
The guard is added directly in resolver, either at resolver level (ex:
roles) or resolver-endpoint level (ex: metadata). this can be challenged
!
In this PR, we are implementing the updateRole endpoint with the
following rules
1. A user can only update a member's role if they have the permission (=
the admin role)
2. Admin role can't be unassigned if there are no other admin in the
workspace
3. (For now) as members can only have one role for now, when they are
assigned a new role, they are first unassigned the other role (if any)
4. (For now) removing a member's admin role = leaving the member with no
role = calling updateRole with a null roleId
In this PR
- introducing roles module to separate roles logic (assign a Role, get a
workspace's roles etc.) from permission logic (check if a user has a
permission)
- Introduces getRoles endpoint to fetch a workspace's roles
- introduces the first permission check: getRoles in only accessible to
users with permission on ROLE setting. Implemented
validatesUserHasWorkspaceSettingPermissionOrThrow
In this PR
- fixing Collapse on view groups views: aggregate bar should be included
in the collapse (@magrinj )
- respect the html table pattern: the aggregate bar is now a <tr>
element included in a <table> (before that, it was a <tr> not included
in anything)
- add a top-border on the aggregate bar
- introduce short labels for the on-cell value display (display "Empty"
instead of "Count empty" to lighten the interface)
- remove the feature flag !
Removing labelIdentifierFieldMetadataId and
imageIdentifierFieldMetadataId that should not be part of
createObjectInput as they don't make sense for an object yet to create
and will break
As a follow-up of https://github.com/twentyhq/twenty/pull/9304, we are
here creating a migration to run at the next release, aiming at adding
the new aggregate operation options (CountEmpty, CountNotEmpty, ...,
PercentEmpty, PercentNotEmpty) to the enums on View and ViewField's
aggregateOperations fields.
---------
Co-authored-by: Lucas Bordeau <bordeau.lucas@gmail.com>
As part of our rollout strategy, let's remove the workspaces gates,
which will trigger the creation of the field metadatas needed for the
aggregate queries features.
In a later release we will remove the feature flag completely, after all
fields have been created for all workspaces
Marie