cooperlyt / keycloak-phone-provider

A Keycloak provider for phone and SMS
MIT License
270 stars 151 forks source link

need `phone number verify` required action ? #41

Open cooperlyt opened 1 year ago

cooperlyt commented 1 year ago

now we have update phone number , but this required action can change phone number .

sbabych commented 1 year ago

Hello, it seems like update phone number action doesnt check if such phoneNumber already assigned to another user

sbabych commented 1 year ago

@cooperlyt after a bit debug. so as far as I understood update phone number action should un-verify another users with the same phone. Unfortunately it doesn't work at keycloak 21.0.2

would it be posiibe to configure if it should throw an error or un-verify other users ?

cooperlyt commented 1 year ago

I will check it;

nilskretschmer commented 1 year ago

Maybe this is related to the problem:

Let's say we have this setup:

  1. User are organized in LDAP
  2. The user's phone number is also set in LDAP
  3. The user's phone number is mapped to phoneNumber user attribute in Keycloak (required by this package)
  4. The user logs in and still needs to verify his/her phone number
  5. Now The user is able to just enter another phone numer and request a verification code
  6. User enters the received verification code -> user successfully changed his own phone number even if he/she shouldn't be a allowed to do that

I think this could also lead to severe security issue, because user's are able to change their phone numbers on their behave

Possible solution: The policy should never update a user's phone number if there already is a phoneNumber and phoneNumberVerified = true on the user and the user should not be able to change his/her phone number on login.

nilskretschmer commented 1 year ago

Another finding: The SMS-OTP workflow fails on Configure OTP over SMS required action if LDAP is set to READ_ONLY. For me the easiest solution would be that the required action does not (re-)set a user's phoneNumber if it is already set and verified.