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

Fix sanity check in resource/create controller #16477

Merged
merged 1 commit into from Mar 6, 2024
Merged

Conversation

arjen-t
Copy link
Contributor

@arjen-t arjen-t commented Sep 27, 2023

What does it do?

Add sanity check for $userGroups in XPDO query during creating a new resource.

Why is it needed?

In the case the user is a sudo user and isn't assigned to any groups, the $userGroups which is a reflection of the user assigned groups can be empty. This cause a SQL error during creating a new resource.

Array
(
    [0] => 42000
    [1] => 1064
    [2] => You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') OR  ( `ProfileUserGroup`.`usergroup` IS NULL AND `UGProfile`.`active` = 1 )  )' at line 1
)

How to test

  1. Login in the manager with only sudo rights.
  2. Create a new resource
  3. See error log for the SQL error.

Related issue(s)/PR(s)

#16376

}

$criteriaUserGroups[] = array(
'OR:ProfileUserGroup.usergroup:IS' => null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this OR be there if the previous condition isn't added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far I know this OR will be ignored by xpdo when the previous condition isn't added.

When logging the statement in MODX the following occurs:

Without the $userGroups

AND ( ( ProfileUserGroup.usergroup IS NULL AND UGProfile.active = 1 ) OR ProfileUserGroup.usergroup IS NULL ) )

With $userGroups:

AND ( ( ProfileUserGroup.usergroup IN ('1') OR ( ProfileUserGroup.usergroup IS NULL AND UGProfile.active = 1 ) ) OR ProfileUserGroup.usergroup IS NULL ) )

side note which has nothing to do with this bug.

OR ( ProfileUserGroup.usergroup IS NULL AND UGProfile.active = 1 )

This nested OR seems to be redundant.

@rthrash rthrash added this to the v2.8.7 milestone Jan 24, 2024
Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

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

👍

@rthrash rthrash added the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Feb 8, 2024
@Mark-H Mark-H added the pr/port-to-3 Pull request has to be ported to 3.x. label Feb 10, 2024
@opengeek opengeek merged commit a6fc2d9 into modxcms:2.x Mar 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/port-to-3 Pull request has to be ported to 3.x. pr/ready-for-merging Pull request reviewed and tested and ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants