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

UI remove mention of "email" and request a username #933

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented May 31, 2024

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@zilto zilto requested a review from elijahbenizzy May 31, 2024 23:51
@zilto zilto marked this pull request as ready for review May 31, 2024 23:52
@skrawcz
Copy link
Collaborator

skrawcz commented Jun 1, 2024

does the backend assume an email? 🤔 or the SDK?

@zilto
Copy link
Collaborator Author

zilto commented Jun 3, 2024

does the backend assume an email? 🤔 or the SDK?

With this change UI change, I can create a new account with username=my_username and I'm able to run the examples/hamilton_ui/run.py and track results + display in the UI.

References to "email" in the code

The SDK had only one reference to email in a docstring. Otherwise, everything is under username which is a string type.

For the server, the concept of email is more common and it's a field of the User django model (ref). It will check for the input to be a valid email using regex. In our code, the user authentication during tracking assumes an email from the username passed via HTTP.

Next steps

From my manual testing, this UI change doesn't introduce any bugs for local mode, but a valid email might be required for enterprise features.

Potential solutions:

  1. Generate a valid email from the username string on the server side when in local mode.
  2. Add a UI element saying "Emails are stored within your application for authentication and never communicated externally."

IMHO, it still relevant to remove the "enter email" constraint for a locally hosted application. On the other hand, extremely popular self-hosted tools still require an email + password to enforce good security practices (e.g., NGINX proxy manager)

Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

So this will work -- the full auth system integration will break (it requires emails), but given that this is separate that's fine.

@zilto zilto merged commit 47e7fa3 into main Jun 4, 2024
27 checks passed
@zilto zilto deleted the ui/remove-email-parsing branch June 4, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants