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.

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

refactor(server): Add ApiUrl + ServerUrl env + allow usage of https #8579

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

Conversation

AMoreaux
Copy link
Contributor

@AMoreaux AMoreaux commented Nov 19, 2024

  • Use SERVER_URL to define the NestJS instance URL
  • Add API_URL to set the public URL. Loadbalancer in production. In non-secure environments, it will be the same as the SERVER_URL
  • Allows setting an SSL certificate and using https.

Replaced individual environment-based server URL retrievals with a centralized ServerUrl utility. This change simplifies URL management and ensures a consistent approach across different services. Added validation for SSL configurations when using HTTPS.
Removed standalone ServerUrl and integrated with combined ServerUrl/ApiUrl module. Refactored codebase to use ApiUrl for public network accessibility and adjusted corresponding imports.
Renamed utils file for better readability and consistency. Updated all references to the new file name and added unit tests for ServerUrl and ApiUrl functionalities.
Copy link

github-actions bot commented Nov 19, 2024

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 79a8da2

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 adds HTTPS support and introduces separate URL management for server and API endpoints, with SSL certificate configuration and improved URL handling across the application.

  • Added SSL_KEY_PATH and SSL_CERT_PATH environment variables in /packages/twenty-server/.env.example for HTTPS support
  • Introduced ServerUrl and ApiUrl utilities in /packages/twenty-server/src/engine/utils/server-and-api-urls.ts for centralized URL management
  • Added validation in main.ts to ensure SSL certificates exist when using HTTPS protocol
  • Refactored services to use ApiUrl.get() instead of direct environment variable access for consistent URL handling
  • Added IPv6 support and protocol detection in URL handling with proper hostname normalization

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

packages/twenty-server/.env.example Outdated Show resolved Hide resolved
packages/twenty-server/.env.example Outdated Show resolved Hide resolved
packages/twenty-server/src/main.ts Show resolved Hide resolved
packages/twenty-server/src/main.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/main.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/main.ts Show resolved Hide resolved
Move the reset of ServerUrl and ApiUrl to beforeEach in tests to ensure clean state before each test runs. Update example SSL_KEY_PATH and SSL_CERT_PATH to use relative paths for more portability.
Import the ApiUrl module and set the local URL in the setup function. This ensures the tests have the correct API endpoint configured.
Reordered import statements in `workspace-invitation.service.spec.ts` for improved code organization. This change ensures that dependencies are imported in a more logical sequence.
Included API_URL in self-hosting documentation for public endpoint configuration. This helps developers set up the correct URLs for API interactions in their self-hosted environments.
packages/twenty-server/.env.example Outdated Show resolved Hide resolved
packages/twenty-server/src/main.ts Outdated Show resolved Hide resolved
Removed protocol check logic from main.ts and enforced URL validation to require protocol directly in environment variables definition. This ensures consistent and secure URL formats throughout the application.
Add a new Bash script for generating self-signed SSL certificates, including a README with instructions. Supports customizable domain, root certificate name, and validity period, and integrates root certificate into macOS keychain.
@AMoreaux AMoreaux requested a review from Weiko November 20, 2024 14:41
@@ -6,8 +6,8 @@ import { AxiosResponse } from 'axios';

import { Query } from 'src/engine/api/rest/core/types/query.type';
import { getServerUrl } from 'src/utils/get-server-url';
Copy link
Member

Choose a reason for hiding this comment

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

add TODO + Deprecate flag

// prevent ipv6 issue for redirectUri builder
url.hostname = url.hostname === '[::1]' ? 'localhost' : url.hostname;

ServerUrl.set(url.toString());
Copy link
Member

Choose a reason for hiding this comment

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

Make API_URL mandatory (maybe with default in environment.variable or in .env.example)

@@ -68,7 +86,17 @@ const bootstrap = async () => {
app.use(session(getSessionStorageOptions(environmentService)));
}

await app.listen(process.env.PORT ?? 3000);
await app.listen(serverUrl.port, serverUrl.hostname);
Copy link
Member

Choose a reason for hiding this comment

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

this won't work for self-hosting configuration where both server and front are server by server + this will prevent internal curls


const bootstrap = async () => {
const app = await NestFactory.create<NestExpressApplication>(AppModule, {
cors: true,
bufferLogs: process.env.LOGGER_IS_BUFFER_ENABLED === 'true',
rawBody: true,
snapshot: process.env.DEBUG_MODE === 'true',
...(process.env.SERVER_URL &&
process.env.SERVER_URL.startsWith('https') &&
process.env.SSL_KEY_PATH &&
Copy link
Member

Choose a reason for hiding this comment

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

I would only make the check SSL_KEY_PATH and SSL_CERT_PATH

ServerUrl.set(url.toString());
ApiUrl.set(environmentService.get('API_URL'));

logger.log(`Application is running on: ${url.toString()}`, 'Server Info');
Copy link
Member

Choose a reason for hiding this comment

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

API is running

Copy link
Member

Choose a reason for hiding this comment

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

remove

@@ -37,6 +37,7 @@ yarn command:prod cron:calendar:calendar-event-list-fetch
['REDIS_URL', 'redis://localhost:6379', 'Redis connection url'],
['FRONT_BASE_URL', 'http://localhost:3001', 'Url to the hosted frontend'],
['SERVER_URL', 'http://localhost:3000', 'Url to the hosted server'],
['API_URL', 'http://my-load-balancer', 'Url to the public endpoint'],
Copy link
Member

Choose a reason for hiding this comment

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

URL to the API

@@ -37,6 +37,7 @@ yarn command:prod cron:calendar:calendar-event-list-fetch
['REDIS_URL', 'redis://localhost:6379', 'Redis connection url'],
['FRONT_BASE_URL', 'http://localhost:3001', 'Url to the hosted frontend'],
Copy link
Member

@charlesBochet charlesBochet Nov 22, 2024

Choose a reason for hiding this comment

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

this will be renamed FRONT_DOMAIN + default subDomain + protocol

Copy link
Member

Choose a reason for hiding this comment

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

redirect (workspace.subDomain . FRONT_DOMAIN)

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep: FRONT_BASE_URL and extract defaultSubDomain + protocol + domain from there when we need it

Copy link
Member

Choose a reason for hiding this comment

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

in case we are not in multidomain, we should expect defaultSubDomain to be extratable from FRONT_BASE_URL

@@ -0,0 +1,37 @@
// The url of the Server, should be exposed in a private network
const ServerUrl = (() => {
Copy link
Member

Choose a reason for hiding this comment

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

remove these classes

@@ -234,7 +235,7 @@ export class WorkspaceInvitationService {
link: link.toString(),
workspace: { name: workspace.displayName, logo: workspace.logo },
sender: { email: sender.email, firstName: sender.firstName },
serverUrl: this.environmentService.get('SERVER_URL'),
serverUrl: ApiUrl.get(),
Copy link
Member

Choose a reason for hiding this comment

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

pass API_URL from environmentService

@@ -70,6 +71,7 @@ describe('WorkspaceInvitationService', () => {
environmentService = module.get<EnvironmentService>(EnvironmentService);
emailService = module.get<EmailService>(EmailService);
onboardingService = module.get<OnboardingService>(OnboardingService);
ApiUrl.set('http://localhost:3000');
Copy link
Member

Choose a reason for hiding this comment

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

not need anymore

@@ -199,7 +193,11 @@ export class SSOService {
buildIssuerURL(
identityProvider: Pick<WorkspaceSSOIdentityProvider, 'id' | 'type'>,
) {
return `${this.environmentService.get('SERVER_URL')}/auth/${identityProvider.type.toLowerCase()}/login/${identityProvider.id}`;
const authorizationUrl = new URL(ApiUrl.get());
Copy link
Member

Choose a reason for hiding this comment

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

pass API_URL from environmentService

request,
this.environmentService.get('SERVER_URL'),
);
const baseUrl = getServerUrl(request, ApiUrl.get());
Copy link
Member

Choose a reason for hiding this comment

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

pass API_URL from environmentService

request,
this.environmentService.get('SERVER_URL'),
);
const baseUrl = getServerUrl(request, ApiUrl.get());
Copy link
Member

Choose a reason for hiding this comment

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

pass API_URL from environmentService ==> actually use the existing SERVER_URL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants