Metadata Mapping api access control

Regarding adding access control to the metadata mapping module api: https://issues.openmrs.org/browse/MAP-7

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?

I started a new topic for this issue as the earlier status/design thread is getting a bit hard to follow: Metadata Mapping Module development progress

We should have multiple different privilege levels.

Actually I’m not sure that reading a mapping should require any privilege at all (e.g. every user of the system will need to do this).

But there should be a privilege required to create/edit metadata mappings.

Ok, so we will do:

  • No privileges required for metadata item get methods

  • Privilege “Manage Metadata Mappings” required for managing metadata mappings and sources

For the latter, there seem to be separate add, edit and remove privileges in some cases but I can’t see a use case for such granularity.

@raff, does this look ok to you?

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.

One more thing to consider is that we have a helper method to fetch any metadata object i.e. getMetadataItem at https://github.com/openmrs/openmrs-module-metadatamapping/blob/master/api/src/main/java/org/openmrs/module/metadatamapping/api/MetadataMappingService.java#L341 Normally the VIEW_LOCATIONS privilege is required for getting a location, the VIEW_VISIT_TYPES privilege for a visit type, etc. For the purpose of getMetadataItem(s) I would suggest we have the “View Metadata” privilege.

@raff, thanks for being more specific. Works for me.

I took a closer look at the code and found two more issues we should address:

  1. 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.

  2. 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)?

@raff, any thoughts on my previous comment?

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.