-
-
Notifications
You must be signed in to change notification settings - Fork 50
Support user login and registration #495
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
base: oauth-phase-2
Are you sure you want to change the base?
Conversation
Test Results236 tests +37 231 ✅ +37 48s ⏱️ +21s Results for commit da8e04c. ± Comparison against base commit 4003c6b. This pull request removes 24 and adds 61 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Add a moderation log to keep track of decisions as well.
It was duplicated and unused on two pages
and add a button to show/hide the password
+ automatic formating
Fix form validation and UI hints, rework the UI to be more user-friendly, cleanup markup, make it more mobile friendly
same as other pages, big call-to-action button as in the design files
We don't want end-users to create accounts directly on MeB.org, but it might just happen, so adding a link to the signup page they would be looking for.
Otherwise the entire test suite fails
…rotection to admin forms, and fixed moderation commit handling
mayhem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very cursory scan, but looks good from that distance. I think we should make a great effort testing this codebase and be really picky about ongoing PRs. We'll need lots of eyes on the new site.
| @@ -1,5 +1,7 @@ | |||
| BEGIN; | |||
|
|
|||
| -- TODO: Add some, if needed. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup?
|
|
||
|
|
||
| class MeBFlaskForm(FlaskForm): | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docstrings.
| from brainzutils.mail import send_mail | ||
| from metabrainz.model.user import User | ||
|
|
||
| VERIFY_EMAIL = "verify-email" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are oddly named. What do they do?
| EMAIL_VERIFICATION_EXPIRY = timedelta(hours=24) | ||
| EMAIL_RESET_PASSWORD_EXPIRY = timedelta(hours=24) | ||
|
|
||
| # Bcrypt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these config items? Some explanation or a link to possible values would be good.
|
One request: The admin interface was always ugly and now its gotten even worse. :( No need to design a masterpiece, but it would be nice to make some improvements so we dont have giant black bars and then like. :) |
| response.headers["Pragma"] = "no-cache" | ||
| response.headers["X-Frame-Options"] = "DENY" | ||
| response.headers["Referrer-Policy"] = "no-referrer" | ||
| # todo: add content-security-policy headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps while working with this file, it may be worthwhile to resolve this todo.
For example, knowing that X-Frame-Options: DENY is in place (i.e., this is not to be displayed in a frame anywhere), and assuming that this is only meant to be shown on meb.o (or self-hosted equivalents) and possibly draw from static media, we could use:
Content-Security-Policy: frame-ancestors 'none'; frame-src 'none'; object-src 'none'; img-src 'self' https://staticbrainz.org/
...but that's an approximation. (ref: MDN Content-Security-Policy documentation)
| def after_oauth2_request(response): | ||
| """ Add security headers for Referrer-Policy, Content-Security-Policy, Cache-Control and X-Frame-Options """ | ||
| response.headers["Cache-Control"] = "no-store" | ||
| response.headers["Pragma"] = "no-cache" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you expect relevant endpoints to be called by HTTP/1.0 clients? If not, we should consider removing this header, as it is deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since Cache-Control: no-store per the preceding line, I believe this setting is also kind of ignored since no-store seems to be the most restrictive directive among possible Cache-Control values; see MDN Pragma, Cache-Control pages.
| - "8150" | ||
|
|
||
| meb_db: | ||
| image: postgres:12.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change would be out of scope for this specific PR, but even though this Compose file is for a dev environment, I still feel obliged to point out that Postgres 12 is well past end-of-life. 13 goes EoL in a few days as well, so 14 should be the minimum at this point.




TBA