guardian / grid

The Guardian’s image management system
https://www.theguardian.com/info/developer-blog/2015/aug/12/open-sourcing-grid-image-service
Apache License 2.0
1.44k stars 120 forks source link

actually enforce that Guardian users have at `grid_access` permission #4271

Closed twrichards closed 4 months ago

twrichards commented 4 months ago

https://github.com/guardian/grid/pull/4270 added logging for users without any permission whilst we can get everybody's permissions in good shape.

What does this change?

This makes use of the hasAnyGridPermission val we created in #4270 for the result of the validateUser function. We also override the showUnauthedMessage function to return a more useful 403 response...

image

NOTE: as with #4270 this should be no-op for other organisations using Grid (since they'll be plugging their own AuthorisationProvider or relying on the default implementation of the key function hasBasicAccess which returns true).

How should a reviewer test this change?

Assuming https://github.com/guardian/permissions/pull/186 is deployed to CODE then deploy this branch to TEST main (or run locally) and experiment accessing grid with and without some form of grid permission (incl. new grid_access permission), observe the logs and you should see something like the image above.

How can success be measured?

adhering better to our security principles guardian/recommendations@b4746d4/README.md#security-principles

Who should look at this?

@guardian/digital-cms @guardian/newsroom-resilience @RichardLe @AndyKilmory @Conalb97

Tested? Documented?

github-actions[bot] commented 4 months ago

Deploy build 12488 to TEST

All deployment options - [Deploy build 12488 to TEST](https://riffraff.gutools.co.uk/deployment/deployAgain?project=media-service%3A%3Agrid%3A%3Aall&build=12488&stage=TEST&updateStrategy=MostlyHarmless&action=deploy) - [Deploy parts of build 12488 to TEST by previewing it first](https://riffraff.gutools.co.uk/preview/yaml?project=media-service%3A%3Agrid%3A%3Aall&build=12488&stage=TEST&updateStrategy=MostlyHarmless)

From guardian/actions-riff-raff.

twrichards commented 4 months ago

In https://github.com/guardian/grid/pull/4270 we logged both whether the user accessing has grid_access permission and if not whether they have at least some other level of permission. There was only one user in the last two weeks who had the latter, so this PR actually simplifies to definitely require grid_access permission (see https://github.com/guardian/permissions/pull/186 - noting that grid_access permission is granted automatically based on some group membership)

CC @andrew-nowak given https://github.com/guardian/grid/pull/4270#pullrequestreview-2048062942

prout-bot commented 4 months ago

Seen on auth, usage, image-loader, metadata-editor, thrall, leases, cropper, collections, media-api, kahuna (merged by @twrichards 8 minutes and 33 seconds ago) Please check your changes!