1480844 - Deleting contact in top level of addressbook can delete account in AD (active directory) if used as member of domain admin, with no warning. Delete for ldap accounts should be disabled

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[fpicabia]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20180626223325

Steps to reproduce:

Prerequisite: Account linked to LDAP in AD (MS Active Directory) has domain admin rights.

Look up a contact within context of the domain (click on world icon) -> right click on person and Delete is greyed out.  That is good.

Look up a contact on the context of All Addressbooks (top level) -> right click on person and Delete is available.  When deleted, if this account is in AD, the AD account is deleted!  There is no warning, no information on the context where I am finding this user.



Actual results:

AD account is deleted while tidying up legacy contacts in addressbook accrued from many years of using Thunderbird.  Triggered a security incident at my work place.  Not fun.


Expected results:

Grey out Delete if the account is in AD, or say something about what is about to happen with an "are you sure?".  I can't believe anyone intended for AD accounts to be managed through the Thunderbird addressbook.

We are removing domain rights from personal accounts used for email, which will eliminate the risk, but at the same time, given the greyed out right click "Delete" option within the LDAP context, I don't think this capability was intended again the LDAP server.

Comment 1

[Magnus Melin [:mkmelin]]

Somewhat surprising yes, I didn't think we support writing ldap (bug 86405).

That said, the ldap server seems mis-configured. Why is it giving you delete privileges instead of just add and edit?

Comment 2

[fpicabia]

We have changed things so my account doesn't have domain admin rights.  Domain admins can add, change and delete users.  It is a feature.  Do you want to convince Microsoft that should change?

I see it as a Thunderbird bug, as much as you are inclined to say "it's a feature".  

Why is the option to delete greyed out if I'm in the LDAP context?  Would that be the way it is supposed to work?

Why does it allow deleting of local and LDAP - any context, any source, from the top level of searching the addressbook?  In my case there were addresses collected from having email sent to them, and these were very old test addresses.  I believed they were only in Thunderbird and I selected one contact too many - which was in AD.  It deleted the account in AD and in O365 after Dirsync completed 30 minutes later.

If I were to delete someone in AD using the Microsoft AD Users and Computers, it would have prompted me with a confirmation.

I asked another domain admin to attempt the same in Outlook with the global addressbook and deletion in AD isn't possible from there.

I've got half a dozen admins here now that are very unimpressed with the Thunderbird behaviour.  If it wasn't for removing me from the domain admins, I would be uninstalling Thunderbird at this point as "not safe for work".

Comment 3

[Wayne Mery (:wsmwk)]

> Why is it giving you delete privileges instead of just add and edit?

One could just as easily ask why not. Are you saying no other applications which use your ldap allow this?

Agreed there should at least be a prompt.

Karl, iirc you are on AD, but you have an ldap environment. Do you see anything like this?

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Wayne Mery (:wsmwk) from comment #3)
> > Why is it giving you delete privileges instead of just add and edit?
> 
> One could just as easily ask why not. Are you saying no other applications
> which use your ldap allow this?

I suppose for domain admins it could give those privileges. That group is somewhat limited of course.
If you're a regular user, then it's definitely a misconfiguration to allow deleting other people's entries.

> Agreed there should at least be a prompt.

There's a prompt. It's just not clear it will delete the user account if the ldap is managing user accounts and you happen to have enough privileges.

Comment 5

[Magnus Melin [:mkmelin]]

I agree it's unintended behavior, and we should disable Delete for ldap accounts.

https://dxr.mozilla.org/comm-central/rev/e5e1510b8d914bfa8439b21ba3f73e4f2e83e957/mail/components/addrbook/content/abCommon.js#99

In the "All Addressbooks" case the uri is moz-abdirectory://? so it won't match and won't disable delete.

That uri check would have to see for each card if we have a match. Like so: https://dxr.mozilla.org/comm-central/rev/e5e1510b8d914bfa8439b21ba3f73e4f2e83e957/mail/components/addrbook/content/abCard.js#177

Comment 6

[Joseph McCallum-Nattrass]

I'd like to work on a patch if you'd assign me. At least a prompt to quell this "bug vs feature" debate until a consensus can be met. I'm a student at Coventry University and would like to include this bug fix in my coursework.

Comment 7

[Magnus Melin [:mkmelin]]

Great, go ahead. A description of what needs to be done is in comment 5, with code pointers.

Comment 8

[james]

Hey,
Is anyone still owrking on this bug?
If not, I would like to give it a shot

Comment 9

[Magnus Melin [:mkmelin]]

Give it you best shot!

Comment 10

[james]

(In reply to Magnus Melin [:mkmelin] from comment #9)

Give it you best shot!

I am new to this, Can you please provide me with some kind of a lead or a starting point

Comment 11

[Magnus Melin [:mkmelin]]

You'll have to dig around some
https://searchfox.org/comm-central/rev/b88bab9f14fb8c5485e0c5be2779b383e1382b50/mail/components/addrbook/content/abCommon.js#453 see also https://searchfox.org/comm-central/rev/b88bab9f14fb8c5485e0c5be2779b383e1382b50/mail/components/addrbook/content/abCommon.js#119

Comment 12

[james]

(In reply to Magnus Melin [:mkmelin] from comment #11)

You'll have to dig around some
https://searchfox.org/comm-central/rev/b88bab9f14fb8c5485e0c5be2779b383e1382b50/mail/components/addrbook/content/abCommon.js#453 see also https://searchfox.org/comm-central/rev/b88bab9f14fb8c5485e0c5be2779b383e1382b50/mail/components/addrbook/content/abCommon.js#119

I have a few question,

1. What is meant by top level of address book?
2. what do you mean by member of domain admin?
3. In Comment 2 what is meant by "top level of searching the address book"?
4. What is LDAP context?

see also https://searchfox.org/comm-central/rev/b88bab9f14fb8c5485e0c5be2779b383e1382b50/mail/components/addrbook/content/abCommon.js#119

5. As far as I understand this if condition checks whether the account is in the LDAP list, and returns true or false for deletion and non deletion respectively. if we need to disable deletion of LDAP accounts we can simply just return TRUE if the main if condition gets satistfied, i.e. if the contact is in mailing list, correct me if I am wrong here plz.

Comment 13

[Ping Chen [:rnons] (away)]

Attachment: Bug 1480844 - Disable delete menu/button for LDAP contacts. r=mkmelin

Fix the problem of when "All Address Books" is selected, delete menu/button is enabled for LDAP contacts.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/32cf985282dc
Disable delete menu/button for LDAP contacts. r=mkmelin