-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: Require login #17
Conversation
############################ | ||
# Authentication | ||
############################ | ||
# The auth token used by the local endpoints | ||
API_AUTH_TOKEN=LOCAL_AUTH_12345678 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is leftover from the Flask API code we imported, deleted it to avoid any confusion.
app/src/cache.py
Outdated
|
||
|
||
def get_appconfig() -> AppConfig: | ||
global _app_config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To facilitate testing, consider minimizing use of global
by encapsulating globals in an object. Plus you can use @cached_property
(like this) or @property
to reduce the boilerplate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about the built in cache decorators-- updated the code to use this!
app/tests/src/test_login.py
Outdated
def test_require_login_no_password(monkeypatch): | ||
if "GLOBAL_PASSWORD" in os.environ: | ||
monkeypatch.delenv("GLOBAL_PASSWORD") | ||
src.cache._app_config = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear why this line is needed without examining the code. This is probably an artifact of using global variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment to the first instance of this in the file!
monkeypatch.delenv("GLOBAL_PASSWORD") | ||
src.cache._app_config = None | ||
|
||
require_login() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably, this could be replaced with something like app.init()
, which would call require_login()
. Depends on what you want to demonstrate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, ideally I'd like to have some kind of integration test of chainlit.py
. Maybe a future improvement?
Ticket
https://navalabs.atlassian.net/browse/DST-234
Changes
shared.py
so code isn't reconstructing expensive instances like AppConfig or SentenceTransformer unnecessarily. Maybe a better name would beshared.py
?login.py
that adds a login page whenGLOBAL_PASSWORD
andCHAINLIT_AUTH_SECRET
are setContext for reviewers
Testing
For local testing, set
GLOBAL_PASSWORD
andCHAINLIT_AUTH_SECRET
in your .env and restart the app (make start
). You may want to use incognito windows to avoid clearing your cookies each time.Screenshot showing integration with LiteralAI (when this API key is also set):