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

[GDPR] | Invite to a role - Removing Role, with access to settings does not remove all attributes of the previous role #11015

Open
1 task done
Tribunal33 opened this issue Mar 2, 2025 · 11 comments
Assignees
Labels
Bug:2:Major A bug found in common paths that reduces functionality for a large number of users
Milestone

Comments

@Tribunal33
Copy link
Contributor

Valid Title

  • I have updated the title to accurately reflect the bug description

Description

When removing a Journal Editor role but I think it might be for all roles. There are traces of the role left over. I will go over the easiest to identify but I'm worried this might mean that the database tables for removed role are not fully being flushed correctly. Easiest way to see this is with the Journal Editor role from Dbarnes.

I think this works for removing role either when inviting to a new role or from existing user table. I will focus on when inviting to a new role.

Steps to Reproduce

  1. Login with a User that can access settings and is not Dbarnes, who has Journal Editor role
  2. Navigate to Users and Roles
  3. Invite Dbarnes (or an existing user with JE permissions) to a new role.
  4. Remove the Journal Editor role
  5. Proceed with inviting Dbarnes to any other role that should not have access to settings like author
  6. After invitation is completed login with Dbarnes
  7. Go to Dashboard observe that Settings is not hidden for this user. Author's, for example, should not have access to settings.
  8. Select settings Users & Roles and keep a bookmark of the URL (for another bug)

Expected Result

Should not be able to view the settings navigation options unless you have access to that.

Actual Result

Can access the settings options and

Environment Details

No response

Application Version

OJS stable-3_5_0

Logs

No response

Additional Information

No response

@Tribunal33 Tribunal33 added the Bug:2:Major A bug found in common paths that reduces functionality for a large number of users label Mar 2, 2025
@Tribunal33 Tribunal33 added this to the 3.5 Internal milestone Mar 2, 2025
@Devika008
Copy link

I just tested this as Dbarnes and found that I cannot see anything. However, despite not accepting the role of Production Editor, I can still access the Dashboard from the dropdown and then shown the profile, which seems inconsistent.

If a user has no assigned role—not even as a reader—and has not accepted any role, they should not be shown the dashboard. This behavior needs to be reviewed.

Image

@ipula
Copy link
Contributor

ipula commented Mar 4, 2025

@Tribunal33 @Devika008 there is a check in the code. The navigation URL will be change according to the user roles. I will not going to change this until reviewed this

@Tribunal33
Copy link
Contributor Author

@ipula sorry ipula this might be a bigger issue. The focus of my point was the Dashboard Settings is only available to the admin, JM and JE roles or if that role was granted access as in the ticket #5504

@ewhanson
Copy link
Collaborator

ewhanson commented Mar 6, 2025

@ipula, a check to see if a role is active should be performed higher up the chain here:

$userGroups = UserGroup::withUserIds([$user->getId()])
->withContextIds($context ? [$context->getId(), Application::SITE_CONTEXT_ID] : [Application::SITE_CONTEXT_ID])
->get()
->all();

and can be done so with

->withUserUserGroupStatus('active')

@asmecher, does this make sense to put this additional constraint on the role check in the policy here?

@ewhanson
Copy link
Collaborator

ewhanson commented Mar 6, 2025

Note: this will also need to be forwarded ported to main if this is the direction we choose to take.

@ipula
Copy link
Contributor

ipula commented Mar 6, 2025

@ewhanson Somehow it returns the all user groups. I have to do some small changes. now Authorization works fine

$userGroups = UserGroup::withUserIds([$user->getId()])
            ->withContextIds($context ? [$context->getId(), Application::SITE_CONTEXT_ID] : [Application::SITE_CONTEXT_ID])
            ->whereHas('userUserGroups', function ($query) use ($user) {
                $query->withUserId($user->getId())->withActive();
            })
            ->get()
            ->all(); 

@ewhanson
Copy link
Collaborator

ewhanson commented Mar 7, 2025

Hey @ipula, could you share some more details about what returns all user groups? When I tested the change I mentioned, it worked as expected and only checked the active user groups with the scope method on the model.

@ipula
Copy link
Contributor

ipula commented Mar 7, 2025

Its return the same user groups even I use ->withUserUserGroupStatus('active') did not filter the active ones. Then I used changed that I mention. I think ->withUserUserGroupStatus('active') didn't use the user id to filter from UserUserGroups

@withanage
Copy link
Member

Hi @Tribunal33

This change was implemented here and can be tested. https://github.com/ipula/pkp-
lib/blob/039dd1745ef7e8ebad5868e856de285f340a1da9/classes/security/authorization/UserRolesRequiredPolicy.php#L57

@Tribunal33
Copy link
Contributor Author

@withanage Don't know if this is related, but I'm not able to see any of the submissions now for any of the roles. Getting a console error.

Image

Image

@withanage
Copy link
Member

@Tribunal33 this does not seem to be related to the GDOR inviation or user roles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug:2:Major A bug found in common paths that reduces functionality for a large number of users
Projects
None yet
Development

No branches or pull requests

6 participants