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

Start enforcing passwords on log in and sign up #2364

Merged
merged 8 commits into from
Jan 16, 2022

Conversation

aapeliv
Copy link
Member

@aapeliv aapeliv commented Jan 15, 2022

Start of the end of magic login links.

  • Enforce password on signup
  • Jail users if they don't have a password, so they're forced to set one the next time they log in
  • Jail bit in web frontend
  • Password field on account form in web frontend

Backend checklist

  • Formatted my code by running autoflake -r -i --remove-all-unused-imports src && isort . && black . in app/backend
  • Added tests for any new code or added a regression test if fixing a bug
  • All tests pass
  • Run the backend locally and it works
  • Added migrations if there are any database changes, rebased onto develop if necessary for linear migration history

Web frontend checklist

  • Formatted my code with yarn format && yarn lint --fix
  • There are no warnings from yarn lint
  • There are no console warnings when running the app
  • Added any new components to storybook
  • Added tests where relevant
  • All tests pass
  • Clicked around my changes running locally and it works
  • Checked Desktop, Mobile and Tablet screen sizes

@aapeliv aapeliv changed the title Jail users if they don't have a password Start enforcing passwords on log in and sign up Jan 15, 2022
@aapeliv aapeliv requested review from kalvdans and a team January 15, 2022 20:35
@aapeliv aapeliv force-pushed the backend/improvement/require-passwords branch from ae3de03 to 0601159 Compare January 16, 2022 08:58
Copy link
Member

@darrenvong darrenvong left a comment

Choose a reason for hiding this comment

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

Only some minor things, but frontend mostly looks fine

app/web/features/auth/jail/PasswordSection.tsx Outdated Show resolved Hide resolved
app/web/features/auth/jail/PasswordSection.tsx Outdated Show resolved Hide resolved
app/web/features/auth/jail/PasswordSection.tsx Outdated Show resolved Hide resolved
app/web/features/auth/jail/PasswordSection.tsx Outdated Show resolved Hide resolved
app/web/features/auth/jail/PasswordSection.tsx Outdated Show resolved Hide resolved

message SetPasswordReq {
// the frontend should ask for the password twice and whatnot
string new_password = 1 [ (sensitive) = true ];
Copy link
Member

@darrenvong darrenvong Jan 16, 2022

Choose a reason for hiding this comment

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

What does grpc do to sensitive fields?

(Obviously unrelated to the frontend review, mainly curious about this)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used in the backend to clear those fields before they're stored in to a debugging table:

def _sanitized_bytes(self, proto):
"""
Remove fields marked sensitive and return serialized bytes
"""
if not proto:
return None
new_proto = deepcopy(proto)
for name, descriptor in new_proto.DESCRIPTOR.fields_by_name.items():
if descriptor.GetOptions().Extensions[annotations_pb2.sensitive]:
new_proto.ClearField(name)
return new_proto.SerializeToString()

Copy link
Member

@kalvdans kalvdans left a comment

Choose a reason for hiding this comment

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

Backend looks fine, two minor comments

app/backend/src/tests/test_auth.py Outdated Show resolved Hide resolved
app/backend/src/couchers/servicers/auth.py Show resolved Hide resolved
@aapeliv aapeliv force-pushed the backend/improvement/require-passwords branch from 89e9d35 to 0415a49 Compare January 16, 2022 18:29
@aapeliv aapeliv merged commit 0615fc3 into develop Jan 16, 2022
@aapeliv aapeliv deleted the backend/improvement/require-passwords branch January 16, 2022 18:54
@aapeliv aapeliv added release notes: pending Add to stuff that should be included in release notes release notes: done Was mentioned in release notes labels Jan 16, 2022
@aapeliv aapeliv removed the release notes: pending Add to stuff that should be included in release notes label Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: done Was mentioned in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants