Fix WMS layer access control check

[dmarteau]

Description

Fix access control when requesting layer's group with mixed allowed and forbidden layers from WMS requests.

Actual behavior: raise a security exception

Proposed behavior: consider only allowed layers in group.

The behavior for layers submitted to access control is now the following:

• Requesting a forbidden layer explicitely raise a security exception (unchanged behavior)
• Requesting a layer group that contain allowed layers and forbidden layers returns allowed layers and does not raise an exception anymore (changed behavior)
• Requesting a layer group that does not contains allowed layers is now a considered as non-existant group and returns error accordingly - which is consistent with the previous behavior (changed behavior)
• If no layers are requested explicitely, forbidden layers are discarded from the rendering list (unchanged behavior)

Implementation details:

Adding a layer to the rendering list is done by calling the addLayerToRender method with a boolean queryLayer context variable if the layer is added explicitely or not. If the layer is added explicetely, then a security exception is raised otherwie the layer is discarded from the rendering list.

[github-actions]

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 6236202)

[elpaso]

LGTM

[elpaso]

Can you please add a comment about what "queryLayer" means?

[rldhont]

Suggested change

	void addLayerToRender( QgsMapLayer *layer, bool queryLayer );
	bool checkLayerReadPermissions( QgsMapLayer *layer, bool queryLayer );
	/**
	* Adds the layer to the list of layers to be rendered if the layer is readable
	* Throws a QgsSecurityException if the layer is not readable and it is directly requested (queryLayer = true)
	*/
	void addLayerToRender( QgsMapLayer *layer, bool queryLayer );
	/**
	* Check layer read permissions
	* Returns True if the layer is readable
	* Returns False if the layer is not readable and it is not directly requested (queryLayer = false, the layer is in a group that has been requested)
	* Throws a QgsSecurityException if the layer is not readable and it is directly requested (queryLayer = true)
	*/
	bool checkLayerReadPermissions( QgsMapLayer *layer, bool queryLayer );

@elpaso are these comments suitable?

[elpaso]

That looks better, thanks. Just one thing that it is not clear to me in checkLayerReadPermissions documentation:
Returns False if the layer is not readable and it is not directly requested (queryLayer = false, the layer is in a group that has been requested) the text in parenthesis is not clear, shouldn't it be (queryLayer = false or the layer is in a group that has been requested)?

[rldhont]

@elpaso queryLayer == false if the layer is ina group that has been requested or the layer has not been directly requested by has to render.

[elpaso]

Ok, so I recommend to remove the queryLayer parameter completely and handle the throw in client code. It doesn't make sense to me to pass a bool to trigger a throw. You can return false and throw the exception in client code, or am I missing something?

[dmarteau]

Ok, so I recommend to remove the queryLayer parameter completely and handle the throw in client code.

This is not enough: you need to check if the layer is allowed the add it to the group or not without throwing exception depending on the context, so we need to create different methods to handle queryLayer == false cases. The construct was just convenient way to limit code verbosity.

Furthemore I was always told that it was not a good practice to handle control flows with exceptions in C++: if the parameter is a problem, then I would rather go with different methods similar to checkLayerPermission and addLayerToRender that do not throw.

[elpaso]

I just feel very weird that a method which is supposed to return a boolean throws or not depending on a passed in argument.

So, what I would recommend is to remove the queryLayer argument from checkLayerPermission(), call checkLayerPermission() from client code, get the boolean result and decide to throw or not from client code (where you know exactly if it was explicitely requested in query layers or not).

You can probably combine them with something like

if ( !checkLayerPermission( lyr ) && queryLayer ) ... throw

furthermore, I see that t is called three times and two of them it doesn't throw (queryLayers = false).