Skip to content

fix(apiserver): refuse an identity purge that cannot finish - #5000

Open
rasty94 wants to merge 1 commit into
dexidp:masterfrom
rasty94:upstream-purge-atomic
Open

fix(apiserver): refuse an identity purge that cannot finish#5000
rasty94 wants to merge 1 commit into
dexidp:masterfrom
rasty94:upstream-purge-atomic

Conversation

@rasty94

@rasty94 rasty94 commented Sep 3, 2026

Copy link
Copy Markdown

Overview

DeleteUserIdentity can destroy half a user's data and then fail, with no way to finish the job from the API. This checks the step that predictably fails before anything is deleted.

What this PR does / why we need it

The purge cascades over several stores — auth sessions, refresh tokens, offline sessions, the password record, the identity — and dex has no transaction across them. The password record is deleted near the end, and a password that comes from the config file cannot be deleted through the API at all.

So purging a static user today:

  1. ends every session (notifying relying parties over back-channel logout),
  2. revokes every refresh token,
  3. deletes the offline sessions,
  4. fails at purge password: static passwords: read-only cannot delete password.

The caller gets an error, but the account is still there, the user is signed out everywhere, and there is nothing they can do from the API to complete or undo it. For an endpoint whose documented purpose is a GDPR erasure, "half done and not repeatable" is the worst outcome available.

This checks first and refuses:

purge user identity: the password for "jane@example.com" comes from dex's configuration
file and cannot be deleted through the API; remove the user from the config file first.
Nothing was deleted

Special notes for your reviewer

Asking the question needs the static wrapper to answer it, hence IsStaticPassword on staticPasswordsStorage. It is a type assertion in the apiserver rather than a method on storage.Storage, because only the purge needs to ask, and only in order to refuse.

The subtle part is the forwarding. The three static wrappers embed Storage as an interface, so they do not promote each other's methods: a check that only works when the password wrapper is outermost passes its test and does nothing in a real dex, where serve.go stacks clients → passwords → connectors. Each wrapper forwards the question inwards, and the test stacks them the same way serve.go does.

The test asserts the session and the identity survive before it asserts the wording, so reverting the fix fails it on the defect rather than on the message.

Reordering the cascade to attempt the password first was the smaller diff, but it moves the partial state rather than removing it: a non-static user would then lose their password and keep their sessions, which is worse than the reverse.

storage/sql does not build on master right now (testing.(*B).Output requires go1.25, in postgres_test.go); unrelated to this change.

DeleteUserIdentity cascades over several stores, and dex has no transaction
across them. The password record is deleted near the end, and a password that
comes from the config file cannot be deleted through the API at all. So
purging a static user ends every session, revokes every refresh token, and
then fails at the password step -- leaving the account standing, the user
signed out everywhere, and no way to finish from the API.

Check the one step that fails for a predictable reason before destroying
anything, and refuse with a message that says what to do instead.

Asking the question needs the static wrapper to answer it, which is what
IsStaticPassword is for. The three static wrappers embed Storage as an
interface, so they do not promote each other's methods and the answer would
otherwise depend on which one happens to be outermost; each forwards the
question inwards, and the test stacks them the way serve.go does.

Signed-off-by: rasty94 <ajcomputerses@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant