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

Add alias to GET /conversations/{cnv} endpoint for LH devices #2682

Merged
merged 3 commits into from
Sep 9, 2022

Conversation

smatting
Copy link
Contributor

@smatting smatting commented Sep 8, 2022

This PR adds an alias for the endpoint

GET     /conversations/{cnv}

at

GET     /legalhold/conversations/{cnv}

and permits legalhold devices access to it.

We use an alias instead of allowing GET /conversations/{cnv} because the nginz acl files are not expressive enough to match only this endpoint (and not e.g. POST /conversations/join)

Checklist:

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Update nginz config in helm: charts/nginz/values.yaml
  • Update nginz config in the demo: deploy/services-demo/conf/nginz/nginx.conf

@smatting smatting temporarily deployed to cachix September 8, 2022 13:00 Inactive
@smatting smatting temporarily deployed to cachix September 8, 2022 13:00 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 8, 2022
@smatting smatting requested a review from fisx September 8, 2022 13:07
@smatting smatting requested a review from jschaul September 8, 2022 13:19
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

If the intention is only to give legalhold access to one thing (read some information), wouldn't it be better to create another (alias) endpoint /legalhold/conversations/{cnv} and whitelist only that? That would restrict what legalhold can do.

@smatting
Copy link
Contributor Author

smatting commented Sep 8, 2022

If the intention is only to give legalhold access to one thing (read some information), wouldn't it be better to create another (alias) endpoint /legalhold/conversations/{cnv} and whitelist only that? That would restrict what legalhold can do.

Good idea! @dkovacevic, would that work for you?

@smatting smatting temporarily deployed to cachix September 8, 2022 17:51 Inactive
@smatting smatting temporarily deployed to cachix September 8, 2022 17:51 Inactive
@smatting smatting force-pushed the allowlist-lhclients-conv-endpoints branch from 75449ac to eef25f1 Compare September 9, 2022 08:58
@smatting smatting temporarily deployed to cachix September 9, 2022 08:58 Inactive
@smatting smatting temporarily deployed to cachix September 9, 2022 08:58 Inactive
@smatting smatting changed the title Add /conversation/* endpoints to allowlist for LH devices Add alias GET /conversations/{cnv} endpoint for LH devices Sep 9, 2022
@smatting smatting requested a review from jschaul September 9, 2022 09:05
@smatting smatting changed the title Add alias GET /conversations/{cnv} endpoint for LH devices Add alias to GET /conversations/{cnv} endpoint for LH devices Sep 9, 2022
@smatting smatting merged commit e5ea9bd into develop Sep 9, 2022
@smatting smatting deleted the allowlist-lhclients-conv-endpoints branch September 9, 2022 10:57
@jschaul
Copy link
Member

jschaul commented Sep 12, 2022

If the intention is only to give legalhold access to one thing (read some information), wouldn't it be better to create another (alias) endpoint /legalhold/conversations/{cnv} and whitelist only that? That would restrict what legalhold can do.

Good idea!

Thanks for creating the alias!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants