Skip to content

fix: handle team deletion for dex user - #1126

Open
CasLubbers wants to merge 4 commits into
mainfrom
APL-2275
Open

CasLubbers wants to merge 4 commits into
mainfrom
APL-2275

Conversation

@CasLubbers

@CasLubbers CasLubbers commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

When a team was deleted, Dex users kept the team-<id> group. That breaks user edits (Team(s) not found) and gives old members access again if a team with the same name is recreated.

deleteAplTeam now strips the group from all Dex users before deleting the team.

Why the lock

Dex can't add or remove a single group. UpdatePassword always replaces the whole list. So every change is read, modify, write, and two at the same time can overwrite each other. For example, deleting blue and red at the same time could bring blue back.

withDexGroupLock is a small promise queue: each group change waits for the previous one to finish, then reads fresh data. It covers team delete, editUser and editTeamUsers. It's in-process only, which is fine since apl-api runs a single replica.

How to test

  • On a Dex cluster: add a user to teams blue and red, delete blue, then check that the user only has team-red and can still be edited.

Signed-off-by: Cas Lubbers <clubbers@akamai.com>
Copilot AI balanced review requested due to automatic review settings November 6, 2026 10:17
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Coverage report

St.❔
Category Percentage Covered / Total
🟡 Statements
60.7% (+0.39% 🔼)
3037/5003
🔴 Branches
47.26% (+0.11% 🔼)
896/1896
🔴 Functions
54.44% (+0.94% 🔼)
441/810
🔴 Lines
59.99% (+0.37% 🔼)
2775/4626
Show files with reduced coverage 🔻
St.❔
File Statements Branches Functions Lines
🟢 error.ts
80.43% (-4.35% 🔻)
57.14% (-7.14% 🔻)
64.29% (-7.14% 🔻)
80% (-4.44% 🔻)

Test suite run success

905 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from 1a9b95b

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical issues remain because deleting admin strips team-administrator roles, while snapshot-based group updates can overwrite concurrent role changes or restore deleted team memberships.

2 open issues
What changed in this PR

This PR updates team deletion to remove the deleted team’s membership group from Dex users before deleting team resources.

Changes:

  • Adds Dex-only membership cleanup before team deletion.
  • Aborts deletion if a Dex update fails.
  • Adds tests for membership cleanup, failure handling, and unchanged Keycloak behavior.
File Description
src/​otomi-stack.ts Removes team groups from Dex users before deleting team resources.
src/​otomi-stack.test.ts Tests Dex cleanup, deletion aborts, and Keycloak isolation.

Review effort: Balanced

Comment thread src/otomi-stack.ts Resolved
Comment thread src/otomi-stack.ts Resolved
Copilot AI balanced review requested due to automatic review settings November 6, 2026 13:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Unsynchronized group-list updates can restore deleted memberships or overwrite concurrent edits, while deleting the reserved admin team strips the global team-admin role from every Dex team admin.

2 open issues

Review effort: Balanced

…user groups

Signed-off-by: Cas Lubbers <clubbers@akamai.com>
Copilot AI balanced review requested due to automatic review settings November 6, 2026 14:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Deleting the reserved admin ID can revoke the global team-admin role from all matching Dex users because membership cleanup lacks a reserved-ID guard.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread src/otomi-stack.ts
Comment on lines +1096 to 1098
if (env.AUTH_PROVIDER === 'dex') await this.removeTeamFromDexUsers(id)
const filePaths = await this.deleteTeamObjects(id)
await this.doDeleteDeployment(filePaths)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

users are not stored in the filestore

Signed-off-by: Cas Lubbers <clubbers@akamai.com>
Copilot AI balanced review requested due to automatic review settings November 6, 2026 16:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Critical concurrency gaps allow membership updates and user creation to retain or restore deleted team groups, leaving stale memberships that can restore access if the team is recreated.

1 open issue

Review effort: Balanced

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.

3 participants