🙋🏽‍♀️ PROPOSAL: Volunteer route changes

[astroash]

Code

https://github.com/TwinePlatform/twine-monolith/blob/5d7de84d81bd24b452a691d546c15df2dee3b7b4/api/src/api/v1/users/register/volunteers.ts#L35-L48

Why

The new volunteer app is changing the way admin interact with volunteers passwords. It will no longer require admins to enter a password to create a new volunteer, instead it will send an email to the new volunteer for them to set their own password.

This will ensure two personal security issues:

1. that new volunteers are not giving their password to admins to create an account
2. that new volunteers are notified if someone has created an account on their behalf

What

Suggest on `/users/register/volunteers/

• make password optional
• add payload value sendPasswordEmail: boolean
• add check to ensure that if password is not present, sendPasswordEmail is truthy

[eliasmalik]

add payload value sendPasswordEmail: boolean

Is this necessary? Can't we infer this from the role of the user that sends the request? If the user is authenticated (and if they are they would have to be an admin) then send the e-mail, if not, don't send it and reject the request if the password isn't there.

Are there any backwards compatibility issues here?

[astroash]

Can't we infer this from the role of the user that sends the request?

I'm not a fan of code inference, I prefer paths through our apps to be explicit. It means that we are less likely to fall into an edge case where this path is executed., and makes future maintainability easier.

eg. for this password reset email to be send it needs to be explicitly requested via the payload value. If in one of our apps a bug happens to pass that means a password is not sent or is accidentally nullified, then we will not fall into this path, instead we will get an error response saying "password has not been supplied. If this is expected behaviour please set the sendPasswordEmail value in the payload."

Are there any backwards compatibility issues here?

I think this is a concern if we are not explicit

[eliasmalik]

I'm not a fan of code inference, I prefer paths through our apps to be explicit. It means that we are less likely to fall into an edge case where this path is executed., and makes future maintainability easier.

It will be explicit -- in the handler. The problem is actually the opposite: the more leeway we give to the clients in how they interact with the API, the more likely it is that they'll specify a combination of inputs that are unanticipated or lead to an unconsidered code path. Typically the smaller the surface area of the API the fewer paths we need to consider.

What we want is:

• users registered by an admin are created without a password
• users registered by an admin are sent a password reset email
• users registering themselves are required to submit a password
• users registering themselves are not sent a password reset email

We'd need to do role/auth checks anyway since any client could send that request, so we only really end up with more code if we went with the additional payload parameter.

eg. for this password reset email to be send it needs to be explicitly requested via the payload value. If in one of our apps a bug happens to pass that means a password is not sent or is accidentally nullified, then we will not fall into this path, instead we will get an error response saying "password has not been supplied. If this is expected behaviour please set the sendPasswordEmail value in the payload."

It won't fall into the path anyway if the handler is written correctly. The logic is pretty simple.

If the request is unauthenticated:

• password is null:
  • 400
• password is not null:
  • register user
  • no pwd reset email sent

If the request is authenticated:

• role !== CB_ADMIN || VOLUNTEER_ADMIN
  • 403
• password is not null
  • 400 (or alternatively simply ignore the value)
• password is null
  • register user
  • send e-mail

[eliasmalik]

"password has not been supplied. If this is expected behaviour please set the sendPasswordEmail value in the payload."

Also it's usually far outside the client's wheelhouse to be specifying what side effects should happen when an endpoint is hit. All it cares about is creating a new user. It doesn't and shouldn't care about what e-mails are or are not sent, or any other auxiliary actions that the API might take now or in the future. At most, the API may tell the client what has happened (if that is relevant and should be displayed to the user).

[astroash]

It seems over complicated to perform different actions based on role credentials.

If we wanted to simply the api interface we can remove my 3rd bullet point

• make password optional
• add payload value sendPasswordEmail: boolean
• add check to ensure that if password is not present, sendPasswordEmail is truthy

Then the client can decide if they want a password reset email or not.

[astroash]

Also it's usually far outside the client's wheelhouse

Fair, I was just spewing out an example response.

[eliasmalik]

It seems over complicated to perform different actions based on role credentials.

Why? (I mean technically we should be checking permissions rather than roles -- that was the whole point of separating them -- but put that aside for a second)

Then the client can decide if they want a password reset email or not.

My point is that it shouldn't be up to the client.

[astroash]

Our register endpoints are open endpoints, you are suggesting adding very specific boolean logic based on credentials simply to serve the current use case of one of our clients.

What happens if we have another client in the future that allows the creation of volunteers? The will be tied to the specific use case of the current volunteer app.

I think keeping these endpoints as general and client agnostic as possible allows for better future proofing and allows them to be configured for any other needs that may occur.

[eliasmalik]

What happens if we have another client in the future that allows the creation of volunteers? The will be tied to the specific use case of the current volunteer app.

It's not clear to me what situation this might be, can you be a little more concrete? We have the client asking for the creation of a new resource (a user). The API creates that resource. The API performs some other side effects based on the nature of the request. In what situation would we not be able to infer from the existing request (without additional flags in the payload) which side effects should occur?

I think keeping these endpoints as general and client agnostic as possible

They are client agnostic, they are not permission agnostic. We are not changing behaviour based on the origin of the request, or the user agent.

allows for better future proofing and allows them to be configured for any other needs that may occur.

I agree, that's why having this logic in the backend allows us to make business and application level logic changes without needing to push updates to the clients.

[astroash]

It's not clear to me what situation this might be, can you be a little more concrete?

• the admin app may want to be able to create new volunteers
• the dashboard may want to be able to create new volunteers
• we may want to expose an api endpoint like we have for on of the visitor app users

Each of these examples would have a different set of permissions tied to the request. As an open endpoint I do not think that it's functionality should change based on permissions.

[eliasmalik]

the admin app may want to be able to create new volunteers

We would check for the presence of user_details-child:write (which TWINE_ADMIN has)

the dashboard may want to be able to create new volunteers

We would check for the presence of user_details-child:write (which CB_ADMIN has)

we may want to expose an api endpoint like we have for on of the visitor app users

The permissions system for 3rd-party access is underdeveloped, specifically in the sense that we can currently only associate a single permission string to each API token. If we're going to make 3rd party access more widespread and in particular offer write access, we'll need to address this first.

After which point, we can simply assign the user_details-child:write credential to the API token in question, and this case becomes like those above.

As an open endpoint

We would change the auth mode to "try" -- hapi would attempt to authenticate the request, but if it fails, it would continue anyway. The endpoint would be, in a sense, "half open".

[astroash]

This seems like an overcomplicated solution to something can can be specified in the payload.

[eliasmalik]

My suggestion would effectively add a single conditional statement to the handler. In what sense is that more complicated than an additional payload parameter (which would also result in a conditional statement)?

And regardless, we shouldn't be giving clients control over what auxiliary effects happen on the server merely via payload params, especially on endpoints that anyone can access. This is basically like leaking implementation details from a function to a caller.

[astroash]

My suggestion would effectively add a single conditional statement to the handler

Okay, I got the impression that this would be more complicated than that. I'll have a go implementing like this and see how it goes.