Add ability for user to delete themselves if permission present by jescalan · Pull Request #1307 · clerk/javascript

jescalan · 2023-06-06T17:37:28Z

Type of change

Packages affected

Description

When permissions are present, users will see a section in their settings allowing them to delete their account. User deletion permissions are controlled via the Clerk dashboard.
A createOrganizationEnabled property was also added to users in anticipation of a feature allowing permission-gated org creation that should be landing in the next couple days.
When a user's account is deleted, they are signed out and nav to the home page.

changeset-bot · 2023-06-06T17:37:31Z

🦋 Changeset detected

Latest commit: 5570dc8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages

Name	Type
@clerk/localizations	Minor
@clerk/clerk-js	Minor
@clerk/types	Minor
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
gatsby-plugin-clerk	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jit-ci · 2023-06-06T17:37:34Z

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

jescalan · 2023-06-06T17:38:46Z

I don't think we need a changeset for this alone, but I can continue to actually implement the effects of these properties on this PR if desired, and that would definitely merit one.

jit-ci

✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆

jescalan · 2023-06-07T21:28:53Z

Sign in to view

  membersCount: number;
  pendingInvitationsCount: number;
  publicMetadata: OrganizationPublicMetadata;
+  adminDeleteEnabled: boolean;


🤦‍♂️ pushed before I hit save on the file - thank you! just added

jescalan

Self-review as this is my first PR to Clerk!

jescalan · 2023-06-13T20:46:25Z

    "test": "jest",
-    "test:coverage": "jest --collectCoverage && open coverage/lcov-report/index.html"
+    "test:coverage": "jest --collectCoverage && open coverage/lcov-report/index.html",
+    "watch": "webpack --config webpack.config.js --env production --watch"


Added this command to pair with the playground file I added here while developing the feature. When running this in the terminal, it will rebuild changed files more quickly than running a full build after every save.

Makes sense to me. Just curious, does HMR work correctly when using playground.html? I think HMR won't work unless the file is served under https as HMR relies on webhooks.

It does not, requires a manual reload. Maybe we can tinker with this to make it smoother down the line!

jescalan · 2023-06-13T20:47:37Z

@@ -0,0 +1,170 @@
+<!DOCTYPE html>


This playground file is something I built alongside the feature to help me to develop it. This requires little to no setup - you just add your API key, open the file, and its off to the races. I also feel like having a pure js testbed for things is helpful in general - I turned up a number of bugs that I bet are not present in higher abstractions like react & nextjs libs.

This is great, I wonder if we should move this inside the top-level playground dir instead

Nice one. Jeff, do you think we can move it under playground as Pantelis suggested? The playground directory hosts other example apps. Not related to this PR but it might be a good idea to do a few cleanups such as:

move the examples from packages/sdk-node/examples into playground or remove them completely

remove any playground apps that we don't use often

automatically deploy some playground apps to Vercel on every staging release (merge into main) as we do for Accounts.

align the interface of all package.json files (this is something I'm currently working on)

Hope the extra context is useful to you. I'm very interested to hear your thoughts and more ideas about the playgrounds apps :)

jescalan · 2023-06-13T20:49:21Z

    this.publicMetadata = data.public_metadata;
    this.membersCount = data.members_count;
    this.pendingInvitationsCount = data.pending_invitations_count;
+    this.adminDeleteEnabled = data.admin_delete_enabled;


I kind of snuck this addition in here - the original PR was just too add these new properties as they were finished in the backend, but it expanded to covering the user delete feature. We decided to move the other two paired features, allowing admins to delete organizations, and gating organization creation permissions for users, to separate PRs, which I will be submitting soon.

If you want, I can move these out to the other PRs to keep the history squeaky clean, just let me know.

We can leave it as is. No need for extra work.

jescalan · 2023-06-13T20:49:42Z

  publicMetadata: UserPublicMetadata = {};
  unsafeMetadata: UserUnsafeMetadata = {};
  lastSignInAt: Date | null = null;
+  createOrganizationEnabled: boolean = false;


Same here with the leaked properties note above

❓ Are these named based on the FAPI payload? Another naming that'd make sense to me would be canCreateOrganization but I'm fine with createOrganizationEnabled as well. Same applies to the deleteSelfEnabled prop as well (deleteSelfEnabled -> canDeleteSelf)

This is an extremely nitpicky comment, please feel free to completely ignore :)

Yes, it is based on FAPI. We iterated a lot on naming these parameters. I can't recall why we didn't pick the canX proposal that was discussed at some point.

@mzhong9723 @jescalan Do you maybe remember?

@nikosdouvlis FYI this is the design doc. You can also check the comments section for naming decisions.

I poked into this more last night and I don't think these properties are actually needed at all on the User/Org resources directly. They are accessed through UserSettings and OrganizationSettings on the actions sub-property, as it is returned from bapi, and currently the properties are here but they are not actually populated or used, so I'm going to remove them I think.

as far as canX, I'm not sure to be honest, I just mirrored the property name from bapi for consistency

jescalan · 2023-06-13T20:50:04Z

    this.backupCodeEnabled = data.backup_code_enabled;
    this.twoFactorEnabled = data.two_factor_enabled;

+    this.createOrganizationEnabled = data.create_organization_enabled;


...and here

jescalan · 2023-06-13T20:51:14Z

    this.attributes = Object.fromEntries(
      Object.entries(data.attributes).map(a => [a[0], { ...a[1], name: a[0] }]),
    ) as Attributes;
+    this.actions = data.actions;


This small modification allows the environment context to have access to this new "actions" property which contains the values for permissions associated with user actions, which are needed for the user delete feature. Big thanks for @SokratisVidros for helping me to find this and patch this value into the right place.

jescalan · 2023-06-13T20:55:32Z

+      await clerk.signOut({ sessionId: session.id });
+      router.navigate(environment.displayConfig.homeUrl);
+      // TODO: for routerless mode, explicitly open the "choose" route
+      clerk.openSignIn();


Want to add a little extra context here - after deleting a user, you get signed out, and the router will navigate to the home route, which should contain the sign in UI, in theory. If you have a multi-session app, the sign in UI should allow the user to select the next available session data to log into.

However, if the implementation is just using pure JS and is not associated with a router, the navigation will do nothing other than changing the url hash, so we explicitly open the sign in window. This is a slight bug IMO for router-less apps, since we want to show the "choose" route within the sign in UI to allow to user to choose other sessions if there are any. If you refresh the page, it will automatically log into any other available sessions for a multi-session app, but a refresh shouldn't be necessary.

Is this fix targeted for multi-session or single-session apps? The sign-in choose screen is only available in multi-session apps.

This behavior is opinionated when it comes to UX. ClerkJS supports afterSignOut props that should also be used in this case as they are the main control for Clerk users to handle post sign-out behavior.

Most importantly, I'd also that FAPI signs out the user during await user.delete. So, the clerk.signOut shouldn't be necessary as the payload of user.delete should purge the current session, and should unmount. Let me double-check that.

jescalan · 2023-06-13T20:56:25Z

+        deleteAccountButton: 'Delete Account',
+        deleteAccountTitle: 'Delete Account',
+        deleteAccountDescription: 'Delete your account and all its associated data',
+      },


Just as a note, I only ran the english translation here. I can probably cover a couple more but I'm not sure how we normally tackle translation.

😀 We officially support only the english translation. It's a personal decision if you want to do the rest as well.

As Pantelis said, only the en-US localization is officially supported. Localizations that don't have these values set will simply fallback to the en-US one.

SokratisVidros · 2023-06-15T12:34:21Z

    this.publicMetadata = data.public_metadata;
    this.membersCount = data.members_count;
    this.pendingInvitationsCount = data.pending_invitations_count;
+    this.adminDeleteEnabled = data.admin_delete_enabled;


We can leave it as is. No need for extra work.

SokratisVidros · 2023-06-15T12:46:22Z

+      await clerk.signOut({ sessionId: session.id });
+      router.navigate(environment.displayConfig.homeUrl);
+      // TODO: for routerless mode, explicitly open the "choose" route
+      clerk.openSignIn();


Is this fix targeted for multi-session or single-session apps? The sign-in choose screen is only available in multi-session apps.

This behavior is opinionated when it comes to UX. ClerkJS supports afterSignOut props that should also be used in this case as they are the main control for Clerk users to handle post sign-out behavior.

Most importantly, I'd also that FAPI signs out the user during await user.delete. So, the clerk.signOut shouldn't be necessary as the payload of user.delete should purge the current session, and should unmount. Let me double-check that.

SokratisVidros · 2023-06-15T12:46:37Z

+  };
+
+  return (
+    <>


❓ Is this fragment required?

Ah, no it isn't - good catch. This came from me removing the wizard initially, but also ended up removing the success page state then it was no longer needed. Will clean up now.

jescalan · 2023-06-15T17:18:47Z

@SokratisVidros removed the unneeded fragment, tested whether user.delete() will kill the session and it does, so removed clerk log out line!

SokratisVidros

Nicely done @jescalan.

One last comment about the router.navigate and it's good to go!

…roperties to User

- When permissions are present, users will see a section in their settings allowing them to delete their account. - `createOrganizationEnabled` property was also added to users in anticipation of a feature allowing permission-gated org creation that should be landing in the next couple days - When a user's account is deleted, they are signed out and nav to the home page.

Co-authored-by: Sokratis Vidros <SokratisVidros@users.noreply.github.com>

…urces

jescalan · 2023-06-16T14:12:50Z

  };

+  delete = (): Promise<void> => {
+    return this._baseDelete({ path: '/me' });


Just want to flag this, as it's a somewhat non-standard path. Once we fully roll this out in the API there's no going back without a major version boost.

…Url implementation

…k#1307) feat(clerk-js): Add delete self feature for users - When permissions are present, users will see a section in their settings allowing them to delete their account. - `createOrganizationEnabled` property was also added to users in anticipation of a feature allowing permission-gated org creation that should be landing in the next couple days. - When a user's account is deleted, they are signed out and nav to the home page.

clerk-cookie · 2024-06-16T00:01:30Z

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jescalan requested review from SokratisVidros and nikosdouvlis as code owners June 6, 2023 17:37

github-actions Bot added clerk-js types labels Jun 6, 2023

jit-ci Bot reviewed Jun 6, 2023

View reviewed changes

SokratisVidros reviewed Jun 7, 2023

View reviewed changes

SokratisVidros approved these changes Jun 7, 2023

View reviewed changes

SokratisVidros force-pushed the je.add-new-properties-to-user branch from a1cb208 to fc388c9 Compare June 9, 2023 20:52

github-actions Bot added the localizations label Jun 9, 2023

jescalan changed the title ~~Add createOrganizationEnabled and deleteSelfEnabled props to User~~ Add ability for user to delete themselves if permission present Jun 13, 2023

jescalan force-pushed the je.add-new-properties-to-user branch from 128d75e to 94891e6 Compare June 13, 2023 20:44

jescalan requested a review from SokratisVidros June 13, 2023 20:45

jescalan commented Jun 13, 2023

View reviewed changes

jescalan force-pushed the je.add-new-properties-to-user branch from bb957fa to 6ff753f Compare June 14, 2023 02:00

SokratisVidros suggested changes Jun 15, 2023

View reviewed changes

nikosdouvlis approved these changes Jun 15, 2023

View reviewed changes

jescalan requested a review from SokratisVidros June 15, 2023 17:17

jescalan force-pushed the je.add-new-properties-to-user branch from c03f173 to 3485e0f Compare June 15, 2023 21:10

SokratisVidros approved these changes Jun 16, 2023

View reviewed changes

Comment thread packages/clerk-js/src/ui/components/UserProfile/DeletePage.tsx Outdated

jescalan added 6 commits June 16, 2023 10:03

feat(clerk-js): Add createOrganizationEnabled and deleteSelfEnabled p…

9bbc084

…roperties to User

fix(clerk-js): Cleanup

2e1de91

fix(clerk-js): Add changeset

4b57f0b

fix(clerk-js): Clean up unused import

f6a2f81

fix(clerk-js): More cleanup to fix tests

9f5272d

jescalan and others added 4 commits June 16, 2023 10:03

fix(clerk-js): More test fixes

2b40775

fix(clerk-js): Remove sign out and show signIn on delete page

c4c3206

fix(clerk-js:) Small fix for async

6d26a5e

Co-authored-by: Sokratis Vidros <SokratisVidros@users.noreply.github.com>

fix(clerk-js): Remove unused properties on User and Organization reso…

2c259d4

…urces

jescalan force-pushed the je.add-new-properties-to-user branch from cc10097 to 2c259d4 Compare June 16, 2023 14:08

fix(clerk-js): Move playground into the right folder

6b25c40

clerk-cookie added the playground label Jun 16, 2023

jescalan commented Jun 16, 2023

View reviewed changes

jescalan added 2 commits June 16, 2023 10:21

fix(clerk-js): Remove unused typings from json type, fix afterSignOut…

3eeb5bd

…Url implementation

fix(clerk-js): Fix redirect logic for multi-session apps

5570dc8

jescalan merged commit 4e1bb2b into main Jun 16, 2023

jescalan deleted the je.add-new-properties-to-user branch June 16, 2023 17:38

clerk-cookie mentioned this pull request Jun 16, 2023

Version Packages #1369

Merged

jescalan mentioned this pull request Jun 16, 2023

User delete self fixes #1372

Merged

20 tasks

clerk locked as resolved and limited conversation to collaborators Jun 16, 2024

Conversation

jescalan commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Packages affected

Description

Uh oh!

changeset-bot Bot commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

jit-ci Bot commented Jun 6, 2023

Uh oh!

jescalan commented Jun 6, 2023

Uh oh!

jit-ci Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

jescalan Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jescalan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikosdouvlis Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jescalan commented Jun 15, 2023

Uh oh!

SokratisVidros left a comment

jescalan commented Jun 6, 2023 •

edited

Loading

changeset-bot Bot commented Jun 6, 2023 •

edited

Loading

jescalan Jun 7, 2023 •

edited

Loading

nikosdouvlis Jun 15, 2023 •

edited

Loading