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

Exercise submission #4

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

falexandrou
Copy link

@falexandrou falexandrou commented Nov 28, 2022

What I've done

  • Added Dockerfile and docker-compose files to help with development (The schema is auto-imported when the database container gets created)
  • Sanitize and validate the users data on the model
  • Added the ability to render JSON in the app
  • Installed the latest Bootstrap & jQuery versions
  • Added Server Side Validation
  • Prevent users from having duplicate email addresses
  • Validates and stores users
  • Edits and updates users
  • Deletes users

Trade-offs

  • The forms contain a novalidate attribute to showcase the server-side validation.
  • I tried to stay within the 2-4 hour timeframe so I refrained from installing React / Webpack etc that would eat up time. I kept it really simple with jQuery

What I would do differently in real-life

  • I would use a JS framework
  • I would provide a test suite
  • I would also focus on security with SSL on the server, authentication, csrf tokens etc
  • Finally, I would build some kind of automation around CI/CD which I avoided this time

Demo URL

http://178.62.230.167/ running on DigitalOcean

@falexandrou falexandrou marked this pull request as ready for review November 28, 2022 14:59
Comment on lines 3 to 6
if (
!isset($_SERVER['HTTP_X_REQUESTED_WITH'])
|| strtolower($_SERVER['HTTP_X_REQUESTED_WITH']) !== 'xmlhttprequest'
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add validation of a CSRF token?

Copy link
Author

Choose a reason for hiding this comment

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

As mentioned in the PR comment, I didn't do anything security related (that I would normally do in real life). Happy to add it at a later stage if you like

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

Successfully merging this pull request may close these issues.

2 participants