OpenWF/SpaceNinjaServer
28
29
Fork 35
Code Issues 23 Pull Requests 1 Actions Packages Projects Releases Activity

feat(webui): delete account #615

Merged
Sainan merged 3 commits from delete-account into main 2024-12-22 18:34:14 -08:00
Conversation 2 Commits 3 Files Changed 4 +32
4 changed files with 32 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 2a9d2a3be6 - Show all commits

19
src/controllers/custom/deleteAccountController.ts Normal file
View File

@ -0,0 +1,19 @@
import { RequestHandler } from "express";
import { getAccountIdForRequest } from "@/src/services/loginService";
import { Account } from "@/src/models/loginModel";
import { Inventory } from "@/src/models/inventoryModels/inventoryModel";
import { Loadout } from "@/src/models/inventoryModels/loadoutModel";
import { PersonalRooms } from "@/src/models/personalRoomsModel";
import { Ship } from "@/src/models/shipModel";
export const deleteAccountController: RequestHandler = async (req, res) => {
const accountId = await getAccountIdForRequest(req);
await Promise.all([
Account.deleteOne({ _id: accountId }),
Inventory.deleteOne({ accountOwnerId: accountId }),
Loadout.deleteOne({ loadoutOwnerId: accountId }),
PersonalRooms.deleteOne({ personalRoomsOwnerId: accountId }),
Ship.deleteOne({ ShipOwnerId: accountId })
]);
res.end();
};
coderabbitai[bot] commented 2024-12-22 17:50:13 -08:00 (Migrated from github.com)
Review
Copy Link

⚠️ Potential issue

Critical: Implement proper security measures and error handling

The current implementation has several security and reliability concerns:

  1. Using GET for deletion operations violates HTTP semantics
  2. No additional authentication verification
  3. No confirmation token validation
  4. Missing error handling
  5. No transaction management

Consider implementing these improvements:

-export const deleteAccountController: RequestHandler = async (req, res) => {
+export const deleteAccountController: RequestHandler = async (req, res, next) => {
+    try {
+        // Verify confirmation token
+        const confirmationToken = req.body.confirmationToken;
+        if (!confirmationToken) {
+            return res.status(400).json({ error: 'Confirmation token required' });
+        }
+
         const accountId = await getAccountIdForRequest(req);
-        await Promise.all([
-            Account.deleteOne({ _id: accountId }),
-            Inventory.deleteOne({ accountOwnerId: accountId }),
-            Loadout.deleteOne({ loadoutOwnerId: accountId }),
-            PersonalRooms.deleteOne({ personalRoomsOwnerId: accountId }),
-            Ship.deleteOne({ ShipOwnerId: accountId })
-        ]);
-        res.end();
+        
+        // Start transaction
+        const session = await mongoose.startSession();
+        await session.withTransaction(async () => {
+            await Promise.all([
+                Account.deleteOne({ _id: accountId }, { session }),
+                Inventory.deleteOne({ accountOwnerId: accountId }, { session }),
+                Loadout.deleteOne({ loadoutOwnerId: accountId }, { session }),
+                PersonalRooms.deleteOne({ personalRoomsOwnerId: accountId }, { session }),
+                Ship.deleteOne({ ShipOwnerId: accountId }, { session })
+            ]);
+        });
+        
+        res.status(200).json({ message: 'Account deleted successfully' });
+    } catch (error) {
+        next(error);
+    }
 };

Committable suggestion skipped: line range outside the PR's diff.

_:warning: Potential issue_ **Critical: Implement proper security measures and error handling** The current implementation has several security and reliability concerns: 1. Using GET for deletion operations violates HTTP semantics 2. No additional authentication verification 3. No confirmation token validation 4. Missing error handling 5. No transaction management Consider implementing these improvements: ```diff -export const deleteAccountController: RequestHandler = async (req, res) => { +export const deleteAccountController: RequestHandler = async (req, res, next) => { + try { + // Verify confirmation token + const confirmationToken = req.body.confirmationToken; + if (!confirmationToken) { + return res.status(400).json({ error: 'Confirmation token required' }); + } + const accountId = await getAccountIdForRequest(req); - await Promise.all([ - Account.deleteOne({ _id: accountId }), - Inventory.deleteOne({ accountOwnerId: accountId }), - Loadout.deleteOne({ loadoutOwnerId: accountId }), - PersonalRooms.deleteOne({ personalRoomsOwnerId: accountId }), - Ship.deleteOne({ ShipOwnerId: accountId }) - ]); - res.end(); + + // Start transaction + const session = await mongoose.startSession(); + await session.withTransaction(async () => { + await Promise.all([ + Account.deleteOne({ _id: accountId }, { session }), + Inventory.deleteOne({ accountOwnerId: accountId }, { session }), + Loadout.deleteOne({ loadoutOwnerId: accountId }, { session }), + PersonalRooms.deleteOne({ personalRoomsOwnerId: accountId }, { session }), + Ship.deleteOne({ ShipOwnerId: accountId }, { session }) + ]); + }); + + res.status(200).json({ message: 'Account deleted successfully' }); + } catch (error) { + next(error); + } }; ``` > Committable suggestion skipped: line range outside the PR's diff. <!-- This is an auto-generated comment by CodeRabbit -->

2
src/routes/custom.ts
View File

@ -4,6 +4,7 @@ import { tunablesController } from "@/src/controllers/custom/tunablesController"
import { getItemListsController } from "@/src/controllers/custom/getItemListsController";
import { pushArchonCrystalUpgradeController } from "@/src/controllers/custom/pushArchonCrystalUpgradeController";
import { popArchonCrystalUpgradeController } from "@/src/controllers/custom/popArchonCrystalUpgradeController";
import { deleteAccountController } from "@/src/controllers/custom/deleteAccountController";
import { createAccountController } from "@/src/controllers/custom/createAccountController";
import { addItemController } from "@/src/controllers/custom/addItemController";
@ -17,6 +18,7 @@ customRouter.get("/tunables.json", tunablesController);
customRouter.get("/getItemLists", getItemListsController);
customRouter.get("/pushArchonCrystalUpgrade", pushArchonCrystalUpgradeController);
customRouter.get("/popArchonCrystalUpgrade", popArchonCrystalUpgradeController);
customRouter.get("/deleteAccount", deleteAccountController);
customRouter.post("/createAccount", createAccountController);
customRouter.post("/addItem", addItemController);

2
static/webui/index.html
View File

@ -38,6 +38,8 @@
<button class="nav-link dropdown-toggle displayname" data-bs-toggle="dropdown" aria-expanded="false"></button>
<ul class="dropdown-menu dropdown-menu-end">
<li><a class="dropdown-item" href="/webui/" onclick="logout();">Logout</a></li>
<li><hr class="dropdown-divider"></li>
<li><a class="dropdown-item" onclick="deleteAccount();">Delete Account</a></li>
coderabbitai[bot] commented 2024-12-22 17:50:13 -08:00 (Migrated from github.com)
Review
Copy Link

🛠️ Refactor suggestion

Enhance user safety and accessibility

The current implementation lacks:

  1. Visual warning about the irreversible action
  2. Confirmation dialog
  3. Accessibility attributes
  4. Loading state during deletion

Consider these improvements:

 <li><hr class="dropdown-divider"></li>
-<li><a class="dropdown-item" onclick="deleteAccount();">Delete Account</a></li>
+<li>
+    <button 
+        class="dropdown-item text-danger"
+        data-bs-toggle="modal"
+        data-bs-target="#deleteAccountModal"
+        aria-label="Delete account permanently"
+    >
+        Delete Account
+    </button>
+</li>

Add this modal for confirmation:

<div class="modal fade" id="deleteAccountModal" tabindex="-1" aria-labelledby="deleteAccountModalLabel" aria-hidden="true">
    <div class="modal-dialog">
        <div class="modal-content">
            <div class="modal-header">
                <h5 class="modal-title" id="deleteAccountModalLabel">Delete Account</h5>
                <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button>
            </div>
            <div class="modal-body">
                <p class="text-danger">Warning: This action cannot be undone. All your data will be permanently deleted.</p>
                <p>Please type "DELETE" to confirm:</p>
                <input type="text" class="form-control" id="deleteConfirmation" />
            </div>
            <div class="modal-footer">
                <button type="button" class="btn btn-secondary" data-bs-dismiss="modal">Cancel</button>
                <button type="button" class="btn btn-danger" id="confirmDeleteAccount" disabled>Delete Account</button>
            </div>
        </div>
    </div>
</div>
_:hammer_and_wrench: Refactor suggestion_ **Enhance user safety and accessibility** The current implementation lacks: 1. Visual warning about the irreversible action 2. Confirmation dialog 3. Accessibility attributes 4. Loading state during deletion Consider these improvements: ```diff <li><hr class="dropdown-divider"></li> -<li><a class="dropdown-item" onclick="deleteAccount();">Delete Account</a></li> +<li> + <button + class="dropdown-item text-danger" + data-bs-toggle="modal" + data-bs-target="#deleteAccountModal" + aria-label="Delete account permanently" + > + Delete Account + </button> +</li> ``` Add this modal for confirmation: ```html <div class="modal fade" id="deleteAccountModal" tabindex="-1" aria-labelledby="deleteAccountModalLabel" aria-hidden="true"> <div class="modal-dialog"> <div class="modal-content"> <div class="modal-header"> <h5 class="modal-title" id="deleteAccountModalLabel">Delete Account</h5> <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button> </div> <div class="modal-body"> <p class="text-danger">Warning: This action cannot be undone. All your data will be permanently deleted.</p> <p>Please type "DELETE" to confirm:</p> <input type="text" class="form-control" id="deleteConfirmation" /> </div> <div class="modal-footer"> <button type="button" class="btn btn-secondary" data-bs-dismiss="modal">Cancel</button> <button type="button" class="btn btn-danger" id="confirmDeleteAccount" disabled>Delete Account</button> </div> </div> </div> </div> ``` <!-- This is an auto-generated comment by CodeRabbit -->
</ul>
</li>
</ul>

9
static/webui/script.js
View File

@ -61,6 +61,15 @@ function logout() {
localStorage.removeItem("password");
}
function deleteAccount() {
if (window.confirm("Are you sure you want to delete your account " + $(".displayname").text() + " (" + localStorage.getItem("email") + ")? This action cannot be undone.")) {
fetch("/custom/deleteAccount?" + window.authz).then(function() {
logout();
single.loadRoute("/webui/"); // Show login screen
});
}
}
if (localStorage.getItem("email") && localStorage.getItem("password")) {
loginFromLocalStorage();
}