Configuration of the user profile authorized to import metadata

[josegar74]

This change allows to configure in the settings the user profile authorized to import metadata: Editor, Reviewer or Administrator:

When the logged user has a lower profile than the one configured in the previous setting:

• In the Contribute menu, the option to Import metadata is not displayed.

• the services that handle the metadata import return an error: The user has no permissions to import metadata.

[ianwallen]

I don't think using ordinal() is the best way to compare the profiles.
It is simply based on the order the enum was created which seems like it could cause issues in the future.

Have you considered using RoleHierarchy?

Which is already configured in config-security-core.xml

core-geonetwork/web/src/main/webapp/WEB-INF/config-security/config-security-core.xml

Lines 292 to 303 in 80080c3

	<bean id="roleHierarchy"
	class="org.springframework.security.access.hierarchicalroles.RoleHierarchyImpl">
	<property name="hierarchy">
	<value>
	Administrator > UserAdmin
	UserAdmin > Reviewer
	Reviewer > Editor
	Editor > RegisteredUser
	RegisteredUser > Guest
	</value>
	</property>
	</bean>

Here is some information on using it.
https://stackoverflow.com/questions/44263972/spring-security-method-to-check-if-a-user-has-a-hierarchical-role

In this case it could check to see if the user has access to the minimal role.
This would also support cases where the Role Hierarchy configuration would change in the future.

[josegar74]

I can check that, but I'm not really sure that is really accurate, I mean afaik UserAdmin > Reviewer is not the case, or even Reviewer > Editor (a Reviewer gets assigned automatically in the UI the Editor role, but it's not hierarchical as in old versions of GeoNetwork).

[ianwallen]

If it is no longer a hierarchy then I'm guessing that there are some inconsistencies in the system. I suspect that locations that are using spring security (i.e. hasAuthority('Editor') will include reviewers). So maybe the hierarchy needs to be updated.

The odd thing is that this PR seems to expect this hierarchy to exists by using the profile ordinal(). So the logic does not seems to be consistent? I don't believe this PR should be assuming hierarchy that does not exists? Maybe it should allow a list of groups to be specified.

I would think the hierarchy that is defined should be the one that is used across the system. So if someone used the existing settings then editor would include reviewer. If we remove the Reviewer > Editor from the config then it would mean that they would need to specify both editor and reviewer

That is just my opinion :)

[fxprunayre]

The hierarchy exists but there are some more precise role checks made by the API operations. At least, I think of:

• a user can be Reviewer in group A and Editor in group B. But the main user profile will be the highest. AccessManager.hasEditingPermissionWithProfile takes care of those checks
• UserAdmin hasAuthority on User/Group/Formatter/Harvester API but needs to also have Editor/Reviewer role in a group to create/import records.

[ianwallen]

re: a user can be Reviewer in group A and Editor in group B.
In that case shouldn't the user have the editor role and reviewer role? (There is currently odd logic in the user profile update that automatically adds the user to the editor role when the user is added to a reviewer - I personally don't believe that logic should exists based on the current spring security hierarchy)
If the user belongs to both roles then hasAuthority('Editor') will detect that they are an editor and then AccessManager.hasEditingPermissionWithProfile will apply the fine grain check to ensure that they are an editor for the current metadata.

re: UserAdmin hasAuthority on User/Group/Formatter/Harvester API but needs to also have Editor/Reviewer role in a group to create/import records.
It seems like you want to use the following hierarchy? This would ensure that admins have all roles and other have registered users access and there is no relationship between UserAdmin, Reviewer and Editor.

Administrator > UserAdmin 
Administrator > Reviewer 
Administrator > Editor
UserAdmin > RegisteredUser 
Reviewer > RegisteredUser
Editor > RegisteredUser 

Functions like "isEditorOrMore" is odd - it implies as hardcoded hierarchy. It should just check for editor privs and the spring security hierarchy will determine if the user has editor privileges or not based on the users assigned roles and on the configured spring security hierarchy.

The main point I'm trying to make is that there seems to be some inconsistencies in the way the roles are handled - in some cases it is assuming the spring security hierarchy and it other cases it is a hard coded hierarchy. My preference would be update all the role checks to be based on spring security hierarchy so that it is easy for someone to change the spring security hierarchy configuration to meet their requirements
In our case, we prefer that all reviewers are editors like currently defined in spring security hierarchy. It seems like geonetwork has been hard coded to use multiple security hierarchy - the one defined in spring security and a hard coded version that is similar to the one I mentioned above. This has cause confusion on my end when initially trying to understand how the geonetwork security is configured.

[fxprunayre]

It seems like you want to use the following hierarchy? This would ensure that admins have all roles and other have registered users access and there is no relationship between UserAdmin, Reviewer and Editor.

Indeed that would clarify the UserAdmin role. The current hierarchy comes from old Jeeves times which was not supporting this kind of links by role.

[josegar74]

@ianwallen I updated the pull request to use the hierarchy of roles, the change of the hierarchy configuration if sounds fine can be managed as a separate pull request?

[ianwallen]

@josegar74 I think that look better - thank you for making the change.

I have not tested locally but the code look good.

[fxprunayre]

Merged using CLI b43cff2 and 598ad13. Not sure what was wrong with this branch. @josegar74 can you double check and close if all fine ? thanks.