I could not find any documentation on the “big picture” of access control in OpenMRS but I guess we have two possible easy-to-implement designs for this module:
a single module based privilege that gives access to all api methods (kind of like metadata sharing module’s {{MetadataSharingConsts.MODULE_PRIVILEGE}})
separate privileges for managing metadata mappings and reading metadata mappings
I would personally go for the latter design.
However, understanding the big picture of access control would be interesting when thinking about MetadataMappingService#getMetadataItem. The implementation goes around all api level access control related to the metadata item in question. For example, normally you might get a Location via LocationService#getLocation(java.lang.Integer) but metadata mapping module loads it directly from the database thus skipping privilege checks. I don’t know why this would be problem as we are only talking about reading metadata. But still, Location api probably requires privileges for a reason, right?
To be specific we should require authentication for get methods (annotate them with @Authorized without specifying privileges) and the “Manage Metadata Mapping” privilege for write access.
I took a closer look at the code and found two more issues we should address:
There is an existing privilege “Metadata Mapping” that does not seem to be used but is created (via config.xml). I suggest we drop this privilege (via liquibase) and use the new and more specific “Manage Metadata Mappings” instead.
The pre 1.1.0 methods in MetadataMappingService, like isAddLocalMappingToConceptOnExport(), do not have any authorization rules. I suggest we apply the same privileges on these methods. On the other hand, would this affect users of Metadata Sharing Module, as it depends on Metadata Mapping Module (and MetadataMappingService)?
I think both changes you suggest make a lot of sense. Metadata Sharing users are unlikely to be affected, because most of them are administrators anyway thus they have all privileges. It is best to fix all methods as you suggest.