Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send a confirmation email before deleting company #184

Open
imnotteixeira opened this issue Sep 4, 2021 · 3 comments
Open

Send a confirmation email before deleting company #184

imnotteixeira opened this issue Sep 4, 2021 · 3 comments
Assignees
Labels
enhancement New feature or request P1 Medium Priority

Comments

@imnotteixeira
Copy link
Collaborator

Currently, after #178, companies (or gods) can delete a company by calling the respective endpoint, which will trigger the deletion of the offers, the company, and the associated account.

As this is really destructive, this endpoint should only send a confirmation email to the respective company with a link to execute the deletion, should they really want to do it.

The link would be to a new endpoint (which can be called at /:companyId/delete, and we can change the current one to /:companyId/requestDeletion to make it more accurate).

This new endpoint would have an associated "code", containing all the relevant data, in a secure/ non-easily-reproducible way. Of the top of my head, I'm thinking we can have a "secure string" in .env (similar to the one we use for session cookies - SESSION_SECRET) which would be our symmetric key used both for encryption/decryption.

The data (to be encrypted by that key) should contain all the relevant parameters so that the endpoint knows what to delete (maybe only the companyId is enough, maybe you want to add more things to make it faster - accountID?), however you can also add a timestamp, so that if more than X time has passed, the endpoint won't do anything, again, for safety. Then you should JSON.stringfy that data and encrypt it, which will give you a nice "random" string (generated in the /requestDeletion endpoint, which will be a parameter of the /delete endpoint. That endpoint will use the same key to decrypt the data and execute the deletion.

Here you can see more or less how this would work in terms of encryption/decryption: https://gist.github.com/ccorcos/1a26653ac5b59cdc3a59 (it's a bit outdated, so you may need to research if nodejs has updated these functions), just search for symmetric encryption with nodejs

@joaonmatos
Copy link

Hey, I don't think this is really a matter of encryption here.

You really only need to prove the user has access to the side channel, so the solutions usually would be:

  • The most common is the stateful approach, which it seems you want to avoid. May I ask why? It would only involve storing a nonce in a collection with the company id, requester user ID, and time of generation, and then when you call the deletion endpoint you issue a lookup for the nonce, which would have been sent to the side-channel.
  • If you want to make it stateless, you just need to guarantee that the token is authentic. In that case you only need to issue a JWT-type signed token with the company id, request user ID, and issue time and send it to the side-channel.

A detail that might seem weird to you here is that I suggest storing the user id of who requested the deletion. That's because I think you should re-check the authorization at the time of performing the actual deletion.

Finally, my two cents are that the platform admins (gods, right?) should probably still have access to a direct deletion endpoint, or at least a bulk deletion endpoint, because you might need to perform bulk deletions for moderation purposes.

@imnotteixeira
Copy link
Collaborator Author

imnotteixeira commented Sep 4, 2021

Hey, answering the question: I don't totally oppose the stateful approach (i.e., using the DB to store token -> deletion request info), however, I have two arguments towards the stateless approach:

  • Considering this project will be using free database instances (I'm assuming for quite a while, if not forever, you know how it works...), anything that can be stateless is probably safer.
  • One of the goals of NIJobs is to allow the contributors to learn new things (and how to do them the right-est way). With that in mind, it's probably more profitable to implement something that is new to many (while being common in the industry - tokens and encryption are everywhere - and satisfying the requirements)

From what I know of JWT, that is pretty similar to the solution I described: a set of data in an object, that will be signed and converted to a string. The only difference being that in JWT the data is signed but not encrypted (at least by default, we can always encrypt the data before putting it in the JWT). Since we shall not be sending any private information in the token (companyIds and accountIds may be public), a JWT token would work here.

However, I have one question: when you mention issue a JWT-type signed token with the company id, request user ID, and issue time, you mean to put that information on the payload, not using it as the signature right? Otherwise, there wouldn't be a way of knowing what the signature was when issuing the JWT


Finally, regarding other endpoints/features, that should be discussed in another issue for that matter (I think regardless of the "requester", there should always be a confirmation step, due to the nature of the request. regarding bulk operations, that's something to think in the future maybe)

@joaonmatos
Copy link

joaonmatos commented Sep 6, 2021

Yes, those fields would make up the payload. I want to put the emphasis on the jwt-like in my other comment. JWT is a specific format for authentication/authorization that needs to support a lot of stuff like different encryption schemes and what not. It would probably be enough to issue a token like:

const token = {
    version: "1.0.0",
    payload: {
        companyId: "wtv",
        requesterId: "wtv",
        issueTime: "2021-09-06T07:29:50.000Z", // or epoch-based timestamp
        // optionally a nonce here too
    },
    signature: "abcxyz this would be some sort of signature of the payload with an asymmetrical key"
}

Then you can generate the URL either by base 64 and url-encoding the entire token or simply by generating a query string (won't commit to a format suggestion here).

Either way, the point here is that you share with the authentic user a piece of data that only you can access: on the db-backed case, you're the only one who knows which IDs have been stored and the user is the only one who knows their key to that collection, and on the token-based solution, only you have the private key and only the user knows what would be the signature for their specific token.

Now, I still think this token based approach, even when simplifying the most we can while keeping the properties we need, turns out over-engineered vs storing an extra kilobyte for each deletion request. I also wanted to push back and say that I doubt that for this specific use case industry uses more cryptographic keys than db-backed keys, since as soon as you start needing to account for other concerns, like rate-limiting these kinds of destructive requests, you will probably need to store it somewhere either way.

Now, I am not planning on implementing this regardless of the chose architecture, so it is not my choice to make, but I just want to make sure the decisions taken are well thought out and the simplest they can be. Engineering is more than cleverness and premature optimisation, after all! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 Medium Priority
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants