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

O3-3002: Queue Module - REST endpoints can be accessed without authentication. #73

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

IamMujuziMoses
Copy link
Contributor

Issue I worked on

see https://issues.openmrs.org/browse/O3-3002

Checklist: I completed these to help reviewers :)

  • Added Authorized annotations to the service classes: QueueService, QueueEntryService, QueueRoomService and RoomProviderMapService.
  • Created PrivilegeConstants class to contain all privilege names and their descriptions.
  • Added liquibase changesets that will add the new privileges and do a default assignment of these privileges to roles.

@IamMujuziMoses IamMujuziMoses force-pushed the O3-3002-III branch 2 times, most recently from 818cdb7 to 26cd748 Compare May 7, 2024 16:53
<preConditions onFail="MARK_RAN">
<not>
<sqlCheck expectedResult="0">
SELECT count(*) FROM role_privilege WHERE privilege='Get Visits';
Copy link
Member

Choose a reason for hiding this comment

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

I would think the more appropriate check here, would be to execute this only if no one already has the "Get Queue Entries" privilege (which would imply it has already previously been assigned as desired). Then, this also allows you to get rid of the "AND NOT EXISTS..." clause from your insert statement below, right?

The same sort of logic would be applied to all of the changesets below. So:

  • If any role already has privilege A, then mark as RAN
  • Otherwise, assign privilege A to any role that already has privilege B.

</not>
</preConditions>
<comment>Add "Manage Queue Entries" privilege to the roles having "Edit Visits"</comment>
<sql>
INSERT INTO role_privilege (role, privilege)
SELECT role, 'Manage Queue Entries' from role_privilege rp
WHERE rp.privilege = 'Edit Visits'
Copy link
Member

Choose a reason for hiding this comment

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

If you are going to do this, I think you need to ensure you only insert the distict roles from both, so a single sql statement, something like:

INSERT INTO role_privilege (role, privilege)
            SELECT distinct role, 'Manage Queue Entries' from role_privilege rp
            WHERE rp.privilege in ('Add Visits', 'Edit Visits');

</not>
</preConditions>
<comment>Add "Manage Queue Rooms" privilege to the roles having "Edit Visits"</comment>
<sql>
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above, you need to use something like:

INSERT INTO role_privilege (role, privilege)
            SELECT distinct role, 'Manage Queue Rooms' from role_privilege rp
            WHERE rp.privilege in ('Add Visits', 'Edit Visits');

Copy link
Member

@mseaton mseaton left a comment

Choose a reason for hiding this comment

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

This looks ok to me now, thanks! Hopefully no further post-commit issues.

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.

None yet

2 participants