Skip to content
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

Jump to bottom

Environment variables in admin panel (read only) - backend #9943

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Environment variables in admin panel (read only) - backend #9943

wants to merge 24 commits into from
+1,687 −234

Conversation

ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Jan 30, 2025 •
edited
Loading

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 30, 2025 •
edited
Loading

Warnings
⚠️ Changes were made to the environment variables, but not to the documentation - Please review your changes and check if a change needs to be documented!

Generated by 🚫 dangerJS against 4f64017

greptile-apps[bot]
greptile-apps bot reviewed Jan 30, 2025
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR implements a secure environment variables management system in the admin panel, allowing read-only access to configuration settings with proper grouping and sensitive data handling.

  • Added getEnvironmentVariablesGrouped GraphQL query with authentication guards and sensitivity control in admin-panel.resolver.ts
  • Introduced @EnvironmentVariablesMetadata decorator in environment-variables-metadata.decorator.ts for organizing variables with groups, descriptions, and sensitivity flags
  • Created hierarchical DTOs in environment-variables.output.ts to structure variables into logical groups and subgroups
  • Added getAll method in environment.service.ts with secure handling of sensitive values through masking
  • Organized environment variables into clear categories with EnvironmentVariablesGroup and EnvironmentVariablesSubGroup enums

11 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile

packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.resolver.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/dtos/environment-variables.output.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/dtos/environment-variables.output.ts Outdated Show resolved Hide resolved
...nty-server/src/engine/core-modules/environment/enums/environment-variables-sub-group.enum.ts Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/environment/environment-variables.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/environment/environment-variables.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/environment/environment-variables.ts Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/environment/environment.service.ts Outdated Show resolved Hide resolved
@ehconitin ehconitin self-assigned this Jan 30, 2025
FelixMalfait
FelixMalfait reviewed Jan 31, 2025
View reviewed changes
Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Great work, looking forward to seeing how this will look!

@ehconitin
Copy link
Contributor Author

@greptileai

greptile-apps[bot]
greptile-apps bot reviewed Jan 31, 2025
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

This PR continues the implementation of the environment variables management system in the admin panel. Here are the key new changes since the last review:

  • Added frontend GraphQL types in graphql.tsx for environment variables querying and display
  • Integrated useGetEnvironmentVariablesGroupedQuery hook in SettingsAdminContent.tsx for fetching environment data
  • Implemented masking strategies in environment-variable-mask-sensitive-data.util.ts for secure handling of sensitive values
  • Added typed reflection support in typed-reflect.ts for environment variable metadata handling

A few important points to note:

  • There's a debug console.log in SettingsAdminContent.tsx that should be removed before merging
  • The UI implementation for displaying the environment variables is not yet complete
  • The validator in the environment variables metadata decorator always returns true without validation

The changes maintain the secure and organized approach established in the previous review while adding the necessary frontend infrastructure.

18 file(s) reviewed, 18 comment(s)
Edit PR Review Bot Settings | Greptile

packages/twenty-front/src/modules/settings/admin-panel/components/SettingsAdminContent.tsx Outdated Show resolved Hide resolved
packages/twenty-front/src/modules/settings/admin-panel/components/SettingsAdminContent.tsx Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.resolver.ts Show resolved Hide resolved
...nty-front/src/modules/settings/admin-panel/graphql/queries/getEnvironmentVariablesGrouped.ts Outdated Show resolved Hide resolved
...nty-front/src/modules/settings/admin-panel/graphql/queries/getEnvironmentVariablesGrouped.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/environment/environment.service.ts Outdated
Comment on lines 41 to 45
const allEnvVarNames =
Reflect.getMetadata(
ENVIRONMENT_VARIABLES_METADATA_DECORATOR_NAMES_KEY,
EnvironmentVariables,
) || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider caching the metadata result since it won't change during runtime and reflection operations are expensive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔

packages/twenty-server/src/engine/core-modules/environment/environment.service.ts Outdated
Comment on lines 64 to 67
const varMaskingConfig =
ENVIRONMENT_VARIABLES_MASKING_CONFIG[
key as keyof typeof ENVIRONMENT_VARIABLES_MASKING_CONFIG
];
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Add error handling for invalid masking config access. TypeScript casting doesn't guarantee runtime safety

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔

...r/src/engine/core-modules/environment/utils/environment-variable-mask-sensitive-data.util.ts Outdated Show resolved Hide resolved
...r/src/engine/core-modules/environment/utils/environment-variable-mask-sensitive-data.util.ts Outdated Show resolved Hide resolved
...r/src/engine/core-modules/environment/utils/environment-variable-mask-sensitive-data.util.ts
Comment on lines +19 to +29
const url = new URL(value);

if (url.password) {
url.password = '********';
}
if (url.username) {
url.username = '********';
}

return url.toString();
} catch {
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: URL parsing silently returns unmasked value on invalid URLs. This could expose sensitive data if the string contains credentials but isn't a valid URL. Consider additional validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FelixMalfait 🤔

martmull
martmull reviewed Jan 31, 2025
View reviewed changes
Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

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

Hey, nice code thank you. I have left some comments

packages/twenty-server/src/engine/core-modules/admin-panel/admin-panel.service.ts Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/admin-panel/dtos/environment-variables.output.ts Outdated Show resolved Hide resolved
...server/src/engine/core-modules/environment/constants/environment-variables-group-position.ts Show resolved Hide resolved
...server/src/engine/core-modules/environment/constants/environment-variables-masking-config.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/environment/environment.service.ts Show resolved Hide resolved
...r/src/engine/core-modules/environment/utils/environment-variable-mask-sensitive-data.util.ts Show resolved Hide resolved
...r/src/engine/core-modules/environment/utils/environment-variable-mask-sensitive-data.util.ts Outdated Show resolved Hide resolved
...r/src/engine/core-modules/environment/utils/environment-variable-mask-sensitive-data.util.ts
}

default:
return value;
Copy link
Contributor

Choose a reason for hiding this comment

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

same, or throw error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't in this case !? If that env var is not in maskConfig it will return its value

@ehconitin ehconitin requested a review from martmull January 31, 2025 18:10
FelixMalfait
FelixMalfait approved these changes Feb 2, 2025
View reviewed changes
Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Added a few comments but good for me you can merge when you feel it's good! Thank you

martmull
martmull reviewed Feb 3, 2025
View reviewed changes
Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

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

One change then GTM

@ehconitin ehconitin requested a review from martmull February 3, 2025 13:21
martmull
martmull approved these changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

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

lgtm thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@martmull martmull martmull approved these changes

@FelixMalfait FelixMalfait FelixMalfait approved these changes

Assignees

@ehconitin ehconitin

Labels
-PR: awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ehconitin @FelixMalfait @martmull