feat(SUP-52187): UI conf restoration from admin console

[inbalvasserman]

add the option to restore uiconf from the admin console

SUP-52187

[github-actions]

@github-copilot suggest

[github-actions]

@github-copilot suggest

[shahbaa123]

🤖 AI PR Review

🚦 Verdict: 🔴 CRITICAL

The new restoreAction mutates a UIConf's status but omits the global-partner authorization check that every other mutating action in this same service enforces, letting a caller without the dedicated permission restore a shared/global UIConf.

---

📌 Context Summary & Code Archaeology

This PR replaces a manual, T3-only recovery process (a one-off script that flips a soft-deleted UIConf and its file syncs back to "ready") with a self-service Admin Console action. The Jira ticket's explicit expected outcome is "restrict access to authorized roles only" and "eliminate the need for manual scripts... for this action."

The new UiConfAdminService::restoreAction() closely mirrors the logic of the existing internal script that performs this same recovery today, retrieving the UIConf and its local file syncs (bypassing the criteria filter that hides deleted rows) and flipping their status back to ready.

---

📋 Actionable Feedback

1. BLOCKER — Missing global-partner permission check in restoreAction. File: plugins/admin_console/services/UiConfAdminService.php. Every other mutating action in this file (addAction, updateAction, deleteAction) explicitly checks if ($dbUiConf->getPartnerId() == PartnerPeer::GLOBAL_PARTNER && !kPermissionManager::isPermitted(self::PERMISSION_GLOBAL_PARTNER_UI_CONF_UPDTAE)) before mutating a UIConf owned by the global partner. restoreAction performs an equivalent mutation (flips status) but has no such check, so any caller permitted to reach this service can restore a global-partner UIConf even without the dedicated permission that delete/update require for the exact same entity. This directly contradicts the ticket's own "restrict access to authorized roles only" requirement.

   function restoreAction($id)
   {
       uiConfPeer::setUseCriteriaFilter(false);
       $dbUiConf = uiConfPeer::retrieveByPK($id);
       uiConfPeer::setUseCriteriaFilter(true);
   
       if (!$dbUiConf)
           throw new KalturaAPIException(APIErrors::INVALID_UI_CONF_ID, $id);
   
       if ($dbUiConf->getPartnerId() == PartnerPeer::GLOBAL_PARTNER && !kPermissionManager::isPermitted(self::PERMISSION_GLOBAL_PARTNER_UI_CONF_UPDTAE))
           throw new KalturaAPIException(APIErrors::INVALID_UI_CONF_ID, $id);
       ...

2. MAJOR — No verification that the underlying files still exist before marking file syncs "ready". File: plugins/admin_console/services/UiConfAdminService.php, restoreAction. The internal script this logic was modeled on validates file_exists($fileSync->getFullPath()) for every file sync before flipping any status, and aborts if a file is missing. The new action skips that check entirely — it sets every discovered file sync (and the UIConf itself) to "ready" regardless of whether the physical file is actually still on disk. If the file was already purged (e.g. by a retention/cleanup job), the admin gets a false "restored" success while the widget/skin is actually still broken.

3. MAJOR — Soft-deleted records are exposed based on a client-supplied string match. File: plugins/admin_console/services/UiConfAdminService.php, listAction/doList. Whether deleted UIConfs are included in the list is decided by strpos(strtolower(kCurrentContext::$client_lang), 'admin') !== false — a substring match on the caller-supplied clientTag. This is a fragile, implicit way to control visibility of soft-deleted data: any current or future caller whose clientTag happens to contain "admin" (for the same allowed partner) will silently start receiving deleted records. Recommend an explicit, exact clientTag comparison (or a dedicated boolean action parameter defaulting to false) instead of a loose substring heuristic for a decision that affects data visibility.

4. MINOR — Restore precondition not validated. restoreAction never checks that the target UIConf is actually in UI_CONF_STATUS_DELETED before restoring, unlike deleteAction/updateAction in the same file, which validate preconditions before mutating. Calling restore on a UIConf that isn't deleted silently forces its file syncs to "ready" instead of returning a clear validation error.

5. MINOR — UI gates the new "Restore" option with the existing delete ACL instead of a dedicated one. File: admin_console/views/scripts/widget/list.phtml. The server side adds a distinct restore permission item (deployment/permissions/service.adminconsole.uiconfadmin.ini), but the view shows the "Restore" menu option behind Infra_AclHelper::isAllowed('widget','delete') rather than a restore-specific ACL check. Worth confirming this reuse is intentional rather than a placeholder.

6. MINOR — Hard-coded status literal in the view. File: admin_console/views/scripts/widget/list.phtml, <?php if ($uiConf->status == 3): ?>. 3 corresponds to uiConf::UI_CONF_STATUS_DELETED but there's no symbolic constant available on the admin-console/client side referencing it, so the raw literal is used with no explanatory comment.

[github-actions]

@github-copilot suggest

[shahbaa123]

🤖 AI PR Review (Updated)

🚦 Verdict: 🟡 SUBOPTIMAL

The blocking authorization gap from the prior review is fixed, but the ported recovery logic still drops one safety check from the original script it was modeled on — a UIConf can be marked "restored" even when it has zero known file syncs.

---

🔄 Re-Review Delta

• Prior review date: 2026-08-05
• Issues resolved since last review (commit add permission checks):
  1. BLOCKER — Missing global-partner permission check in restoreAction → fixed, now mirrors addAction/updateAction/deleteAction.
  2. MAJOR — No verification files exist before marking file syncs "ready" → fixed, file_exists() check added, throws UI_CONF_FILE_SYNC_MISSING if any found file sync's file is missing on disk.
  3. MAJOR — Soft-deleted records exposed via a loose strpos substring match on clientTag → fixed, now an exact === comparison.
  4. MINOR — Restore precondition not validated → fixed, now throws UI_CONF_NOT_IN_DELETED_STATUS if the UIConf isn't in UI_CONF_STATUS_DELETED.
  5. MINOR — UI gated "Restore" behind the delete ACL → fixed, now uses a dedicated Infra_AclHelper::isAllowed('widget','restore') check (confirmed this resolves correctly against the existing access.widget.all wildcard rule, no additional ACL config needed).
• Issues still open:
  6. MINOR — Hard-coded status literal 3 in list.phtml (if ($uiConf->status == 3 ...)). An explanatory inline comment was added but the magic number itself remains unreplaced by a shared constant.
• New issues found: see below.

---

📋 Actionable Feedback

1. MAJOR — restoreAction silently proceeds when no file sync is found at all. File: plugins/admin_console/services/UiConfAdminService.php, lines 148–172. The reference script this logic was ported from (alpha/scripts/utils/unDeleteUIConf.php) explicitly aborts with die("No file_sync found...") when $fileSyncs is empty after checking all three sub-types. The new restoreAction has no equivalent guard: if none of the DATA/CONFIG/FEATURES file syncs are found, both foreach loops over $fileSyncs simply execute zero iterations, and execution falls through to setting the UIConf's status to UI_CONF_STATUS_READY and saving it. The caller gets a successful "restored" response for a UIConf that has no underlying content at all.

   $fileSyncs = array();
   FileSyncPeer::setUseCriteriaFilter(false);
   foreach ($fileSyncSubTypes as $subType)
   {
       $fileSync = kFileSyncUtils::getLocalFileSyncForKey($dbUiConf->getSyncKey($subType), false);
       if ($fileSync)
           $fileSyncs[] = $fileSync;
   }
   FileSyncPeer::setUseCriteriaFilter(true);
   
   if (empty($fileSyncs))
       throw new KalturaAPIException(KalturaAdminConsoleErrors::UI_CONF_FILE_SYNC_MISSING, $id, '');

2. MINOR (carried over, still open) — Hard-coded status literal in the view. File: admin_console/views/scripts/widget/list.phtml, <?php if ($uiConf->status == 3 /* uiConf::UI_CONF_STATUS_DELETED */): ?>. The comment clarifies intent but 3 is still a raw literal rather than a shared constant exposed to the admin-console/client side.

[github-actions]

@github-copilot suggest

[shahbaa123]

🤖 AI PR Review (Updated)

🚦 Verdict: 🔴 CRITICAL

The two issues from the prior review are addressed, but the fix for the "hard-coded status literal" finding introduces a new class that will fatal-error at runtime and lives in a path this repo's own .gitignore marks as generated, not hand-maintained.

---

🔄 Re-Review Delta

• Prior review date: 2026-08-06
• Issues resolved since last review (commit "fix code review issues in uiConf restore flow"):
  1. MAJOR — restoreAction silently proceeded when zero file syncs were found → fixed. An empty($fileSyncs) guard now throws UI_CONF_FILE_SYNC_MISSING before the restore proceeds, matching the reference script's behavior.
  2. MINOR — hard-coded status literal 3 in list.phtml → replaced with a named constant, but the replacement introduces a new issue (see below).
• New issues found: see below.

---

📋 Actionable Feedback

1. BLOCKER — New enum class extends a non-existent base class and lives in a git-ignored, generator-owned directory. File: admin_console/lib/Kaltura/Client/Enum/UiConfStatus.php.

   • The repo's root .gitignore explicitly excludes admin_console/lib/Kaltura/Client — that entire tree is the auto-generated Kaltura API client library produced at build time, not hand-authored source (the same category as **/om/Base*.php for Propel). A hand-written file checked into this path will not survive a real client-library regeneration.
   • More urgently, the new class declares class Kaltura_Client_Enum_UiConfStatus extends Kaltura_Client_EnumBase, and Kaltura_Client_EnumBase does not exist anywhere in this codebase. The legitimate generated enum classes used elsewhere in admin_console (e.g. Kaltura_Client_Enum_PartnerStatus, Kaltura_Client_Enum_PermissionName) are produced by a generator not present in this repo, and the analogous plain client-enum pattern found under deployment/uiconf/KalturaClient.php (class KalturaEntryStatus { const ... }) shows these enum classes don't extend a shared base at all. Since list.phtml now references Kaltura_Client_Enum_UiConfStatus::DELETED unconditionally when rendering the widget list, loading this class will throw Fatal error: Class 'Kaltura_Client_EnumBase' not found the first time a deleted UIConf row is rendered — breaking the exact "Restore" UI this PR is adding.

   Suggested fix — keep the constant in a tracked, hand-maintained file that's already part of this PR instead of the generated client directory, e.g.:

   // admin_console/controllers/WidgetController.php
   class WidgetController extends Zend_Controller_Action
   {
       const HTML5_URL = 'html5_url';
       const UI_CONF_STATUS_DELETED = 3;
       ...

   // admin_console/views/scripts/widget/list.phtml
   <?php if ($uiConf->status == WidgetController::UI_CONF_STATUS_DELETED): ?>

   and remove admin_console/lib/Kaltura/Client/Enum/UiConfStatus.php.

[github-actions]

@github-copilot suggest

[shahbaa123]

🤖 AI PR Review (Updated)

🚦 Verdict: 🟢 SAFE

Delta: BLOCKER (enum class in git-ignored generated dir, extending non-existent base) → fixed. Constant moved to WidgetController::UI_CONF_STATUS_DELETED, generated-dir file removed, list.phtml updated accordingly.

No open or new issues. Good to merge.

[github-actions]

@github-copilot suggest