Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement combined Geoserver service and Workspace/Layer effective permissions #495

Merged
merged 37 commits into from
Feb 1, 2022

Conversation

fmigneault
Copy link
Collaborator

@fmigneault fmigneault commented Jan 10, 2022

Summary

First implementation that resolves most common use cases.
Since the changes are already substantial, start with this version that DOES NOT support multiple layers=<layer1>,<layer2> resources.

A subsequent PR can attempt resolution of combined resources and return only Allow if all resources are accessible.
That will involve modifying the (complicated) effective_permissions method.

Features / Changes

  • Add missing ServiceWFS permissions according to OGC WFS standard.
  • Add missing DescribeLayer permission to ServiceGeoserverWMS according to GeoServer WMS implementation.
  • Add support of specific hierarchy of Resource type Layer nested under Workspace for ServiceGeoserverWMS.
  • Add support of Resource type Layer under ServiceWFS.
  • Allow Resource and Service name to contain colon (:) character in order to define scoped names as it is often the case for Layer names.
  • Add child_structure_allowed attribute to Service implementations allowing them to define specific path-like structures of allowed Resource types hierarchies in order to control at which level and which combinations of nested Resource types are valid under their root Service. When not defined under a Service implementation, any defined Resource type will remain available for creation at any level of the hierarchy, unless the corresponding Resource in the tree already defined child_resource_allowed = False. This was already the original behaviour in previous versions.
  • Add GET /resources/{id}/types endpoint that allows retrieval of applicable children Resource types under a given Resource considering the nested hierarchy definition of its root Service defined by the new attribute child_structure_allowed.
  • Add child_structure_allowed attribute to the response of GET /service/{name} endpoint.
    For backward compatibility, resource_types_allowed parameter already available in the same response will continue to report all possible Resource types at any level under the Service hierarchy, although not necessarily applicable as immediate child Resource under that Service.
  • Add configurable attribute to Service types that supports custom definitions modifying their behaviour.
  • Add service_configurable to response of GET /service/{name} endpoint.
  • Adjust UI to consider child_structure_allowed definitions to propose only applicable Resource types in the combobox when creating a new Resource in the tree hierarchy.
  • Add UI submission field to provide Service JSON configuration at creation when supported by the type.

Bug Fixes

  • Remove invalid params_expected parameter from Service implementations (ServiceAccess, ServiceAPI,
    ServiceTHREDDS) that don't make use of it since they don't derive from ServiceOWS.
  • Fix base Permission definitions for all variants of WMS according to their reference implementations.
  • Remove multiple invalid schema path definitions that are not mapped against any concrete API endpoint.
  • Fix reporting of Service configuration for any type that supports it. Unless overridden during creation with a custom configuration, ServiceTHREDDS implementation would not report their default configuration and would instead return null, making it difficult to know from the API if default or no configuration was being applied for a given Service.
  • Fix Effective Resolution of Permission applied for ServiceGeoserverWMS to consider Scope modifier of Service and Workspace for access to be resolved at the Layer level.

@github-actions github-actions bot added api Something related to the API operations db Issues related to database connection, migration or data models doc Documentation improvements or building problem tests Test execution or additional use cases ui Something related to the UI operations or display labels Jan 10, 2022
dbyrns
dbyrns previously approved these changes Jan 11, 2022
Copy link
Contributor

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got some quick fixes, but overall this is a good PR.
@f-PLT, since I only did a static review, you should validate the implementation with a live geoserver instance so that it fits correctly our use cases before approving.
Thank you!

magpie/models.py Show resolved Hide resolved
magpie/services.py Outdated Show resolved Hide resolved
magpie/services.py Outdated Show resolved Hide resolved
magpie/services.py Outdated Show resolved Hide resolved
magpie/services.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the ci Something related to code tests, deployment and packaging label Jan 11, 2022
@fmigneault fmigneault self-assigned this Jan 20, 2022
@f-PLT
Copy link

f-PLT commented Jan 21, 2022

Tested with the public urls on a test instance and everything works very well and as expected with workspaces and layers.

I did find a small problem with WFS request parameters:
typeNames (version=2.0.0) vs typeName (version<=1.1.0)

We'll have to account for this either by allowing/filtering for both, whatever the WFS version request, or filter by WFS version.

However, Geoserver doesn't seem to care and will accept both for a request using version 2.0.0, which leads to the following security bypass:

Using the following configuration should give access to the workspace isolated, but restrict access to the routes layer

image

  • HOST_URL/geoserver/workspace-name/wfs?&version=2.0.0&request=GetFeature&typeNames=isolated:routes
    will return Access to service is forbidden, as expected

  • HOST_URL/geoserver/workspace-name/wfs?&version=2.0.0&request=GetFeature&typeName=isolated:routes
    will be successful

@fmigneault
Copy link
Collaborator Author

fmigneault commented Jan 22, 2022

Self-notes of things to adjust following feedback and discussions :

  • modify resource_param to support both typename(s) variants indiscriminately
  • add test validating use case where deny permission revokes access on Layer from allowed Workspace

- [ ] adjust GET /services/types/{}/resources to return contextual resource_types_permissions instead of the full service's permissions
- [ ] adjust services resource_types_permissions to apply relevant permissions (eg: GetCapabilities valid on WFS service, but not on Workspace and Layer)
see #499

- [ ] adjust UI to consider above and disable/grey-out non applicable permissions on resources by type
- [ ] realign permission title with comboboxes (ok on Firefox, but not on Chrome)
- [ ] consider http://jsfiddle.net/kunknown/VVaEq/2/ to horizontally scroll permissions titles/bboxes divs together /w max width
see #498

Copy link
Contributor

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got some questions along the road, but it looks really good!

docs/services.rst Show resolved Hide resolved
magpie/services.py Show resolved Hide resolved
magpie/services.py Show resolved Hide resolved
Copy link
Contributor

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Do you think that at some point magpie/services.py could be split? It starts to contains a lot of stuff.

@fmigneault
Copy link
Collaborator Author

@dbyrns

Do you think that at some point magpie/services.py could be split?

Could be but I don't see any advantage other than to reduce how long the file is.
There would need to be many indirect imports to ensure references still work (eg: SERVICE_TYPE_DICT imported from many spaces, and itself importing from many places...)

@f-PLT
Copy link

f-PLT commented Jan 28, 2022

I redid my tests for GetFeature and everything works well on my end.

I tested GetCapabilities a bit more in depth, directly on the workspace url (ex. host.domain/geoserver-service-public-url/workspace-name)

Whether I set the permission to Allow or Deny at the workspace level, the permission at the service level seems to take precedence.

image
With the above permissions, I still get an Access is forbidden.

With Service => Allow and Workspace => Deny, I can still make a GetCapabilities request on the specific workspace.

@fmigneault
Copy link
Collaborator Author

@f-PLT
Would be worth testing another permission because GetCapabilities is handled differently.
It makes senses only onto the service itself.
The service implementation stops even considering any other resource than the service itself in that case.

@dbyrns
Copy link
Contributor

dbyrns commented Jan 31, 2022

Looking at the screenshot posted by @f-PLT, it looks like the permissions don't line up properly with the comboboxes. Is it because of total number of permissions or because some of them seem to explose the allowed width (horizontal comboboxes)?

@f-PLT
Copy link

f-PLT commented Jan 31, 2022

Looking at the screenshot posted by @f-PLT, it looks like the permissions don't line up properly with the comboboxes. Is it because of total number of permissions or because some of them seem to explose the allowed width (horizontal comboboxes)?

Already has been made an issue by @fmigneault : #498

@fmigneault
Copy link
Collaborator Author

@dbyrns
Seems to be a problem in Chrome. I actually noticed only with screenshots by @f-PLT because they line up correctly in Firefox.

image

When there are many permissions though, the UI needs to be improved also because there no clean way to display them, lined-up or not (they wrap around on 2nd line).

image

@fmigneault fmigneault merged commit 074a288 into master Feb 1, 2022
@fmigneault fmigneault deleted the geoserver branch February 1, 2022 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Something related to the API operations ci Something related to code tests, deployment and packaging db Issues related to database connection, migration or data models doc Documentation improvements or building problem plugin Service plugin tests Test execution or additional use cases ui Something related to the UI operations or display
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants