Skip to content

[PB-6134]: feat/stalwart admin api principals#7

Merged
jzunigax2 merged 7 commits intomasterfrom
feat/stalwart-admin-api-principals
Mar 27, 2026
Merged

[PB-6134]: feat/stalwart admin api principals#7
jzunigax2 merged 7 commits intomasterfrom
feat/stalwart-admin-api-principals

Conversation

@jzunigax2
Copy link
Contributor

@jzunigax2 jzunigax2 commented Mar 24, 2026

  • Add StalwartService, HTTP client for the Stalwart admin REST API (principals CRUD via undici/HTTP2)
  • Add StalwartAccountProvider extending AccountProvider port, maps account operations to Stalwart principal operations
  • Add AccountService with address management: add/remove addresses, set primary (delete+recreate principal for rename), delete account with provider cleanup and rollback on failure
  • Add repositories (AccountRepository, AddressRepository, DomainRepository) with Sequelize queries and provider link tracking
  • Add domain models (MailAccount, MailAddress, MailDomain) with builder pattern
  • Unit tests for AccountService, StalwartAccountProvider, and StalwartService

@jzunigax2 jzunigax2 requested review from apsantiso and sg-gs March 24, 2026 02:59
@jzunigax2 jzunigax2 self-assigned this Mar 24, 2026
@jzunigax2 jzunigax2 added the enhancement New feature or request label Mar 24, 2026
@jzunigax2 jzunigax2 force-pushed the feat/stalwart-admin-api-principals branch from e31054d to 1a01822 Compare March 25, 2026 02:33
@jzunigax2 jzunigax2 changed the title Feat/stalwart admin api principals [PB-6134]: feat/stalwart admin api principals Mar 25, 2026
@jzunigax2 jzunigax2 marked this pull request as ready for review March 25, 2026 03:31
Comment on lines +104 to +115
const principalName = this.requirePrincipalName(account);

await this.provider.removeAddress(principalName, address);
await Promise.all([
this.addresses.deleteProviderLink(addressRecord.id),
this.addresses.delete(addressRecord.id),
]);

this.logger.log(
`Removed address '${address}' from account '${driveUserUuid}'`,
);
}
Copy link

@apsantiso apsantiso Mar 25, 2026

Choose a reason for hiding this comment

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

It is somehow hard to wrap my head around this concept as you're using stalwart's implementation names (principal name) in the bussiness layer instead of our domain names such as mainAddress/primaryAddress, I may be wrong tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, my bad. some methods existed before creating our domain names and missed renaming variables creating this odd mix of domain names and stalwart names. addressed

@@ -0,0 +1,91 @@
import { Injectable, Logger } from '@nestjs/common';

Choose a reason for hiding this comment

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

A quick tip if you feel this module gets too big: instead of /modules/infrastructure, you can create a infrastructure folder in the module that requires this service.

Each module can have its own bussiness (usecases), infrastructure (external services, controllers), domain layers it you think things get too crazy. We do not follow a hardcore ports/adapters structure as we tend to be more pragmatic, so it is always up to you @jzunigax2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm lets wait and see how it evolves, I would think we wont need to add much to /infrastructure

Comment on lines +75 to +77
async delete(id: string): Promise<void> {
await this.addressModel.destroy({ where: { id } });
}

Choose a reason for hiding this comment

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

Beware of hard deletes here. We're handling emails so we might want to use soft deletes just in case. You can enable the paranoid option so sequelize handles automatically this for you (by setting a deleted_at), you'll probably need to modify your existent migrations tho

Choose a reason for hiding this comment

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

I suggested the paranoid option but let's see what @sg-gs thinks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

paranoid sounds good, I added it to mail-addresses and mail-accounts. let me know if that sounds good to you

Copy link
Member

@sg-gs sg-gs left a comment

Choose a reason for hiding this comment

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

There are js imports across multiple files @jzunigax2.

Btw, there is a ton heck of code here, let's avoid this gigantic PRs


export interface MailAccountAttributes {
id: string;
driveUserUuid: string;
Copy link
Member

Choose a reason for hiding this comment

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

The user's uuid is common over all the services (drive, network, payments, meet...), therefore, this wording is not accurate. It's better to use userId, as also the format is something not relevant, as the type itself clarifies that is not the incremental id, that btw, we will eventually remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, addressed

address: string;
domainId: string;
isDefault: boolean;
providerExternalId: string | null;
Copy link
Member

Choose a reason for hiding this comment

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

How can this be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, it really should never be.. Made it non-nullable. Each address always has a provider link

export interface MailDomainAttributes {
id: string;
domain: string;
status: string;
Copy link
Member

Choose a reason for hiding this comment

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

Should this status be an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added MailDomainStatus enum with Active and Inactive values

@@ -0,0 +1,43 @@
import { Injectable } from '@nestjs/common';
import { InjectModel } from '@nestjs/sequelize';
import { MailAccount } from '../domain/mail-account.domain.js';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be typescript files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

private readonly accountModel: typeof MailAccountModel,
) {}

async findByDriveUserUuid(uuid: string): Promise<MailAccount | null> {
Copy link
Member

Choose a reason for hiding this comment

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

findByUserId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

): Promise<void> {
// Stalwart uses the principal name as the login.
// Changing the primary address means renaming the principal.
// Current REST API does not support rename — we must recreate.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use a principal handled by us consisting on the user uuid and the mail extension so we prevent these things. Also, which are the consequences of recreating? Would other addresses or the mailbox be deleted from Stalwart?

Copy link
Contributor Author

@jzunigax2 jzunigax2 Mar 26, 2026

Choose a reason for hiding this comment

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

recreating would lose all stored emails and mailbox data since the new principal gets a fresh internal id. however this isn't actually needed this was left over code from an earlier iteration, each address is its own principal, so there's no rename scneario now.

Removed setPrimaryAddress from the provider entirely, it's now a DB-only operation. Also removed addAddress/removeAddress from the port since adding/removing an address means creating/deleting a whole principal via createAccount/deleteAccount rather than adding them as aliasses.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's better, if possible, to decouple a rename on our UI to a rename on the Mail tool itself, if possible

@jzunigax2 jzunigax2 force-pushed the feat/stalwart-admin-api-principals branch from 5046358 to 2919836 Compare March 26, 2026 16:30
@jzunigax2
Copy link
Contributor Author

There are js imports across multiple files @jzunigax2.

Btw, there is a ton heck of code here, let's avoid this gigantic PRs

hey @sg-gs the project uses nodenext for compiler options compilerOptions.module and compilerOptions.moduleResolution, with these options, TypeScript expects the relative import specifiers to match what Node will resolve from the compiled dist files (which are .js) so the extensions are required.

as for the big PR you are right, my bad. initially this was supposed to only include the stalwart api abstraction and I made the mistake to keep building on top of this to see how it would shape out

@jzunigax2 jzunigax2 requested a review from sg-gs March 26, 2026 21:09
Base automatically changed from feat/sequelize-models-wiring to master March 27, 2026 12:17
- Added AccountProvider interface for managing email accounts with methods for creating, deleting, and updating accounts and addresses.
- Implemented StalwartAccountProvider class to interact with the Stalwart service for account management.
- Introduced CreateAccountParams and AccountInfo types for structured account data handling.
- Created StalwartService to manage API interactions with the Stalwart backend, including principal creation, retrieval, and updates.
- Established StalwartModule to provide the StalwartAccountProvider as a dependency for other modules.
- Replaced adminToken with adminUser and adminSecret in the configuration.
- Updated StalwartService to use basic authentication with adminUser and adminSecret instead of bearer token.
- Adjusted headers method to include basic auth credentials for API requests.
- Introduced AccountProvider interface for managing email accounts, including methods for creating, deleting, and updating accounts and addresses.
- Added AccountService to handle business logic for account operations, including address management and primary address setting.
- Created repositories for account, address, and domain management, integrating with Sequelize for data persistence.
- Defined domain models for MailAccount, MailAddress, and MailDomain to structure account-related data.
- Updated .env.template to include new configuration parameters for Stalwart account management.
- Introduced comprehensive unit tests for AccountService, covering account retrieval, deletion, and address management functionalities.
- Added unit tests for StalwartAccountProvider, validating account creation, deletion, and address manipulation methods.
- Updated ESLint configuration to disable unbound-method rule for test files.
- Enhanced test fixtures to support new account and address attributes for improved test coverage.
…ables

- Introduced migrations to add a nullable deleted_at column to both mail_accounts and mail_addresses tables for soft deletion functionality.
- Updated the MailAccount and MailAddress models to include the deletedAt field with paranoid option enabled for Sequelize.
- Refactored AccountService to replace references from principalName to providerAccountId for consistency in account management.
- Created a migration to rename the column drive_user_uuid to user_id in the mail_accounts table for improved clarity and consistency.
- Updated related code in seeders, services, and repositories to reflect the new user_id field.
- Adjusted unit tests to ensure proper functionality with the updated user identification method.
…hods

- Removed addAddress, removeAddress, and setPrimaryAddress methods from the AccountProvider interface and StalwartAccountProvider implementation to streamline account management.
- Updated AccountService to handle address management directly, ensuring proper deletion and linking of addresses during account operations.
- Adjusted related unit tests to reflect the changes in address handling and account deletion processes.
@jzunigax2 jzunigax2 force-pushed the feat/stalwart-admin-api-principals branch from 3efc3a3 to ec2b9e9 Compare March 27, 2026 12:28
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
71.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@jzunigax2 jzunigax2 merged commit 990bf15 into master Mar 27, 2026
4 of 5 checks passed
@jzunigax2 jzunigax2 deleted the feat/stalwart-admin-api-principals branch March 27, 2026 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants