Page MenuHomePhabricator

Add striker UI features for disabling/enabling/deleting tools,
Closed, ResolvedPublicFeature

Description

The per-tool page (e.g. https://toolsadmin.wikimedia.org/tools/id/aaaaaa) should provide a UI for disabling/enabling a tool.

disable

On the backend, disabling a tool should change three ldap settings:

  • pwdAccountLockedTime set to the current time
  • pwdPolicySubentry set to cn=disabled,ou=ppolicies,dc=wikimedia,dc=org
  • loginShell: /bin/disabledtoolshell

Once a tool is disabled, most UI options should also be disabled. The page should include a note about the tool being disabled along with a countdown or date about when the tool will be deleted or archived. It should also provide a widget to re-enable the tool.

enable

On the backend, enabling a tool should change three ldap settings:

  • pwdAccountLockedTime cleared
  • pwdPolicySubentry cleared
  • loginShell: /bin/bash

delete

The 'delete tool' feature should only be available to toolforge admins. Ideally it's available on some sort of bulk UI but a per-tool option would also be ok.

Deleting a tool should make the following ldap changes:

  • pwdAccountLockedTime set to 000001010000Z (indicates 'delete immediately')
  • pwdPolicySubentry set to cn=disabled,ou=ppolicies,dc=wikimedia,dc=org
  • loginShell: /bin/disabledtoolshell

A deleted tool should display all the same UI features as a disabled tool, but there should NOT be an 'enable' option present.

Event Timeline

Andrew renamed this task from Add striker UI for disabling/enabling/deleting tools, to Add striker UI features for disabling/enabling/deleting tools,.Jun 23 2021, 4:39 PM
Andrew created this task.
  • Who should be able to enable a disabled tool? Any member of the tool + admins, or only admins?
  • When a tool is disabled, should it's maintainers list be set to only User:Owner of abandoned tools, or left alone?
  • When a tool is disabled, should the toolinfo records for the tool be purged from the database as well?
  • Who should be able to enable a disabled tool? Any member of the tool + admins, or only admins?

Any member of the tool + admins seems reasonable so that users could reenable their own tool if they change their minds.

  • When a tool is disabled, should it's maintainers list be set to only User:Owner of abandoned tools, or left alone?

Maybe add it as a maintainer, but don't require removal of current maintainers to allow maintainers to reenable.

  • Who should be able to enable a disabled tool? Any member of the tool + admins, or only admins?

Any member or an admin. For bad actors we have the delete option.

  • When a tool is disabled, should it's maintainers list be set to only User:Owner of abandoned tools, or left alone?

I don't think there's any need to adjust the maintainer list. The actual ldap record deletion (including maintainer references) will happen elsewhere, most likely via a cron on the NFS server as part of archiving.

  • When a tool is disabled, should the toolinfo records for the tool be purged from the database as well?

I don't immediately know what's in the toolinfo record, but my guess is that the toolinfo record should be left as is (since 'disabled' is a tentative, reversible step).

That raises the question of how Striker should know/react to a tool actually being fully deleted from LDAP. Striker mostly doesn't need to know about these cases, but it might be useful to have some kind of reconciliation process where Striker notices that it has things in its DB that are no longer in ldap and wipes them out to prevent future name collisions.

BTW, an overly-detailed picture of what happens after someone clicks one of those boxes can be seen here:

https://phabricator.wikimedia.org/T170355#7172542

  • When a tool is disabled, should the toolinfo records for the tool be purged from the database as well?

I don't immediately know what's in the toolinfo record, but my guess is that the toolinfo record should be left as is (since 'disabled' is a tentative, reversible step).

The toolinfo record is the Hay's Directory/Toolhub compatible description of what the tool does. We will very much want to prune this data or at least change the app to stop publishing records once a tool is disabled. If we do not, Hay's Directory and Toolhub will keep telling people that the tool exists after it is gone.

That raises the question of how Striker should know/react to a tool actually being fully deleted from LDAP. Striker mostly doesn't need to know about these cases, but it might be useful to have some kind of reconciliation process where Striker notices that it has things in its DB that are no longer in ldap and wipes them out to prevent future name collisions.

Striker keeps local database records for Phabricator projects and Diffusion repos created for tools. These should get cleaned up if associated with a deleted tool, both in Striker's database and in Phabricator at some point. Striker doesn't have any active polling of LDAP at the moment to see a deletion. I'm not sure if it would be a good idea to add that kind of reconciliation loop to the Django app as a side thread. It would likely be easier to implement an API that can trigger the clean up and let something outside of Striker call that when needed (and/or expose the cleanup in the UI for admins).

The toolinfo record is the Hay's Directory/Toolhub compatible description of what the tool does. We will very much want to prune this data or at least change the app to stop publishing records once a tool is disabled. If we do not, Hay's Directory and Toolhub will keep telling people that the tool exists after it is gone.

Hay's Directory may actually keep publishing the last version of the data it saw no matter what. I honestly don't remember if it notices when a record is removed from a toolinfo.json file. Toolhub will notice though for sure and remove the stored data it has about that tool so that it stops advertising it to folks.

nskaggs triaged this task as Medium priority.Aug 10 2021, 9:53 PM
nskaggs moved this task from Inbox to Soon! on the cloud-services-team (Kanban) board.

This is a bit blocked pending a striker test/dev env and/or someone wanting to work on it. Most of the backend decisions are made though.

bd808 removed Andrew as the assignee of this task.Apr 4 2022, 7:10 PM
bd808 changed the subtype of this task from "Task" to "Feature Request".

That raises the question of how Striker should know/react to a tool actually being fully deleted from LDAP. Striker mostly doesn't need to know about these cases, but it might be useful to have some kind of reconciliation process where Striker notices that it has things in its DB that are no longer in ldap and wipes them out to prevent future name collisions.

This is an issue that will need addressing generally. Striker does not mirror the LDAP tool account into a local database, but it does add various records in it's local database that are linked by name to the LDAP tool account records. In T316991: Add API to export toolinfo v1.2 compatible data I have added some code to filter out toolinfo records associated with LDAP tool account records that no longer exist, but it would be nice to have a more robust system that somehow did cleanup Phabricator projects, GitLab repos, and Toolinfo records that are tied to LDAP tool accounts which have been deleted.

One potential way to handle this would be to add a web API to Striker to process a tool deletion request and have the backend system from T170355: Figure out process for deleting an unused tool call that API just before actually deleting the LDAP records for the tool.

My current sticking point is figuring out how to get django-ldapdb to update the pwdAccountLockedTime operational attribute for a service user. This is currently failing with an ldap.UNDEFINED_TYPE error. I think I'm going to have to figure out how to enable LDAP trace level logging in my development environment to get a clue about how to work around this.

My current sticking point is figuring out how to get django-ldapdb to update the pwdAccountLockedTime operational attribute for a service user. This is currently failing with an ldap.UNDEFINED_TYPE error. I think I'm going to have to figure out how to enable LDAP trace level logging in my development environment to get a clue about how to work around this.

This turned out to be missing configuration in my local openldap service. I thought that I had already added that while implementing T271668: Create Buster dev environment for Striker, but it seems that I was missing some necessary bits.

Change 850267 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[labs/striker@master] feat: Allow disabling tool service accounts

https://gerrit.wikimedia.org/r/850267

Change 850268 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[labs/striker@master] feat: Add UI for disable/enable of tool accounts

https://gerrit.wikimedia.org/r/850268

Change 850267 merged by jenkins-bot:

[labs/striker@master] feat: Allow disabling tool service accounts

https://gerrit.wikimedia.org/r/850267

Change 850268 merged by jenkins-bot:

[labs/striker@master] feat: Add UI for disable/enable of tool accounts

https://gerrit.wikimedia.org/r/850268

Change 850283 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[labs/striker@master] demo: Add missing TOOLS_DISABLED_POLICY_ENTRY envvar

https://gerrit.wikimedia.org/r/850283

Change 850283 merged by jenkins-bot:

[labs/striker@master] demo: Add missing TOOLS_DISABLED_POLICY_ENTRY envvar

https://gerrit.wikimedia.org/r/850283

Change 850301 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[labs/striker@master] feat: reject creating repos, projects, and info for disabled tools

https://gerrit.wikimedia.org/r/850301

Change 850302 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[labs/striker@master] fix: Only show re-enable button to maintainers & admins

https://gerrit.wikimedia.org/r/850302

Change 850301 merged by jenkins-bot:

[labs/striker@master] feat: reject creating repos, projects, and info for disabled tools

https://gerrit.wikimedia.org/r/850301

Change 850302 merged by jenkins-bot:

[labs/striker@master] fix: Only show re-enable button to maintainers & admins

https://gerrit.wikimedia.org/r/850302

Change 851121 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[operations/puppet@production] striker: Bump container version to 2022-10-27-235113-production

https://gerrit.wikimedia.org/r/851121

Change 851121 merged by Andrew Bogott:

[operations/puppet@production] striker: Bump container version to 2022-10-27-235113-production

https://gerrit.wikimedia.org/r/851121