8000 🐛 Bug Report: Error 500 using `updateEmail` when password is not set cause OAuth2 · Issue #4975 · appwrite/appwrite · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

🐛 Bug Report: Error 500 using updateEmail when password is not set cause OAuth2 #4975

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.

Sign up for GitHub

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

Jump to bottom
Closed
2 tasks done
gepd opened this issue Jan 9, 2023 · 11 comments · Fixed by #5621
Closed
2 tasks done

🐛 Bug Report: Error 500 using updateEmail when password is not set cause OAuth2 #4975

gepd opened this issue Jan 9, 2023 · 11 comments · Fixed by #5621
Assignees
@stnguyen90 @gepd
Labels
bug Something isn't working good first issue Good for newcomers product / auth Fixes and upgrades for the Appwrite Auth / Users / Teams services.
Milestone
1.4.0

Comments

@gepd
Copy link
Contributor
gepd commented Jan 9, 2023 •
edited
Loading

👟 Reproduction steps

  1. Authenticate with any OAuth2 provider
  2. try to update the email or phone using the updateEmail or updatePhone endpoint

👍 Expected behavior

Update the email or phone without ask for a password, or use other method to confirm the changes

👎 Actual Behavior

We get an error

Unhandled Runtime Error
AppwriteException: Server Error

I have test it with updateEmail and updatePhone but this should happen in any endpoint who requires a password

the container logs shows the following error:

[Error] Method: PATCH
[Error] URL: /v1/account/phone
[Error] Type: TypeError
[Error] Message: Appwrite\Auth\Auth::passwordVerify(): Argument #2 ($hash) must be of type string, null given, called in /usr/src/code/app/controllers/api/account.php on line 1569
[Error] File: /usr/src/code/src/Appwrite/Auth/Auth.php
[Error] Line: 216

🎲 Appwrite version

Version 1.2.x

💻 Operating system

Linux

🧱 Your Environment

No response

👀 Have you spent some time to check if this issue has been raised before?

  • I checked and didn't find similar issue

🏢 Have you read the Code of Conduct?
@gepd gepd added the bug Something isn't working label Jan 9, 2023
@AkashSharma001
Copy link

I would like to work on this. @stnguyen90

@harshmange44
Copy link

Hey, I would like to fix this @gepd @stnguyen90

@stnguyen90 stnguyen90 added the product / auth Fixes and upgrades for the Appwrite Auth / Users / Teams services. label Jan 10, 2023
@stnguyen90
Copy link
Contributor

@gepd, would you please provide the docker logs for the appwrite container related to this error?

@stnguyen90 stnguyen90 self-assigned this Jan 10, 2023
@gepd
Copy link
Contributor Author
gepd commented Jan 10, 2023

Sure! @stnguyen90 I have updated the issue with the info

@stnguyen90
Copy link
Contributor

@gepd thanks! let me check with the team to see how we want Appwrite to behave for this.

@gepd
Copy link
Contributor Author
gepd commented Jan 10, 2023 •
edited
Loading

I didn't realize, this is caused for a different reason in my case; the user is using an OAuth2 authentication that is why there is not password set.

Isn't possible to this happen with email and password authentication as, for use any of that endpoint first you need to have an active session and as the user haven't a password, the session can't be created

I think this problem can happen with any other password less authentication method when the user doesn't have set a password

@gepd gepd changed the title 🐛 Bug Report: Error 500 using updateEmail|updatePhone when password is not set 🐛 Bug Report: Error 500 using updateEmail when password is not set cause OAuth2 Jan 10, 2023
@gepd
Copy link
Contributor Author
gepd commented Jan 17, 2023

@stnguyen90 did you get any answer from the team?

@stnguyen90
Copy link
Contributor

@gepd, sorry I missed this! we discussed this internally, and we have decided we:

  1. shouldn't do any validation before updating email or phone if the user doesn't have a password
  2. shouldn't set the password with what was passed in if the user doesn't have a password

@stnguyen90
Copy link
Contributor

Possibly related: #4990

@stnguyen90 stnguyen90 removed their assignment Feb 23, 2023
@stnguyen90 stnguyen90 added the good first issue Good for newcomers label Feb 23, 2023
@gepd
Copy link
Contributor Author
gepd commented Feb 24, 2023 •
edited
Loading

To do this, we should skip the validation in this 3 endpoints

appwrite/app/controllers/api/account.php

Line 1561 in 8b7dd6e

if (!empty($user->getAttribute('passwordUpdate')) && !Auth::passwordVerify($oldPassword, $user->getAttribute('password'), $user->getAttribute('hash'), $user->getAttribute('hashOptions'))) { // Double check user password

appwrite/app/controllers/api/account.php

Line 1661 in 8b7dd6e

!Auth::passwordVerify($password, $user->getAttribute('password'), $user->getAttribute('hash'), $user->getAttribute('hashOptions'))

appwrite/app/controllers/api/account.php

Line 1605 in 8b7dd6e

!Auth::passwordVerify($password, $user->getAttribute('password'), $user->getAttribute('hash'), $user->getAttribute('hashOptions'))

I can send a PR to fix this, what do you think?

@stnguyen90
Copy link
Contributor

@gepd, assigned!

@stnguyen90 stnguyen90 self-assigned this May 25, 2023
@stnguyen90 stnguyen90 mentioned this issue May 25, 2023
Closed
2 tasks
@stnguyen90 stnguyen90 added this to the 1.4.0 milestone May 26, 2023
@stnguyen90 stnguyen90 mentioned this issue Jun 1, 2023
Merged
2 tasks
@stnguyen90 stnguyen90 mentioned this issue Jul 22, 2023
Merged
This was referenced Jul 25, 2023
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stnguyen90 stnguyen90

@gepd gepd

Labels
bug Something isn't working good first issue Good for newcomers product / auth Fixes and upgrades for the Appwrite Auth / Users / Teams services.
Projects
None yet
Milestone
1.4.0
Development

Successfully merging a pull request may close this issue.

Allow Updating email and phone without password appwrite/appwrite
4 participants
@stnguyen90 @gepd @harshmange44 @AkashSharma001
0