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

Company Applications and Approvals #54

Closed
4 tasks done
miguelpduarte opened this issue Nov 26, 2019 · 18 comments · Fixed by #93
Closed
4 tasks done

Company Applications and Approvals #54

miguelpduarte opened this issue Nov 26, 2019 · 18 comments · Fixed by #93
Assignees
Labels
P0 High priority
Milestone

Comments

@miguelpduarte
Copy link
Collaborator

miguelpduarte commented Nov 26, 2019

  • Create CompanyApplication model: Has attributes of Account + Company (for creating them later on if the Application is accepted) + approvalDate + submitDate + rejectDate (the dates - except approval - are null until changed to represent that that event did not yet happen)
  • List CompanyApplications endpoint (admin only)
  • CompanyApplication approval endpoint (admin only) -> Creates the Company and, in case of success, the Account for the Company representative
  • CompanyApplication rejection endpoint (admin only)
@GuilhermeJSilva GuilhermeJSilva self-assigned this Jan 27, 2020
@GuilhermeJSilva
Copy link
Collaborator

GuilhermeJSilva commented Jan 28, 2020

As discussed in person with @imnotteixeira, each CompanyApplication should have :

  • motivation text field
  • rejectReason text field (optional only when the application is pending or accepted)
  • link to an Account (add flag for pending verification accounts to the AccountSchema)
  • comunicationEmail, defaults to Account email
  • link to a Company

A Company should have a flag active (has been approved).
An Account should have a flag verified(is pending approval and can't log in).
When a CompanyApplication is created the linked Account and Company are created with the appropriate flags (Account not verified and Company not active)

On a rejected application the Account is deleted.

@GuilhermeJSilva
Copy link
Collaborator

@miguelpduarte Any thoughts?

@imnotteixeira
Copy link
Collaborator

imnotteixeira commented Jan 28, 2020

Additionally, it would be really helpful to have a virtual field Status which, based on the approvalDate + submitDate + rejectDate, returns a value like APPROVED, PENDING or REJECTED, which is clearer when filtering

More info about virtuals: https://mongoosejs.com/docs/guide.html#virtuals

Regarding Companies, we should create a wrapper to fetch active companies only, since those will be the most requested in our API, and we must be careful to fetch those instead of all in order not to show information about unapproved companies.

@miguelpduarte
Copy link
Collaborator Author

I think that this was already discussed, wasn't this in a photo of the whiteboard somewhere?

Don't feel like we should delete the applications.

Furthermore, wasn't an application supposed to have all the fields for the company? Or was that just for the account? Don't recall which one but this was definitely the case for one of them.

@miguelpduarte
Copy link
Collaborator Author

miguelpduarte commented Jan 28, 2020

Derp, it's in the issue description 🤦‍♂

@miguelpduarte
Copy link
Collaborator Author

Due to us working with mongo, I think it makes more sense to simply have all the data as a sort of blob in the companyapplication and then later on create the company and associated rep's account.

Since references in mongo are weird, don't feel like the suggestion of links to things is the way to go.

@miguelpduarte
Copy link
Collaborator Author

Basically my opinion is to defualt to what was already decided in the issue description... The creation of a company and account and then tagging them with a disabled attribute just seems like unnecessary confusion to me.

Only creating them after approval makes more sense imo.

As such, there would be no trouble only listing the active companies or not allowing login to some accounts. These two models would retain their semantic value with no problem whatsoever.

However, agreed on the virtual field, seems useful. I was thinking of implementing it as a schema function or something like it but wasn't sure of the syntax and would be investigating in the docs, but I think that seems like what we want.

@imnotteixeira
Copy link
Collaborator

imnotteixeira commented Jan 28, 2020

@miguelpduarte after reading your comments, I agree that the Account would not be necessary for the CompanyApplication (@GuilhermeJSilva made me think it would, but I have reconsidered - if you have further arguments, please expose them :P), provided we have a communication email and the Account would only allow the applicant to log in without any benefits of doing so (since the account was pending approval)

That being said, I propose the following:
An application contains:

  • approvalDate + submitDate + rejectDate (the dates - except approval - are null until changed to represent that that event did not yet happen)
  • motivation text field
  • rejectReason text field (optional only when the application is pending or accepted)
  • email
  • companyName

And when the application is approved, a link would be sent to the applicant (something like <domain>/completeRegistration/applicationID + some hash maybe) where they would provide a password, a company bio, and other company-related details we might find useful in the future (maybe company logo?)

This relieves the applicants from the burden of thinking about passwords when they don't actually have an account, and thinking of a bio for the same reasons. For us, it's better as we keep the CompanyApplication to a minimum, while still being able to create Account and Company instances since the user would provide further details once approved.

@miguelpduarte
Copy link
Collaborator Author

I believe the idea at the time was to include the password in the companyapplication as well so that when the application was accepted, the account could be created straight away.

The idea at the time, if I recall correctly, was to reduce implementation complexity, and prevent the potential problems with sending tokens in emails. A simple email saying "Your application has been accepted, please login using the credentials you entered in your application" would suffice.

If there is a concern that the password might be lost between applying and being accepted, then that is a problem solvable with password resetting, and not with changing this procedure, imo.

@imnotteixeira
Copy link
Collaborator

@miguelpduarte that was indeed the way we thought of initially, however, from the user's point of view, it makes no sense to provide a password when there is no account being created directly: it must be approved beforehand. The issue here would not be a lost/forgotten password in my opinion, but my proposal would solve that as well.

Regarding the "token", I don't mean it as a security point of view, it's as simple as a random uuid/whatever that is generated when the application is approved, which is then associated with that application so that no one can redeem an accepted application simply by knowing an applicationID (even though that could suffice). We then send the built link with applicationId+randomstuff to the email and they only have to access it to finalize the process.

In my opinion, this is a better process from a user point of view, as the extra work in the beginning (I don't necessarily mean the password, but company bio+details) might discourage them into applying at all ("oh.. it requires this, but I don't have it right now, I' ll do it later"... Proceeds to forget it and not use nijobs)

Lastly, it does not involve that much more work: sure, it's an additional route, but the logic inside it will be needed anyway for company editing...

@miguelpduarte
Copy link
Collaborator Author

Regarding the "token", I don't mean it as a security point of view, it's as simple as a random uuid/whatever that is generated when the application is approved, which is then associated with that application so that no one can redeem an accepted application simply by knowing an applicationID (even though that could suffice). We then send the built link with applicationId+randomstuff to the email and they only have to access it to finalize the process.

What I meant in terms of security is that some investigation will be necessary to ensure this is done in a secure way, since there are ways of sending secrets via email that are unsafe. (I think generally sending things without a short expiry period via email are bad, and this token would likely not be able to have a short expiry period.)

Other than that agree on the bio + company details. I believe some should be provided, in order to better validate the application. However, as you mention, it makes sense to be able to "create" a "profile" without filling in every bit of data that is possible to insert.

Lastly, it does not involve that much more work: sure, it's an additional route, but the logic inside it will be needed anyway for company editing...

On this, we'll simply disagree. There will be additional investigation necessary in order to ensure that the implementation complies with security standards.

Company editing will be a completely different subject since it will simply validate against a log-in session to see if the permissions match-up. I don't see how that relates to this.

@imnotteixeira
Copy link
Collaborator

What I meant in terms of security is that some investigation will be necessary to ensure this is done in a secure way, since there are ways of sending secrets via email that are unsafe. (I think generally sending things without a short expiry period via email are bad, and this token would likely not be able to have a short expiry period.)

I have a KISS solution for that: since we have an approved_at, why not use that to know when the link was generated, therefore invalidating any attempts to "redeem" the company application that are made at approved_at + APPROVAL_TIMEOUT? I think it would work as intended without having to think much of it.

Company editing will be a completely different subject since it will simply validate against a log-in session to see if the permissions match-up. I don't see how that relates to this.

When I mentioned the logic would be needed I was talking about the more low-level stuff like

  • "setting a password" -> used for reset
  • "setting the bio" -> used for bio editing

Of course, when editing the company details, additional security checks would be necessary, but the underlying functionality is the same, and that's what I was referring to.

@miguelpduarte
Copy link
Collaborator Author

I have a KISS solution for that: since we have an approved_at, why not use that to know when the link was generated, therefore invalidating any attempts to "redeem" the company application that are made at approved_at + APPROVAL_TIMEOUT? I think it would work as intended without having to think much of it.

The problem is not in the timeout, imo. That part seems like simple enough to implement. The problem is with generating secure hashes for sending via e-mail. Ensuring they are not guessable, and just trusting email sending to be secure were the real security vulnerabilities I was looking at here.

When I mentioned the logic would be needed I was talking about the more low-level stuff like

* "setting a password" -> used for reset

* "setting the bio" -> used for bio editing

Of course, when editing the company details, additional security checks would be necessary, but the underlying functionality is the same, and that's what I was referring to.

Sure, but that's so low level that the implementation is trivial. Even using the services structure we are rolling with right now, which could possibly make things "more complicated", it would still be trivial to implement using mongoose models. That is not a problem at all.

My biggest gripe at the moment is with sending an email with secrets in them. I would rather have the password pre-filled in order to not have to worry about that. I know that might seem bad for UX, but a compromise always has to be considered due to several factors, security being one of them.

@imnotteixeira
Copy link
Collaborator

Ok, I am willing to concede on the password thing for the mentioned reasons. In this case the company application will have an email and password, but no Account to prevent candidates to log in (thus, no need for account verification).

Regarding company details, only the name is needed, as the motivation field should provide enough context for admins to approve/reject the application.

@miguelpduarte
Copy link
Collaborator Author

Yup, sounds good! @GuilhermeJSilva do you agree? I think that this will work and be a good compromise between good and easy implementation 😄

@imnotteixeira
Copy link
Collaborator

imnotteixeira commented Jan 29, 2020

To make this clear and to summarize what has been decided, the CompanyApplication schema will be the following:

  • submittedAt (it's not only a date, it's date and time, therefore following this notation)
  • approvedAt
  • rejectedAt (the dates - except submittedAt- are null until changed to represent that that event did not yet happen)
  • motivation text field
  • rejectReason text field (optional only when the application is pending or accepted)
  • email (should not be unique, in order to allow another application with same email after rejection)
  • password
  • companyName

No field is unique except for the default ObjectId

Additionally, there should be a virtual field to extract the status of the application based on the dates.

If approved, the approvedAt should be set and an Account with the provided email and password should be created and associated with a Company (also created then with the provided company name)

If rejected, the rejectedAt should be set and the rejectReason provided

Both events (approval/rejection) should trigger an email regarding the status update.

This proposal requires the following endpoints:

  • GET List CompanyApplications endpoint with (admin only)
  • POST CompanyApplication approval endpoint (admin only) -> Creates the Company and, in case of success, the Account for the Company representative
  • POST CompanyApplication rejection endpoint (admin only)

@GuilhermeJSilva
Copy link
Collaborator

GuilhermeJSilva commented Jan 30, 2020

The model sounds good, yet it originates the need to remove the password hashing from the mongoose hook. With the current implementation the password would be hashed once when the CompanyApplication is created and then again when the respective Account is created. I will move that functionality to a function that must be called before the creation of an Account, if the function is not called the password would be saved in text form and the user will not be able to login.

The endpoint to create an application is missing:

  • Post Create a new CompanyApplication

@imnotteixeira
Copy link
Collaborator

imnotteixeira commented Jul 18, 2020

CompanyApplication creation is done through #67 and #68.

The Review part is done at #74

The features include:

  • Listing company applications (must allow filters, sorting and pagination)

  • Approving applications - sets approvedAt, and activates company account

  • Rejecting applications - sets rejectedAt and rejectReason

  • Hide applications (useful for avoiding spam)- does not delete, but marks them as hidden so that they can't submit another application multiple times with same email
    Moving this one to a separate issue, as it has a lower priority than the others (Hide applications - marks as hidden so that they can't submit another application multiple times with the same email #92)

  • Multiple Approve - approves multiple applications at once

  • Multiple Reject - rejects multiple applications at once
    These last won't be done as they promote careless approval/disapproval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants