azure-search-openai-demo icon indicating copy to clipboard operation
azure-search-openai-demo copied to clipboard

Allow public documents when authentication is enabled

Open mattgotteiner opened this issue 1 year ago • 8 comments
trafficstars

Purpose

  • Currently, when authentication is enforced there's no way to select "public" documents. Public documents are documents with neither oid or group ids associated with them.
  • Add AZURE_ALLOW_PUBLIC_DOCUMENTS env var which allows selecting these documents even if authentication is enabled.

Does this introduce a breaking change?

When developers merge from main and run the server, azd up, or azd deploy, will this produce an error? If you're not sure, try it out on an old environment.

[ ] Yes
[X] No

Does this require changes to learn.microsoft.com docs?

This repository is referenced by this tutorial which includes deployment, settings and usage instructions. If text or screenshot need to change in the tutorial, check the box below and notify the tutorial author. A Microsoft employee can do this for you if you're an external contributor.

[X] Yes
[ ] No

Type of change

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

Code quality checklist

See CONTRIBUTING.md for more details.

  • [X] The current tests all pass (python -m pytest).
  • [X] I added tests that prove my fix is effective or that my feature works
  • [X] I ran python -m pytest --cov to verify 100% coverage of added lines
  • [X] I ran python -m mypy to check for type errors
  • [X] I either used the pre-commit hooks or ran ruff and black manually on my code.

mattgotteiner avatar May 01 '24 19:05 mattgotteiner

Update docs

mattgotteiner avatar May 01 '24 19:05 mattgotteiner

Manual e2e test

mattgotteiner avatar May 01 '24 19:05 mattgotteiner

Manual e2e test

Local test succeeded. Manually tested the following:

  1. Unset oids, groups - publicly accessible
  2. Set oids, unset groups - not publicly accessible
  3. Unset oids, set groups - not publicly accessible

mattgotteiner avatar May 01 '24 21:05 mattgotteiner

Thanks @mattgotteiner :)

Just wondering, suppose below are my env settings:

AZURE_USE_AUTHENTICATION = True AZURE_ALLOW_PUBLIC_DOCUMENTS = True AZURE_ENFORCE_ACCESS_CONTROL = True

This would allow users to search for public documents without login? currently the chat text area field is disabled, and you need to login to continue. If I understand correctly the idea here it to allow authenticated users to chat with public documents, correct?

zedhaque avatar May 01 '24 22:05 zedhaque

Manual e2e test

Local test succeeded. Manually tested the following:

  1. Unset oids, groups - publicly accessible
  2. Set oids, unset groups - not publicly accessible
  3. Unset oids, set groups - not publicly accessible

deployed test succeeded as well

mattgotteiner avatar May 02 '24 00:05 mattgotteiner

Thanks @mattgotteiner :)

Just wondering, suppose below are my env settings:

AZURE_USE_AUTHENTICATION = True AZURE_ALLOW_PUBLIC_DOCUMENTS = True AZURE_ENFORCE_ACCESS_CONTROL = True

This would allow users to search for public documents without login? currently the chat text area field is disabled, and you need to login to continue. If I understand correctly the idea here it to allow authenticated users to chat with public documents, correct?

They still need to login (ENFORCE_ACCESS_CONTROL makes login require). The only purpose of this PR is to allow authenticated users to chat with public documents.

mattgotteiner avatar May 02 '24 00:05 mattgotteiner

Hm, I wonder if public would be clearer if it was "global"? public may imply logged out users (which Zed has a use case for).

AZURE_DEFAULT_DOCUMENT_ACCESS_IS_GLOBAL ??

Not very snappy! I'll sleep on it.

It'd also be great if we could provide smoke tests that verify the expected behavior to increase confidence, but I haven't setup Entra authenticated smoke tests before.

pamelafox avatar May 02 '24 04:05 pamelafox

Hm, I wonder if public would be clearer if it was "global"? public may imply logged out users (which Zed has a use case for).

AZURE_DEFAULT_DOCUMENT_ACCESS_IS_GLOBAL ??

Not very snappy! I'll sleep on it.

It'd also be great if we could provide smoke tests that verify the expected behavior to increase confidence, but I haven't setup Entra authenticated smoke tests before.

We don't need Entra authentication IMO - we just need to mock the search behavior or have a search instance for smoke tests

mattgotteiner avatar May 02 '24 15:05 mattgotteiner