Add an addon for password-based authentication against Keycloak. #1116

Merged

very-ape merged 1 commit from keycloakpassword into develop

2021-05-19 19:10:40 +02:00

very-ape commented

2021-05-19 01:05:19 +02:00

(Migrated from github.com)

Would it be possible to use the "authenticate" hook for this instead/in addition? The way that it's done now would seem to create issues with any external authentication mechanism at all, even if the addon saves a copy of the user's hashed password in Friendica's database upon authentication. For example, if I were to log in via the LDAP addon and then change my password on the LDAP side, Friendica will continue expecting my old password in these password confirmation boxes until I log out and back in. Using the "authenticate" hook instead would solve this problem of credentials getting out of sync and would prevent the user's credentials from being stored in more places than necessary, which seems a little bit better to me from a security perspective.

Edit: It seems like DI::auth()->withPassword (or some abbreviated derivative of it) instead of User::getIdFromPasswordAuthentication might do the trick, at least for the check in Module/Settings/TwoFactor/Index.php. This is work I'd be willing to do myself, if it seems good and right to y'all for me to do so.

Edit 2: Reviewing the ldapauth addon again, I see that there's actually no credential synchronization done beyond the initial user creation upon first login. So if I ever change my password in LDAP, I'll still have to manually change my password in Friendica for those "current password" boxes to stop expecting my old password. My mistake.

My idea with this addon was to allow the user to use their Keycloak password wherever password confirmation is required in the Friendica UI, but I see that (at least in the case of the two factor UI) this is done by directly checking the user's password in the database. Would it be possible to use the "authenticate" hook for this instead/in addition? The way that it's done now would seem to create issues with any external authentication mechanism at all, even if the addon saves a copy of the user's hashed password in Friendica's database upon authentication. ~~For example, if I were to log in via the LDAP addon and then change my password on the LDAP side, Friendica will continue expecting my old password in these password confirmation boxes until I log out and back in.~~ Using the "authenticate" hook instead would solve this problem of credentials getting out of sync and would prevent the user's credentials from being stored in more places than necessary, which seems a little bit better to me from a security perspective. Edit: It seems like DI::auth()->withPassword (or some abbreviated derivative of it) instead of User::getIdFromPasswordAuthentication might do the trick, at least for the check in Module/Settings/TwoFactor/Index.php. This is work I'd be willing to do myself, if it seems good and right to y'all for me to do so. Edit 2: Reviewing the ldapauth addon again, I see that there's actually no credential synchronization done beyond the initial user creation upon first login. So if I ever change my password in LDAP, I'll still have to manually change my password in Friendica for those "current password" boxes to stop expecting my old password. My mistake.

MrPetovan commented

2021-05-19 17:58:47 +02:00

(Migrated from github.com)

It took me a minute and a couple reads to understand the issue, but now I get it. Yes, it would be better to use the authenticate hook, if you're up for it, please go ahead.

It took me a minute and a couple reads to understand the issue, but now I get it. Yes, it would be better to use the `authenticate` hook, if you're up for it, please go ahead.

🎉 1

MrPetovan commented

2021-05-19 18:10:00 +02:00

(Migrated from github.com)

Does this PR depends on the work you've planned?

very-ape commented

2021-05-19 18:36:07 +02:00

(Migrated from github.com)

No, it's just enhanced by it. This addon works fine as it is.

👍 1

MrPetovan (Migrated from github.com) approved these changes 2021-05-19 19:10:34 +02:00