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

IamMujuziMoses · 2024-03-28T19:53:57Z

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.

dkayiwa · 2024-03-28T23:18:48Z

api/src/main/java/org/openmrs/module/queue/utils/PrivilegeConstants.java

+ * AddOnStartup annotation.
+ *
+ * @see org.openmrs.annotation.AddOnStartup
+ * @since 1.8


https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477044/Java+Conventions#JavaConventions-NewPublicMethodsandClasses

I have never seen version 1.8 of this module 😊

Oh my bad, let me fix that, just to be clear, is 2.4.0 the current version?!

TamtoMefotieJunie

i think we can consider organizing the privilege constants in a logical order, such as alphabetically or by functionality, to improve readability and maintainability.

dkayiwa · 2024-04-04T11:33:51Z

@IamMujuziMoses can you always include a link to the ticket as advised at? https://wiki.openmrs.org/display/docs/Pull+Request+Tips

Is there a reason why you added privileges to only some of the service methods instead of all?

IamMujuziMoses · 2024-04-04T14:25:32Z

@dkayiwa the reason I had was that those methods that didn't get privileges called the methods that had privileges e.g. the QueueService method createQueue on which I didn't add privileges instead called saveQueue on which I added privileges, so I thought it wouldn't be necessary. Should I also add privileges on createQueue?!

dkayiwa · 2024-04-04T14:32:33Z

The method's internal implementation details should not affect its public contract. By looking at createQueue i should know the required privilges without looking at its implementation details. Not so? 😊

IamMujuziMoses · 2024-04-04T14:39:07Z

right, okay got it

…tication.

dkayiwa · 2024-04-04T15:07:55Z

right, okay got it

Can you do it also for other services in the module?

dkayiwa · 2024-04-04T22:09:49Z

api/src/main/java/org/openmrs/module/queue/utils/PrivilegeConstants.java

+@HasAddOnStartupPrivileges
+public class PrivilegeConstants {
+
+	// Add Privilege Constants


Do these comments add any value?

dkayiwa · 2024-04-11T16:01:25Z

api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java


-public interface QueueEntryService {
+public interface QueueEntryService extends OpenmrsService {


@IamMujuziMoses is extending OpenmrsService related to this ticket?

dkayiwa · 2024-04-11T19:10:34Z

api/src/main/java/org/openmrs/module/queue/api/QueueService.java

@@ -45,6 +49,7 @@ public interface QueueService {
 	 * @param queue the queue to be saved
 	 * @return saved {@link org.openmrs.module.queue.model.Queue}
 	 */
+	@Authorized({ PrivilegeConstants.ADD_QUEUES, PrivilegeConstants.EDIT_QUEUES })


Does the above require edit?

yes, I think since it delegates saveQueue(), it should also requires the same privileges.

dkayiwa · 2024-04-11T19:12:06Z

api/src/main/java/org/openmrs/module/queue/api/QueueService.java

@@ -53,16 +58,19 @@ public interface QueueService {
 	 * @param queue the queue to be saved
 	 * @return saved {@link org.openmrs.module.queue.model.Queue}
 	 */
+	@Authorized({ PrivilegeConstants.ADD_QUEUES, PrivilegeConstants.EDIT_QUEUES })


Could there be a reason why for this service you are doing Add and Edit and yet for others you are doing Manage?

saveQueue() is used to both update and add new queues, so different privileges must be required, ergo the Add and Edit.

but I have realized that Manage also provides both the Add and Edit privileges at once which is better, let me change that, thanks🙂

dkayiwa · 2024-04-11T20:09:19Z

api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java

@@ -135,5 +146,6 @@ String generateVisitQueueNumber(@NotNull Location location, @NotNull Queue queue
 	 * @return the previous queue entry, null otherwise.
 	 * @throws IllegalStateException if multiple previous queue entries are identified
 	 */
+	@Authorized({ PrivilegeConstants.GET_QUEUE_ENTRIES })


How about the rest of the methods in this interface?

do the methods like generateVisitQueueNumber(), getSortWeightGenerator() and setSortWeightGenerator() require privileges as well?!

Do you want them to be accessed without login?

Absolutely not. okay let me work on them.

dkayiwa · 2024-04-15T16:53:46Z

api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java

@@ -9,6 +9,8 @@
 */
 package org.openmrs.module.queue.api;

+import static org.openmrs.util.PrivilegeConstants.*;


https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477044/Java+Conventions#JavaConventions-Donotusewildcardimports

dkayiwa · 2024-04-15T21:12:39Z

api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java

 	void closeActiveQueueEntries();

 	/**
 	 * @return the instance of SortWeightGenerator that is configured via global property, or null if
 	 *         none configured
 	 */
+	@Authorized({ GET_GLOBAL_PROPERTIES })


This permission is too generic. Should anyone who can access global properties also have access to this?

dkayiwa · 2024-04-15T21:12:50Z

api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java

@@ -124,6 +143,7 @@ String generateVisitQueueNumber(@NotNull Location location, @NotNull Queue queue
 	 * 
 	 * @param sortWeightGenerator the SortWeightGenerator to set
 	 */
+	@Authorized({ MANAGE_GLOBAL_PROPERTIES })


Same as above.

dkayiwa · 2024-04-16T15:26:31Z

api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java

@@ -9,6 +9,9 @@
 */
 package org.openmrs.module.queue.api;

+import static org.openmrs.util.PrivilegeConstants.ADD_VISITS;


Do we need to import static?

Thought it was better than using canonical name in the @Authorized tag.

dkayiwa · 2024-04-16T15:29:04Z

api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java

 	Long getCountOfQueueEntries(@NotNull QueueEntrySearchCriteria searchCriteria);

 	/**
 	 * @param location
 	 * @param queue
 	 * @return VisitQueueNumber - used to identify patients in the queue instead of using patient name
 	 */
+	@Authorized({ ADD_VISITS, EDIT_VISITS })


Why not use the manage privilege for the items where the visit queue number is used?

After looking at the implementation details of this method, i think your privileges are correct. 😊

Instead of import static, how about directly using org.openmrs.util.PrivilegeConstants.ADD_VISITS and org.openmrs.util.PrivilegeConstants.EDIT_VISITS

dkayiwa · 2024-04-16T15:54:59Z

api/src/main/java/org/openmrs/module/queue/api/QueueRoomService.java

 	Optional<QueueRoom> getQueueRoomById(@NotNull int id);

+	/**
+	 * @return {@link List} of all queue rooms


Though this ticket was not about documenting these methods, you decided to do so and hence should also include the method description. 😊

dkayiwa · 2024-04-16T15:55:39Z

api/src/main/java/org/openmrs/module/queue/api/QueueRoomService.java


 public interface QueueRoomService {

+	/**
+	 * Gets a queue room given uuid.


given a uuid

dkayiwa · 2024-04-16T15:56:49Z

api/src/main/java/org/openmrs/module/queue/api/QueueRoomService.java

 	QueueRoom saveQueueRoom(@NotNull QueueRoom queueRoom);

+	/**


Same as above for the method description.

dkayiwa · 2024-04-16T15:57:14Z

api/src/main/java/org/openmrs/module/queue/api/QueueRoomService.java

 	List<QueueRoom> getQueueRooms(QueueRoomSearchCriteria searchCriteria);

+	/**
+	 * Voids a queue room


Void and Retire are not the same.

dkayiwa · 2024-04-16T15:58:00Z

api/src/main/java/org/openmrs/module/queue/api/QueueRoomService.java

+	 * Voids a queue room
+	 *
+	 * @param queueRoom the queue room to retire
+	 * @param voidReason the reason for voiding the queue room


It would be a retireReason instead of voidReason.

dkayiwa · 2024-04-16T15:58:30Z

api/src/main/java/org/openmrs/module/queue/api/RoomProviderMapService.java


 public interface RoomProviderMapService {

+	/**
+	 * Gets a room provider map given uuid.


given a uuid

dkayiwa · 2024-04-16T15:59:11Z

api/src/main/java/org/openmrs/module/queue/api/RoomProviderMapService.java

+	/**
+	 * Gets a room provider map by id.
+	 *
+	 * @param id roomProviderMapId - the id of the room provider map to retrieve.


Is the param id or roomProviderMapId?

dkayiwa · 2024-04-16T15:59:44Z

api/src/main/java/org/openmrs/module/queue/api/RoomProviderMapService.java

 	RoomProviderMap saveRoomProviderMap(@NotNull RoomProviderMap roomProviderMap);

+	/**


Description missing.

dkayiwa · 2024-04-16T15:59:56Z

api/src/main/java/org/openmrs/module/queue/api/RoomProviderMapService.java

 	List<RoomProviderMap> getAllRoomProviderMaps();

+	/**


Description missing.

dkayiwa · 2024-04-16T16:00:23Z

api/src/main/java/org/openmrs/module/queue/api/RoomProviderMapService.java

+	/**
+	 * Voids a room provider map
+	 *
+	 * @param roomProviderMap the room provider map to retire


retire and void are not the same.

…tication.

dkayiwa reviewed Mar 28, 2024

View reviewed changes

IamMujuziMoses force-pushed the O3-3002 branch from f62c000 to 87dde5e Compare March 29, 2024 00:26

TamtoMefotieJunie reviewed Mar 30, 2024

View reviewed changes

IamMujuziMoses force-pushed the O3-3002 branch from 87dde5e to 8791272 Compare March 31, 2024 09:22

O3-3002: Queue Module - REST endpoints can be accessed without authen…

aa52acb

…tication.

IamMujuziMoses force-pushed the O3-3002 branch from 8791272 to aa52acb Compare April 4, 2024 14:49

IamMujuziMoses requested a review from dkayiwa April 4, 2024 17:02

dkayiwa reviewed Apr 4, 2024

View reviewed changes

IamMujuziMoses force-pushed the O3-3002 branch from 4b47256 to c7cfd8c Compare April 5, 2024 08:17

IamMujuziMoses requested a review from dkayiwa April 5, 2024 08:21

dkayiwa reviewed Apr 11, 2024

View reviewed changes

IamMujuziMoses force-pushed the O3-3002 branch from c7cfd8c to df181af Compare April 11, 2024 18:08

IamMujuziMoses requested a review from dkayiwa April 11, 2024 18:12

dkayiwa reviewed Apr 11, 2024

View reviewed changes

IamMujuziMoses force-pushed the O3-3002 branch from df181af to e615b19 Compare April 11, 2024 19:55

IamMujuziMoses requested a review from dkayiwa April 11, 2024 19:57

dkayiwa reviewed Apr 11, 2024

View reviewed changes

IamMujuziMoses force-pushed the O3-3002 branch from e615b19 to 5c019fb Compare April 12, 2024 13:51

IamMujuziMoses requested a review from dkayiwa April 12, 2024 14:00

dkayiwa reviewed Apr 15, 2024

View reviewed changes

IamMujuziMoses force-pushed the O3-3002 branch from 5c019fb to fe0b9cb Compare April 15, 2024 21:08

IamMujuziMoses requested a review from dkayiwa April 15, 2024 21:09

dkayiwa reviewed Apr 15, 2024

View reviewed changes

IamMujuziMoses force-pushed the O3-3002 branch from fe0b9cb to dc3f09e Compare April 16, 2024 15:22

IamMujuziMoses requested a review from dkayiwa April 16, 2024 15:24

dkayiwa reviewed Apr 16, 2024

View reviewed changes

O3-3002: Queue Module - REST endpoints can be accessed without authen…

41855af

…tication.

IamMujuziMoses force-pushed the O3-3002 branch from dc3f09e to 41855af Compare April 16, 2024 18:30

IamMujuziMoses requested a review from dkayiwa April 16, 2024 18:35

dkayiwa merged commit 43117c9 into openmrs:main Apr 16, 2024
1 check passed

mseaton mentioned this pull request Apr 17, 2024

Revert "O3-3002: Queue Module - REST endpoints can be accessed without authentication." #70

Merged


		public interface QueueEntryService {
		public interface QueueEntryService extends OpenmrsService {

		RoomProviderMap saveRoomProviderMap(@NotNull RoomProviderMap roomProviderMap);

		/**

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

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

Conversation

IamMujuziMoses commented Mar 28, 2024 • edited Loading

Issue I worked on

Checklist: I completed these to help reviewers :)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamtoMefotieJunie left a comment

Choose a reason for hiding this comment

dkayiwa commented Apr 4, 2024

IamMujuziMoses commented Apr 4, 2024

dkayiwa commented Apr 4, 2024 • edited Loading

IamMujuziMoses commented Apr 4, 2024

dkayiwa commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IamMujuziMoses commented Mar 28, 2024 •

edited

Loading

dkayiwa commented Apr 4, 2024 •

edited

Loading

dkayiwa Apr 11, 2024 •

edited

Loading

dkayiwa Apr 11, 2024 •

edited

Loading

dkayiwa Apr 11, 2024 •

edited

Loading

dkayiwa Apr 16, 2024 •

edited

Loading