Skip to content

Fix password in clear text + extra#101

Merged
sanderhurlen merged 6 commits intodevelopfrom
hotfix/fix-password-in-clear-text
Feb 24, 2021
Merged

Fix password in clear text + extra#101
sanderhurlen merged 6 commits intodevelopfrom
hotfix/fix-password-in-clear-text

Conversation

@sanderhurlen
Copy link
Copy Markdown
Contributor

@sanderhurlen sanderhurlen commented Feb 19, 2021

fixes #100, #96

This PR became a bit bigger than expected:

  • fixes password stored in cleartext
  • adds tests for election organizer, this is by no means full coverage.
  • in order to add tests, election organizer service needed to be rewritten a bit

@sanderhurlen sanderhurlen added bug This is a bug infastructure Code, work, cloud infastructure related priority: high High priority, needs immediate response security Any issue that might violate security labels Feb 19, 2021
@sanderhurlen sanderhurlen added this to the Sprint 7 milestone Feb 19, 2021
@sanderhurlen sanderhurlen self-assigned this Feb 19, 2021
@sanderhurlen sanderhurlen marked this pull request as draft February 19, 2021 14:49
@sanderhurlen sanderhurlen marked this pull request as ready for review February 19, 2021 14:52
@freshfish70 freshfish70 self-requested a review February 20, 2021 18:42
Comment thread tests/services/ElectionOrgService.test.ts Outdated
Co-authored-by: Christoffer A Træen <christoffer.andersen@live.com>
Copy link
Copy Markdown
Contributor

@spiftire spiftire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm but made some suggestions

Comment thread tests/services/ElectionOrgService.test.ts
const passwordPassedIn = '@passwordIsSecret1099'
const rand = Math.random() * 10
beforeAll(async () => {
db = await getTestDatabase()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably use on file for this. I have added a file src/loaders/setupTestDB.ts that basically does the same thing. These files should be merged

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it can all be moved to the tests/Tests.utils.ts

lastName: 'Last name org',
email: rand + 'testing@gmail.com',
password: passwordPassedIn
}) as Promise<ElectionOrganizer>)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this?

Comment on lines +29 to +33
for (const organizer of organizers) {
const org = await service.delete(organizer.id)
}
await db.close()
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (const organizer of organizers) {
const org = await service.delete(organizer.id)
}
await db.close()
})
await clearDatabaseTable()
await db.close()
})
async clearDatabaseTable() {
service.remove(await service.find())
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove() takes an array and removes all entities. and find() returns a list of all entities 🤯

I made a similar function on the feature/default-enums branch. It is generic and only needs the repo to work.

@sanderhurlen
Copy link
Copy Markdown
Contributor Author

Merging this pr as the fixes provided by @spiftire is not ready to be implemented. Issue is created #106

@sanderhurlen sanderhurlen merged commit 49c36bb into develop Feb 24, 2021
@sanderhurlen sanderhurlen deleted the hotfix/fix-password-in-clear-text branch February 24, 2021 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This is a bug infastructure Code, work, cloud infastructure related priority: high High priority, needs immediate response security Any issue that might violate security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Election organizer servie should not return a token on create

4 participants