I am having trouble preventing ERPNext from leaking information out via the website.
We have a large number of suppliers. We add various contacts to these suppliers to keep track of them. Just because we have added a contact, doesn’t mean we want them to see any of our internal documents (which includes all the PO, PINV, PREC etc).
If one of these suppliers goes and registers on our website, their account will automatically get set to ‘Supplier’. There appears to be no way to disable this (it is set in erpnext hooks.py).
If they are a ‘Supplier’, they see a load of sidebar menus including Project, PINV, PO etc that we don’t want them to see. There appears to be no (non-hacky) way of hiding these, since Portal Settings is read-only (albeit only in the JS).
If they click on (for example) one of the POs, they can see all the details. They even get the ‘create PINV’ button.
Removing all permissions from ‘Supplier’ has not fixed this issue - despite having no system permissions or user permissions for these POs and PINVs they can still see these documents.
It should be hard to reveal data and easy to hide it. Nowhere does it say in the ERPNext documentation ‘oh, by the way, if you add an email contact for a supplier they will be able to see their documents and even create new ones’! This is a horrible security problem for us.
I have done as a temporary security measure, but we would like to sell things online at some point (as well as actually using some custom portals that our suppliers are allowed to see).
Customers, suppliers, basically everyone who has access to the portal will only see documents that belong to them. For example, customer A will not see invoices or orders of customer B.
Also, if you’re thinking of selling items through the website, you will need the portal.
And, you can hide those unrelated portal pages from DocTypes. For example, Customize Form → Projects → uncheck “has web view”.
A bit of brief testing suggests this is not the case. Yes, they don’t appear in the list, but plug the routes in directly and any supplier can see any purchase order on the system regardless of supplier…
If the routes were randomised this wouldn’t be a disaster but since they are just in numerical order, it would be trivial to valid documents.
It gets weirder: seems you can’t see any PO at all if you have the ‘Customer’ role (or more specifically, if you have a Customer attached to the contact attached to the user). I think the controllers → website_list_for_contact → has_website_permission is buggy and broken.
Time to do some debugging…
I still think you shouldn’t be able to see any documents you don’t have role-based permissions for (which would simply require adding ‘doc.has_permission(‘read’)’ to templates/pages/order.py, as is done for other pages).
How is it supposed to work? There are competing things here.
First, you want users with Desk access and Role permissions for (for example) Purchase Order to be able to see those on the website. This can be handled by role permissions: if you have the role, you have the permission.
Second, you might want website users to be able to see none of the documents, all of the documents, or just only those documents they are linked to. You can’t easily have all three options available, and I can’t quite work out how it is supposed to work in ERPNext.
If you require the user to have Role permissions to view even linked contacts, then you can easily restrict website users by just removing the permissions from ‘Supplier’. But then Desk viewers can’t view the documents either unless they are linked directly…
If you don’t require the user to have Role permissions to view their documents, then it is really hard to stop them. I think it can be done by adding a custom has_website_permission hook for each document type you want to restrict access to, but this is hacky and it should just be an option somewhere.
You might also just decide not to give users linked to Suppliers the Supplier role to prevent them . Unfortunately, you can’t easily do that as it is automatically assigned to them when they sign up.
I would note that the way I think it is currently working in ERPNext (or will be once an incidental bug is fixed) is that the Supplier role has permissions for Purchase Order, and as long as the user is linked by contact that is the only PO they will be able to view. But if that got deleted somehow, the system would fall back to role-based permission, and then they would be able to see all of them… Equally a system user linked to a Purchase Order would only be able to see their own POs!
I already have (and have done in the past), but response is always glacially slow (reported another one about 2/3 weeks ago that I still haven’t heard back about yet)…
We need this fixed now, so was going to try and do it myself.
As I understand it, Role Permissions are for desk and api access. Portal access is handled differently, where users are able to see only documents linked to them.
I have also (coincidentally) realised that Portal Settings is broken because it was using ‘grid.only_sortable’ in the JS. In V11 this didn’t make the grid rows themselves read-only (or maybe just checkboxes?’ but in V12 this seems to make the rows read-only as well. Which is annoying, as that is functionality I have also used… The aim there is to prevent rows being added or deleted, but allowing some alteration of values in columns that aren’t read-only (i.e. turning things on and off).
I think something’s amiss with your particular instance. On my install, also v12, I’m seeing very different behavior:
Portal settings are fully editable, and they allow me to control which kinds of documents are visible from the portal.
Website users are only able to see documents that they are linked to, and they get a permission error if they manually enter urls routes for documents they shouldn’t see.
I can confirm that Purchase Orders are unexpectedly NOT visible for users who are both customers and suppliers. I am also able to create a Purchase Invoice from a Purchase Order. This appears to be intentional, however, and I don’t know enough about accounting to have a sense of why this choice was made.
Most broadly, I am also concerned by the unexpected behavior whereby users are matched via email address. I think it would be a far better approach to require manual linking, though I’m not encountering any of the serious security issues you are.
Edit: Apologies, it seems we’re posting at the same time, and you’ve addressed a few things here already
At least I’m not going completely mad
What version of V12 (commit ref or whatever) are you using? I’m going to do a bench update and see if the only_sortable/Portal Settings thing persists, and if its broken in the newest version try and track down the offending commit…
Good find! The version I tested was a local instance, and I’m away from that computer now unfortunately so I can’t check the exact version. Based on when I set it up, I suspect it was the 12.7.1 release (though it could have been slightly earlier). I did have develop mode on, though, so it’s possible I changed something.
Anyway, I’ll keep poking at it, and I’m glad you’re checking into the security here. We had been thinking about rolling out the portal ourselves, and you’ve raised a few important questions.
The more you look at the code (which incidentally doesn’t have a single comment in the entire file) the worse it gets…
As well as completely failing the account for a user who is both a Customer and a Supplier (or more specifically, has any Customer linked to their Contact as well as any Supplier linked to their contact), the bit of code that matches contacts to the user looks at the contact email_id, rather than the ‘user’ field. So you might think that emptying the ‘User’ field on the contact (which is normally auto-filled with a matching User) would remove the permissions access. But you would be wrong…
Well, that is precisely the point of insisting and promoting ‘the tests’ as the truth - to state and enforce the agreed convention and understanding of how the code is supposed to work:
