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

Projects page on the website portal is public, anyone can see it #39009

Closed
juanc1479 opened this issue Dec 28, 2023 · 12 comments · Fixed by #39164
Closed

Projects page on the website portal is public, anyone can see it #39009

juanc1479 opened this issue Dec 28, 2023 · 12 comments · Fixed by #39164
Labels

Comments

@juanc1479
Copy link

juanc1479 commented Dec 28, 2023

Information about bug

Under an incognito and unlogged user, I can access the list of projects when they should be private and only accessible when signed in with the correct permissions. erp.example.com/project When I click on erp.example.com/newsletters they are inaccessible and I need to log in, which is correct as I set the newsletters to private.

Could you point me to where in the code base I can look to see if I can fix the privacy issue? Thank you.

2023-12-28_10-10-24

Module

projects, portal

Version

ERPNext: v15.8.3 (version-15)
Frappe Framework: v15.7.0 (version-15)
Frappe HR: v15.7.1 (version-15)
Payments: v0.0.1 (version-15)

Installation method

manual install

Relevant log output / Stack trace / Full Error Message.

I checked the logs in the sites folder, but I couldn't find the line items related to this bug. Are there other logs where I can find more information about this bug?

@juanc1479 juanc1479 added the bug label Dec 28, 2023
@agritheory
Copy link
Contributor

@juanc1479 What you say is true - that it is configured to be visible by default. You can disable it in Portal Settings.

@juanc1479
Copy link
Author

After disabling the menu item and changing the Route, public access to the list of projects remains for non-logged-in users.

image

@agritheory
Copy link
Contributor

@juanc1479 I agree, that doesn't seem right. The file you asked for is located here, but I don't see a problem. I guess there's an edge case where the "Guest" user could be a "Project User". Are these projects all the ones in your database?

https://github.com/frappe/erpnext/blob/version-15/erpnext/templates/pages/projects.py

@juanc1479
Copy link
Author

Yes, those are all the projects in the database.

@juanc1479
Copy link
Author

These are the current role permissions for Guest:

image

@agritheory
Copy link
Contributor

In the "Users" section of those projects, can you see if there is a "Guest" user? That eliminates the edge case.

@juanc1479
Copy link
Author

The "Users" section is empty:

image

Could the bug originate from line 16 where the IF statement grants the "Guest" user access to the project list?

if frappe.session.user != "Administrator" and (
not project_user or frappe.session.user == "Guest"

image

@agritheory
Copy link
Contributor

agritheory commented Dec 28, 2023

This hasn't changed since V14 and it works correctly on my V14 instance. I suspect this is not the source of the bug. This is most likely a configuration problem, but I don't know where it could be coming from. Testing locally in V14 works as expected.

@juanc1479
Copy link
Author

I installed both ERPNext V14 and V15 versions without any modifications. On V14 erp.example.com/project the project list is inaccessible. On V15, the erp.example.com/project project list is public to any user who is not logged in. I presume there was a code change in V15 that is allowing the project list to be public.

image

@SvbZ3r0
Copy link
Contributor

SvbZ3r0 commented Dec 29, 2023

Can confirm this issue exists on my v15 instance.

@s-aga-r
Copy link
Contributor

s-aga-r commented Dec 31, 2023

@juanc1479 Duplicate of #39155

@s-aga-r s-aga-r closed this as not planned Won't fix, can't repro, duplicate, stale Dec 31, 2023
@0xD0M1M0
Copy link
Contributor

0xD0M1M0 commented Jan 4, 2024

@s-aga-r I found the issue. It is not in the frappe/frappe, but an frappe/erpnext issue. Permissions in

frappe-bench/apps/erpnext/erpnext/projects/doctype/project/project.py : get_project_list

are overwritten with "ignore_permissions = True". The problem with fixing it is the following:

The permissions check, for the users are "complicated". There are the "Portal User" permissions by the Supplier and Customer and the "Project User Permissions". Which one should be taken into account? I would personnaly opt for the "Project User Permission" to be checked and validated if set. I think this is not at all in the code. At least the line:

if is_website_user():

should be at least:

if is_website_user() and user!="Guest":

and ignore_permissions = True should be inside the if:

if customers:
    filters.append([doctype, "customer", "in", customers])
    ignore_permissions = True

This fixes the main issue.

0xD0M1M0 added a commit to 0xD0M1M0/erpnext that referenced this issue Jan 5, 2024
fix: projects website list visible for guests and all logged in customers

see the issue for details

fixes issue frappe#39009
s-aga-r pushed a commit that referenced this issue Jan 10, 2024
…mers (#39164)

* fix: projects website list visible for guests

fix: projects website list visible for guests and all logged in customers

see the issue for details

fixes issue #39009

* fix: remove user = frappe.session.user
mergify bot pushed a commit that referenced this issue Jan 10, 2024
…mers (#39164)

* fix: projects website list visible for guests

fix: projects website list visible for guests and all logged in customers

see the issue for details

fixes issue #39009

* fix: remove user = frappe.session.user

(cherry picked from commit eabf706)
s-aga-r pushed a commit that referenced this issue Jan 10, 2024
…mers (backport #39164) (#39273)

fix: projects website list visible for guests and all logged in customers (#39164)

* fix: projects website list visible for guests

fix: projects website list visible for guests and all logged in customers

see the issue for details

fixes issue #39009

* fix: remove user = frappe.session.user

(cherry picked from commit eabf706)

Co-authored-by: 0xD0M1M0 <76812428+0xD0M1M0@users.noreply.github.com>
frappe-pr-bot pushed a commit that referenced this issue Jan 10, 2024
# [15.10.0](v15.9.1...v15.10.0) (2024-01-10)

### Bug Fixes

* add expected_start_date in sort by ([b064944](b064944))
* Add name to Hungary - Chart of Accounts for Microenterprises json ([0784488](0784488))
* add read permission to Buying Settings ([e62dd0d](e62dd0d))
* bank transction status upon reconciliation ([277aa7b](277aa7b))
* BOM replace tool does not update exploded items of root (backport [#39244](#39244)) ([#39250](#39250)) ([0898ea5](0898ea5))
* Creating Asset Activity while Importing Asset ([#39113](#39113)) ([5e46937](5e46937))
* don't set rate for non-stock item in Internal Transfer (backport [#39140](#39140)) ([#39169](#39169)) ([f3882a8](f3882a8))
* Duplicate Closing Stock Balance (backport [#39262](#39262)) ([#39264](#39264)) ([78c65f2](78c65f2))
* duplicate entry for serial / batch creation (backport [#39188](#39188)) ([#39192](#39192)) ([672e6d6](672e6d6))
* **Employee:** treeview ([#39126](#39126)) ([6020c8e](6020c8e))
* FG Item incorrect qty in the work order (backport [#39200](#39200)) ([#39211](#39211)) ([abc99f8](abc99f8))
* flaky demo test case (backport [#39135](#39135)) ([#39198](#39198)) ([3caf462](3caf462))
* Ignore asset qty and status validation while cancelling LCV ([87d1b0f](87d1b0f))
* ignore cancelled payments in Sales/Purchase Register ([9e1b443](9e1b443))
* Ignore UP on "allowed to transact with" ([#39103](#39103)) ([aad39cf](aad39cf))
* improved validation message ([e89dce7](e89dce7))
* incorrect indicator title for portal sales order (backport [#39247](#39247)) ([#39255](#39255)) ([9f793b9](9f793b9))
* incorrect outstanding amt validation on advance as liability ([51d8a7a](51d8a7a))
* Introduced company field to show amounts in company currency ([1d2e831](1d2e831))
* inventory dimension negative stock validation (backport [#39149](#39149)) ([#39151](#39151)) ([82b96d3](82b96d3))
* possible key error on Financial ratios report ([16269b0](16269b0))
* possible typeerror on transaction.js ([fec892c](fec892c))
* projects website list visible for guests and all logged in customers (backport [#39164](#39164)) ([#39273](#39273)) ([5d6bc96](5d6bc96)), closes [#39009](#39009)
* Purchase date and amount is not mandatory for composite asset creation ([d6d54ed](d6d54ed))
* Resolved conflict ([afefae0](afefae0))
* serial / batch barcode scanner (backport [#39114](#39114)) ([#39143](#39143)) ([2db1e1a](2db1e1a))
* Set asset purchase amount based on qty and valuation_rate ([f0070b4](f0070b4))
* set parent doctype on chart (backport [#39286](#39286)) ([#39288](#39288)) ([fe973a4](fe973a4))
* Show maintain-stock and is-fixed-asset checkbox in item quick entry dialog ([50300b9](50300b9))
* Show timesheet table after fetching data from timesheet (backport [#39275](#39275)) ([#39281](#39281)) ([2598f8e](2598f8e))
* skip rate validation for return `DN Items` with `Moving Average` valuation (backport [#39242](#39242)) ([#39266](#39266)) ([f00a6f6](f00a6f6))
* Subscription update patch ([7640fea](7640fea))
* total allocated percentage for sales team issue ([756c062](756c062))
* TypeError is pricing rules (backport [#39252](#39252)) ([#39260](#39260)) ([bb6025c](bb6025c))
* typerror on multi select dialog ([54a0df5](54a0df5))
* update Maintenance Schedule status on Maintenance Visit submit (backport [#39167](#39167)) ([#39186](#39186)) ([926850d](926850d))
* update status on manual allocation ([a147e29](a147e29))
* **UX:** dont override framework's permission check messages (backport [#39118](#39118)) ([#39120](#39120)) ([40ec5ff](40ec5ff))

### Features

* provision to close SCO (backport [#39127](#39127)) ([#39144](#39144)) ([b192ddd](b192ddd))
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants