mimickal / funboxusers Goto Github PK

The session cookie contains the current csrf token. Can we just read this token out of the cookie instead of setting it in a JSON header or hidden form field?

This is probably better done as an admin page, or at least as a better script. The current implementation is really crap and involves plain-text passwords.

Depends on #22

makeUniqueCode checks the database for code uniqueness, but doesn't add the code. We rely on the calling logic to add the code, mostly because we have multiple code types (which we shouldn't).

Once codes are made generic, makeUniqueCode should also add the code to the database when creating it. This means user_id should also become nullable and updated after the fact.

In order to reset a password, we will send the user an email containing a one-time-use code. The backend needs a way to represent the relationship between this code and its use for resetting passwords. We'll probably end up needing to add a "PasswordReset" table for this. The code should expire after some time.

In this task

• Create password reset code
• Associate code with the user it's for (probably DB table)
• Expire the code after a length of time (48 hours?)

Users need to be able to actually log out.

This should probably be its own endpoint that invalidates the user's login session cookie. The browser itself should be instructed to "forget" the session information too. The UI link to do this should probably be on the account overview page.

Use the Err icon. Should probably be a .ico

Currently if one server test fails, every subsequent test automatically fails in the beforeEach step because of a database uniqueness violation. I suspect this is because pocha doesn't run the cleanup step if a test fails.

We can work around this by either making it always run that cleanup step (which it should anyway, this might be a pocha bug), or just dumping the database between runs.

It might make sense to use an in-memory database for testing. That would make it easy to dump everything between tests.

Backend endpoint to allow a logged in user to change their password. This should require both the old password to verify, and the new password to change it to. This should not require an email on the account.

Backend endpoint for triggering a reset of an account password. This is to allow users who are not logged in to ask the server for a password reset link. The link will be sent to them via email (but we'll handle the email portion in another task). Requires a valid user or email to perform this. Should be a POST request.

The backend endpoint that accepts the reset code and serves the reset page. This should be a GET endpoint so it works on-click in browsers. This endpoint should be rate-limited to prevent brute forcing password resets.

• Create GET endpoint
• serve reset page
• rate limit requests

Frontend page for resetting a password. Should have the usual CSRF protection. Simply asks for a new password, with a confirm field. Should also have some javascript making sure the passwords match. On submission, sends the password change request with the one-time-use code to the backend.

Also, visiting account page without active login should redirect you to the login page.
Logging out should redirect to login page.
This should be enforced at the server level, not the client level, since the server can decide to just not serve a page.

Backend endpoint for performing the password reset operation. This should be a PUT request.

Currently, when trying to update or add an email, we attempt to create an entry in the PendingEmail table. However, we do not check if a pending record already exists for the user, so trying to add a second email will lead to a 500 due to a DB uniqueness violation.

We should update the record if it already exists, or create it otherwise (also known as "upsert"). Since this endpoint sends an email with a confirmation code, the code for the pending record will have to change too, so that email link is no longer valid. The old code should be invalidated.

The login cookie should expire after some time. The cookie should also have the various fields (httponly, secure, etc...) set properly.

Backend endpoint for removing an email from an account. Requires the user to be logged in (so should check the session token). Should probably be a DELETE. Should also require CSRF protection.
Depends on #48

When we change email, we send an email to the new address. If the server running fbusers doesn't have a mail server configured, fbusers will encounter an error which propagates to the user as a 500.

We should detect when the mail server isn't configured, print (or log) a warning, then gracefully avoid trying to send emails.

If we want to be really slick, we could also write a "send email" mock that just logs the email to the console instead of sending it when testing manually. We could toggle this on with an environment variable like 'FB_DEV_MODE=Y' or something.

When inserting code association rows, we require passing in the code's string value, not the code object itself. This is counter-intuitive and leads to a lot of subtle, frustrating issues. Can we override the BaseCode class to return the code value by default, kind of like how other objects return the primary key when printing?

Instead of this: LoginCode.create(user=user, code=mycode.code)
I want to do this: LoginCode.create(user=user, code=mycode)

The backend endpoint for retrieving account information. This should return the logged in user's account info (and nobody else's). Probably use the session token for this. Should return data containing:

• username
• email
  • and whether or not it's confirmed

When a user attempts to log in and the server finds an active token (like the situation in #77), maybe we should flash a little message on the login (or account) page. Nothing should functionally change, but it might be valuable for the user to know what happened. We could potentially add something about "Maybe your cleared your cookies?"

Not sure if this is valuable.

I don't want to start refactoring until we have some solid features in place, so for now, this list will just keep track of issues I'd like to see fixed at some point. If any one thing gets too large, I'll create a separate issue for it and link it here.

That means don't work on stuff in this issue until I create a separate issue and assign it to someone.

• Pull things into utility file
  • function for generating a code of arbitrary length (code things handled in #8)
  • server.makeUniqueCode should use this util function Handled, can't remember which issue
  • makeUniqueCode itself should be pulled into the util file too. Same as above
  • server.sendMail should be pulled into the util file (or possibly even an email class. Not sure yet.) Handled in #58
  • yaml config loading pulled out to util file #95
  • password hashing #95
  • email validation #95
    • Also make email validation match front-end, and generally just not be so restrictive (possibly it's own task)
  • verifying login session utility function
    • Also helpers for testing this on every endpoint?
• We have codes for emails, passwords, and now login sessions too. The database should be generalized to handle this without needing to add special code cases all the time. (Handled in #22)
  • Email codes should not store the email in the code itself. Put it somewhere else, like on the user (Will need 'email' and 'email_verified' field added to user) Handled in some issue I forgot
  • Should only have one set of functions for CRUD operations on codes Handled in #34
• Use CSRF token from session instead of sending it as form data (?) (See #15)
• When (if) Pocha ever gets updated to support inherited before/after cases, refactor tests to not repeat the same setup steps all the time
• HTTP responses should be pulled out into their own file (responses.py?) (functions like 'ok' and 'forbidden')
• general project structure should be cleaned up a bit. There's too much crap in the top level. I'm not sure how I want it to look yet, but it is a mess.
  • Move database and secret keys to some root-only directory?
  • Move config to a resources directory?
• When inserting codes, we require adding the code value, not the code object itself. This is counter-intuitive and leads to a lot of subtle, frustrating issues. Can we override the basecode class to return the code value by default, kind of like how other objects return the primary key when printing Handled in #73
• Organize imports better Handled in #89
  • Alphabetical order
  • Separate standard, third party, and local modules
• makeUniqueCode should probably return the actual code object
• Code tests should be generalized so adding a new code type is painless (especially since virtually every code type we add differs in table name only)
• Code.use_code should work at the instance level too, as in mycode.use_code. If this is a pain, maybe just call the instance version mycode.use().
• Probably want to pull endpoints out into constants at some point
• Also want to separate route implementation from route declaration, for easier documentation handled in #111

Right now we hard code 'Funbox' at the top of server.py and use that as the server's name in emails. That should be read in from the config file instead.

Unittest kinda sucks and requires a lot of boilerplate to do basic behavioral tests. The runner and output also suck. Let's move to something cool, like pocha.

As a proof of concept, let's just port one of the simpler test classes. test_add_user.py is a good place to start. Remember to add the pocha test dependency to requirements.txt too.

All requests should happen over https only. Never http.
Also, enable secure for login cookies

We have codes for emails, passwords, and now login sessions too. The database should be generalized to handle this without needing to add special code cases all the time.

Email codes should not store the email in the code itself. Put it somewhere else, like on the user (Will need 'email' and 'email_verified' field added to user)

Should only have one set of functions for CRUD operations on codes

Create the web page to display account info and change account settings. We should probably wait to do this until we have all the backend account endpoints complete.
Also, I'm giving this to myself because I've already almost got this page done.

Page should show:

• Current username
• Current email
• Pending email (if any)

Page should allow:

• Changing password
• Changing email
• Logging out

If a user has successfully logged in and clears their cookies, attempting to log in again causes a 500, preventing the user from logging in again.
peewee.IntegrityError: UNIQUE constraint failed: logincode.user_id

Our current database code isn't very robust.

We extract rows to objects ourselves when the SQLite package provides this functionality.

We also cache the database connection at the module level, which can potentially lead to data corruption (as per @gbMichelle's review comment). We should instead create a connection per application context, as seen here http://flask.pocoo.org/docs/1.0/patterns/sqlite3/

We currently have unit tests that show us that code works as intended when used in the ways we expect. But we don't know what code paths are actually ran.

We should set up a code coverage reporter to see if there is any possible paths that we've missed and to see if there is specific edge cases that we're not testing for.

We have with app.app_context() all over the place for database things right now. We should see if an ORM like SQLAlchemy can reduce some of this noise.

What good is a users system without some way of knowing who is allowed to do what? We should have a database table for permissions, as well as a pivot table for knowing who has those permissions.

The login code should be persisted to the database. This would also allow us to look up the user info from the code alone.

Actually send the user an email containing the password reset link

If a user has an email on file, they should be alerted via that email address when their password is changed.

Page for requesting a password reset for a user. Should accept email or username for the request. On submission always show success even if the user or email doesn't exist. Submit to the password reset trigger endpoint. Login page should link to this page.

Depends on #39

We currently send the email confirmation to the new email address when updating an email. We should also send that same confirmation to the old email before overwriting it, as a security measure. Obviously we won't do this when adding a new email.

There is certainly a better way to ask the system for its hostname. Worst case we can add it to the config, but I would much rather a native function, or at least a shell call to hostname --fqdn

If we get an error that prematurely ends a request handler, any previously created database records currently stay created. This behavior was observed with the change email endpoint, where the server would fail to send an email and send a 500, but would still create the pending email record. Reloading the page and trying to change the email again results in a different 500, this time occurring because of a unique constraint violation in the PendingEmail table. Doing these things in a database transaction would prevent these partial records from being created.

We should log events in the system. For now, let's just log events, including 200, 400, and 500 responses. We should also log error stack traces and the names of the users they occurred for. This will help a lot in manual UI testing as well.

We currently set a placeholder string for the login cookie. Let's generate an actual session cookie and store / return that instead.

Backend endpoint for marking an account as deleted.
We probably want to perform a soft delete here (i.e. mark as deleted, not actually removing the record) to keep reference integrity.

Update: Let's support hard (delete entire row) and soft (mark as deleted, keep row) deletes, with an option for "zero" (zero out other fields before deleting).

We could also maybe support an option for "recursive" in the future that goes through and breaks any references to this account, but that's for another day.

Don't forget to not return soft-deleted accounts (and tests for this functionality, which probably should go under "get account" rather than "delete account").

Will also need to test every endpoint that deals with users, like PasswordReset and EmailConfirm codes.

When testing on our Debian server this test repeatedly fails because the time difference between created_at and the time comparing is consistently above (35 seconds) the default 5 second range for assertDateNearNow.

Verify fancy javascript version login page works and displays properly on the following browsers.

Required:

• Firefox Desktop (current version)
• Chrome Desktop (current version)
• Chromium Desktop (current version)

Nice to haves:

• Firefox Mobile
• Chrome Mobile
• Whatever android's default browser is
• IE (whatever version ships with Windows 7 and up)

Right now getUser endpoint returns the current confirmed email, or the pending email, or none, in that order. A user's original email should not be overridden until the pending email is confirmed. Therefore, this endpoint should return both the current email and the pending email.

Any time a user is retrieved or modified, accessed_at should be updated to datetime.now()

Apparently pyyaml is not a given for every distribution. This causes dependency issues in some cases.

This can be fixed by adding pyyaml to requirements.

Currently the backend endpoint doesn't check for a valid session when updating the email (it checks authorization instead). It should check the session and make sure it's valid. This should also be CSRF protected.
Depends on #48

People shouldn't be able to hammer on the server with failed login attempts, so let's rate limit failed login attempts from a client.

This should probably track something like IP so a bad actor can't lock a user out of their account by spamming invalid logins.

Right now when we ask a user to put in a new password we're only checking if the field exists in the form. We are not however checking if the password strings are empty or if the password matches up to the requirements that the server may have set.

We should create a function to validate passwords based on settings that the admin can configure. And use this whenever a user creates a new password.

Examples would be:

• Min characters.
• Max characters. (Does our database have this for strings? If so, we should add that to our tests and error reporting.)
• Types of required characters, the amount of these required characters that we need.
  • (Think digits 0-9, alphabetic a-z, special characters @#$)
• Forbidden characters?

Flask has some nice built-in things for CSRF. Let's actually use them. https://flask-wtf.readthedocs.io/en/stable/csrf.htmlhttps://flask-wtf.readthedocs.io/en/stable/csrf.html