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

[18.0][MIG] dms: Migration to 18.0 #385

Open
wants to merge 168 commits into
base: 18.0
Choose a base branch
from
Open

Conversation

kobros-tech
Copy link

No description provided.

Mathias Markl and others added 30 commits January 13, 2025 21:48
versions of muk_dms than 12.0.2.0.0.

Tested from 1.2.4 version.
In v13, this test is programmed in such a way that the demo user is supposed to be able to copy that subdirectory: https://github.com/OCA/dms/blob/c3f802db43362127e70d8c7b4987fb71d4c1f01c/dms/tests/test_directory.py#L40

However, in OCA#7 that test was modified indicating that demo user didn't have permissions to do that: https://github.com/OCA/dms/blob/e3b6d8d24534f2a68bfb88e310cc70cefe46bb64/dms/tests/test_directory.py#L39

Rolling back that change to ensure premissions remain the same in both versions of the module.

Also changing the directory to test to ensure it contains no SVG files, whose detection seems to differ among environments, and which have some specific permission restrictions that can make the modified test fail or pass.

@Tecnativa TT25645
@kobros-tech kobros-tech force-pushed the 18.0-mig-dms branch 2 times, most recently from 58ffd79 to 9d4328d Compare January 27, 2025 15:46
@kobros-tech
Copy link
Author

@dnplkndll
@etobella
@pedrobaeza

If have found a critical bug in the module:

if you are an internal user and not a member of dms groups, if you have the url link to any file of dms you would access it.

You still can not modify, but may be accessing file can be critical.

the good this is that this bug is for files and not for directories.

Another simple bug in Portal, you can not find the breadcrump history in the top left corner, and I can fix it.

Some trivial kanban designs if someone can observe too!

@kobros-tech
Copy link
Author

/ocabot migration dms

@pedrobaeza

@OCA-git-bot
Copy link
Contributor

Sorry @kobros-tech you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@kobros-tech
Copy link
Author

@etobella
can we get this merged?

Copy link

@dnplkndll dnplkndll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! there are some features that might be nice but the MIG looks good.

@@ -71,7 +71,7 @@ def portal_my_dms(
if search and search_in == "name":
domain += OR([[], [("name", "ilike", search)]])
# content according to pager and archive selected
items = request.env["dms.directory"].search(domain, order=sort_order)
items = request.env["dms.directory"].sudo().search(domain, order=sort_order)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why sudo?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etobella
there is no defined rule for portal users to access dms.directory

If you can add rules for portal users, I welcome

@etobella
Copy link
Member

etobella commented Feb 3, 2025

Can you squash your commits please?

@@ -116,7 +116,7 @@
<field name="perm_create" eval="1" />
<field name="perm_write" eval="0" />
<field name="perm_unlink" eval="0" />
<field name="domain_force">[('permission_create', '=', user.id)]</field>
<field name="domain_force">[('permission_create', '=', True)]</field>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victoralmau
Does this domain make any sense?

[('permission_create', '=', user.id)]

how can permission_create of type boolean be equal to user.id of type integer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous domain is the right one and it is absolutely necessary for everything to work correctly, to know if a user can access or not (create/delete/write) depends on that domain.

@kobros-tech kobros-tech force-pushed the 18.0-mig-dms branch 2 times, most recently from 65d46c3 to b9befa0 Compare February 4, 2025 02:35
@kobros-tech
Copy link
Author

Can you squash your commits please?

I don't prefer as they indicate fixes to critical issues that can be reviewed or improved.

Even more some contributors from ForgeFlow have committed in this PR, I can not remove commits.

@kobros-tech
Copy link
Author

@etobella
@victoralmau
@pedrobaeza

If you assure to me that all technical reviews and implementations are ok, I don't mind to squach my commits.

@daas13
Copy link

daas13 commented Feb 4, 2025

user with no permissions can access to file and folder !

@kobros-tech
Copy link
Author

user with no permissions can access to file and folder !

not portal users in this migration, only users who are under of group dms user can access

not base backend users and not portal users.

You can remove my sudo and try portal, and also you can create a new user without access group of dms users, and please tell me if they can access?

@daas13
Copy link

daas13 commented Feb 4, 2025

i create bankend user with dms user access , and give him no right in access group, but this user still see all files and folder, i'm doing right here ?

i will see with portal !

user with no permissions can access to file and folder !

not portal users in this migration, only users who are under of group dms user can access

not base backend users and not portal users.

You can remove my sudo and try portal, and also you can create a new user without access group of dms users, and please tell me if they can access?

@kobros-tech
Copy link
Author

@daas13
The default if you make filestore hidden no body will see it no any group.

in your case I will make sure if after you create internal user you remove the dms group as it is appended automatically to base group.

I can try and see.

@daas13
Copy link

daas13 commented Feb 4, 2025

Also the portal user can see all files and folder without any permissions !

@kobros-tech
Copy link
Author

Also the portal user can see all files and folder without any permissions !

yes, I know they can see all not hidden files same like dms users.

I need suggestions and opinions for assigning groups and rules for portal users and base backend users as the module itself doesn't have this till now.

You can write issues here and a part from here and refer to this PR or the module itself.

@kobros-tech
Copy link
Author

@daas13 The default if you make filestore hidden no body will see it no any group.

in your case I will make sure if after you create internal user you remove the dms group as it is appended automatically to base group.

I can try and see.

As I said, internal users can not access documents if they are not part of the dms groups, I tested it again.
Of course portal users are different and both of them need special access groups and rules.
We can have a discussion on how this can be implemented.

Screenshot from 2025-02-04 17-10-56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.