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

Adding the possibility for users to modify their attributes #105

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

savaryj
Copy link

@savaryj savaryj commented May 5, 2021

I was working on this project in a traineeship.
The objective was to add the possibility for the users to identify themselves and then modify some of their basic infos (such as their phone number, profile picture, title, description...).

To do so, I added two new pages, one to authenticate the user and another to modify their infos.

While my coding skills aren't quite as good as yours. They wanted me to share what i've done.

There might be obvious security breaches in it, but it was juged petty as all is done on their own servers and it is a small company.

@faust64
Copy link
Contributor

faust64 commented May 5, 2021

Hi,

Thanks for your contribution!

There's already been several suggestions to implement some kind of authentication - though it did not imply adding anything else but just authenticating users browsing a directory. @coudot would have the final word on this.
In a way, having end-users editing their own entry could be interesting, for sure.

Now, checking the diff, it sounds like you're submitting files that do not match the last master branch: the mailquota attribute is gone from conf/config.php, as well as vcard_group_map / vcard_user_map (recent additions, while your copy has a vcard_map, which used to be there, before we fixed vcards generation dealing with LDAP groups)

Having photos stored locally instead of as LDAP attribute could be nice in some cases.

Filtering for sAMAccountNames (login.php) might work in some cases (active directories), though there's a setting in conf/config.php, that should tell which attribute to look for. Same goes for ditinguishedname, name, objectClass=Person, ...

Now, generating pages that embeds some hidden form input, with our user password might be problematic.
https://github.com/ltb-project/white-pages/pull/105/files#diff-9cc41686355c72b60fd5b3cf39b7034198d331a1ed64fd1fcb364a53fd7fa265R123
Meanwhile, it looks like that psswrd param is not even used, before editing the ldap object:
https://github.com/ltb-project/white-pages/pull/105/files#diff-9cc41686355c72b60fd5b3cf39b7034198d331a1ed64fd1fcb364a53fd7fa265R69

If I understand properly, I could POST to /editentry.php, with arbitrary params, and have the addn authenticate and edit my directory entries unchecked.
Also: not sure introducing an addn/adpwd is necessary: the ldap_binddn and ldap_bindpw might be kept, granted that this account has enough privileges editing LDAP objects.

If anything, user should re-authenticate at that stage. Or probably better: use PHP sessions - but then, might break stateless-ness, in the rare cases that would use several instances of WP, while there's no loadbalancer or if those balancers don't have sticky sessions configured.

Anyway, that's interesting, thanks again!

@coudot coudot added this to the future milestone May 5, 2021
@savaryj
Copy link
Author

savaryj commented May 5, 2021

Thanks for your reply !

In fact i didn't checked the version of the project before working on what they already had, it's a bit outdated.

Storing the photos in a local file is a good idea, but our mail service is paired with the active directory and using the thumbnail photos, allowing users to update them was one of the main goals of my mission.

The hidden psswrd was the way i found to store the user's password from a page to another (it sounds really naive now that i'm writing it).

When we first let users access this feature we saw that they couldn't modify their infos, due indeed to the lack of privileges to do so. That's why i came up with addn/adpwd.
But yeah, i did not realised that after adding the "admin account" (and forgetting to delete the storing of the user's password) anyone could then POST directly to editentry.php with whatever they want.

I bet you've already understood but this is the first time that i contribute to an open source project and i apology for i am a bit clumsy. Thank you for the serious look on my far from flawless code and for pointing out the problems.

I hope that i showed you an intersesting possible enhancement of your project.

Removed hidden inputs
No more admin account to modify the active directory (considering the user have the right privileges to do so), adding a php session to store the user's login informations.
Removed the admin account.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants