Skip to content

feat(security): periodic job to encrypt plaintext passwords in user_ table#35767

Queued
wezell wants to merge 6 commits into
mainfrom
issue-35766-encrypt-plain-passwords-job
Queued

feat(security): periodic job to encrypt plaintext passwords in user_ table#35767
wezell wants to merge 6 commits into
mainfrom
issue-35766-encrypt-plain-passwords-job

Conversation

@wezell
Copy link
Copy Markdown
Member

@wezell wezell commented May 20, 2026

Proposed Changes

Adds EncryptPlainPasswordsJob — a Quartz StatefulJob that runs every 5 minutes, scans the user_ table for rows whose passwordEncrypted flag is false, hashes the cleartext value via PasswordFactoryProxy.generateHash, and flips the flag to true.

Defense-in-depth against any code path that lands a plaintext password in user_.password_ — migrations, bulk imports, manual SQL recovery, or older code that set the password without the encrypted flag. Once the job ticks, the row is hashed using the same utility as the rest of the platform, so existing login (authPassword) continues to work transparently.

Files Touched

File Change
dotCMS/src/main/java/com/dotmarketing/quartz/job/EncryptPlainPasswordsJob.java New. StatefulJob implementation.
dotCMS/src/main/java/com/dotmarketing/init/DotInitScheduler.java Registers the new job (mirrors FreeServerFromClusterJob pattern).
dotcms-integration/src/test/java/com/dotmarketing/quartz/job/EncryptPlainPasswordsJobTest.java New. Five integration tests.

Configuration

Property Default Effect
ENABLE_ENCRYPT_PLAIN_PASSWORDS_JOB true Kill switch. Checked at startup (job not scheduled if false) and at every firing — flip at runtime without a restart.
ENCRYPT_PLAIN_PASSWORDS_CRON_EXPRESSION 0 0/5 * * * ? Standard Quartz cron.

Why a periodic job rather than a one-off migration

The risk of a fresh plaintext row landing in user_ is non-zero on a live system (admin tooling, recovery scripts, bulk imports that bypass UserAPI). A standing sweep catches them within one minute regardless of how they got there.

The query is cheap: passwordEncrypted = false is an extremely selective predicate, so in steady state the job does an index/sequential check that finds zero rows and returns immediately. A partial index CREATE INDEX ... ON user_ (userId) WHERE passwordEncrypted = false would be the right escalation if perf ever becomes a concern, but is unnecessary today.

Test Plan

Integration tests (EncryptPlainPasswordsJobTest):

  • Happy path — plaintext row gets hashed; authPassword(plaintext, storedHash) returns AUTHENTICATED.
  • Already encrypted row — left untouched (no double-hashing).
  • Null password — skipped (no UPDATE issued).
  • Multiple rows — all hashed in a single pass.
  • Disabled flagENABLE_ENCRYPT_PLAIN_PASSWORDS_JOB=false makes the firing a no-op.

Run locally:

./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=EncryptPlainPasswordsJobTest

Rollback safety

Pure additive — new class, new scheduler registration, new test. No schema change, no API contract change, no frontend touched. If anything misbehaves in production, flip ENABLE_ENCRYPT_PLAIN_PASSWORDS_JOB=false and the job becomes a no-op immediately; revert the commit for a full backout.

Refs #35766

🤖 Generated with Claude Code

Adds EncryptPlainPasswordsJob — a Quartz StatefulJob that runs every minute,
scans user_ for rows where passwordEncrypted=false, hashes the password via
PasswordFactoryProxy.generateHash, and flips the encrypted flag.

Defense-in-depth against plaintext passwords landing in the column via
migrations, bulk imports, or manual SQL inserts.

Configurable via:
  ENABLE_ENCRYPT_PLAIN_PASSWORDS_JOB (default true)  — kill switch enforced
    at both scheduler-startup and per-firing so the flag can be flipped
    without a JVM restart
  ENCRYPT_PLAIN_PASSWORDS_CRON_EXPRESSION (default \"0 0/1 * * * ?\")

Integration tests cover: hash + auth roundtrip, already-encrypted row left
alone, null-password row skipped, multi-row pass, disabled-flag no-op.

Refs #35766

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 20, 2026

Claude finished @wezell's task in 2m 2s —— View job


Claude finished @wezell's task in 2m 17s —— View job


Rollback Safety Analysis — ✅ Safe To Rollback

  • Read rollback-unsafe categories reference
  • Get full PR diff
  • Analyze diff against each category
  • Apply AI: Safe To Rollback label

The PR is purely additive Java code with no schema, mapping, or API-contract changes.

Files reviewed

  • dotCMS/src/main/java/com/dotmarketing/quartz/job/EncryptPlainPasswordsJob.java (new) — runtime job; only SELECT/UPDATE on existing user_ columns (userId, password_, passwordEncrypted). No DDL.
  • dotCMS/src/main/java/com/dotmarketing/init/DotInitScheduler.java — adds a scheduling block that mirrors the existing FreeServerFromClusterJob pattern.
  • dotcms-integration/.../EncryptPlainPasswordsJobTest.java and MainSuite2b.java — test-only.

Category check

ID Category Match?
C-1 Structural data model change No — updates existing column values only, no schema change
C-2 Elasticsearch mapping change No — no ES code touched
C-3 contentlet_as_json model version bump No
C-4 DROP TABLE / DROP essential column No
H-1 One-way data transformation Borderline but not unsafe — see note below
H-2 RENAME TABLE / COLUMN No
H-3 Primary-key restructuring No
H-4 New content-type field type No
H-5 Storage-provider change No
H-6 DROP PROCEDURE / FUNCTION No
H-7 NOT NULL column without default No
M-1 Non-broadening column type change No
M-2 Push-publishing bundle format change No
M-3 REST / GraphQL API contract change No
M-4 OSGi public interface change No

Why H-1 does not apply here
The job rewrites password_ from plaintext to a hash and flips passwordEncrypted to true. The target state (encrypted-flag = true with a hashed value) is the long-standing format that N-1 already expects — N-1 authenticates via PasswordFactoryProxy.authPassword(input, storedHash) against exactly that hash. The transformation moves anomalous rows into the canonical format N-1 was built for, not out of it. Plaintext rows are the bug condition; hashed rows are the normal condition. N-1's read path is unaffected by rollback.

Additionally, both a startup-time kill switch (ENABLE_ENCRYPT_PLAIN_PASSWORDS_JOB) and a per-firing check make the job a no-op at runtime if disabled, so any concern can be neutralized without a code change.
• Branch: issue-35766-encrypt-plain-passwords-job

- UPDATE now WHERE-clauses on (passwordEncrypted=false AND password_=?) so a
  concurrent password change between SELECT and UPDATE cannot be clobbered.
  Zero-rows-affected is logged at debug and counted as skipped.
- Add ENCRYPT_PLAIN_PASSWORDS_BATCH_SIZE (default 500) so a bulk import of
  tens of thousands of plaintext rows cannot pin a Quartz worker thread;
  remaining rows are caught on subsequent ticks.
- Default cron lowered from every-minute to every-5-minutes — defense-in-depth
  does not need 525k firings/year.
- Test: delete the cron-scheduled job in @BeforeClass so the real scheduler
  cannot race the explicit job.execute() calls. Use the default company id
  via PublicCompanyFactory instead of hardcoding "dotcms.org". Add a
  concurrent-writer test for the new race guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@wezell
Copy link
Copy Markdown
Member Author

wezell commented May 20, 2026

Thanks for the review — all four substantive findings addressed in bc1d54d:

1 · UPDATE raceUPDATE_HASHED_PASSWORD_SQL now ends with ... and passwordEncrypted = ? and password_ = ? (binding false, plaintext). If anyone changed the row between SELECT and UPDATE, the affected-rows count is 0, the row is logged at debug as skipped, and the concurrent writer's value is preserved. New test test_concurrent_change_is_not_clobbered exercises this.

2 · No batch limit → added ENCRYPT_PLAIN_PASSWORDS_BATCH_SIZE (default 500). SELECT now ends with LIMIT ?. Remaining rows are caught on the next tick — a bulk-import of 50k rows is bounded to ~100 ticks rather than one 2.5-hour firing.

3 · Test/scheduler race@BeforeClass now calls QuartzUtils.getScheduler().deleteJob(JOB_NAME, DOTCMS_JOB_GROUP_NAME) so the real cron is unscheduled for the duration of the test class. The in-test TestJobExecutor.execute() calls are the only invocations.

5 · Cron frequency → default lowered from 0 0/1 * * * ? to 0 0/5 * * * ?. Still overridable via ENCRYPT_PLAIN_PASSWORDS_CRON_EXPRESSION.

Minor nits → all three applied: this.getClass() replaced with EncryptPlainPasswordsJob.class, FQN java.util.* cleaned up, hardcoded "dotcms.org" replaced with PublicCompanyFactory.getDefaultCompany().getCompanyId() (resolved once in @BeforeClass).

Without this entry the test class compiles but CI never executes it.
MainSuite2b is the existing home for the other quartz job tests
(DotStatefulJobTest, CleanUpFieldReferencesJobTest, etc.).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Validates the real-world claim: the hash format produced by the cron job
is interchangeable with hashes produced by the standard user-create /
change-password flows. Calls LoginFactory.doLogin(userId, plaintext)
against a row that the job hashed and asserts the user can actually log in
(and that a wrong password is rejected).

Replaces the previous direct PasswordFactoryProxy.authPassword assertion,
which was circular validation (same library that produced the hash).

Adds emailAddress to the test insert because LoginFactory.doLogin rejects
users without one.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The end-to-end auth test was failing in CI because the default seeded
company uses authType=emailAddress (AUTH_TYPE_EA), so LoginFactory.doLogin
routes through loadByUserByEmail — but the test was passing the userId.
Resolve the right identifier at runtime based on the company auth type so
the test passes for either AUTH_TYPE_EA or AUTH_TYPE_ID configurations.

Extracted emailFor(userId) helper so the email convention lives in one
place.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LoginFactory.doLogin loads the user via UserAPI, which auto-assigns
default roles — rows in users_cms_roles.user_id FK back to user_. The
naive delete-from-user_ in @after then violates fkusers_cms_roles2.

Clear users_cms_roles for the user first, then user_. The auth test
itself was passing (Failures: 0, Errors: 1) — only the @after tearDown
was failing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@wezell wezell added this pull request to the merge queue May 20, 2026
Any commits made after this event will not be merged.
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 20, 2026
@wezell wezell added this pull request to the merge queue May 20, 2026
Any commits made after this event will not be merged.
@wezell wezell added Team : Platform Platform Team Backend Ticket related to Backend technology labels May 20, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks May 20, 2026
@wezell wezell added this pull request to the merge queue May 21, 2026
Any commits made after this event will not be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code Backend Ticket related to Backend technology Team : Platform Platform Team

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants