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

cloud.gov login integration #35

Merged
merged 35 commits into from
Mar 4, 2024
Merged

cloud.gov login integration #35

merged 35 commits into from
Mar 4, 2024

Conversation

akuny
Copy link
Contributor

@akuny akuny commented Feb 21, 2024

Changes

  • Add integration with cloud.gov identity provider to protect Flask routes that concern application logic
  • Authentication dependencies are isolated to a AuthenticationImplementation class
  • Add User domain entity, SQLAlchemy model, database migration

Considerations

  • No role-based authorization logic yet. Since there are two distinct user roles, each with a distinct set of needs, I was thinking we could build this out simply using a "role" attribute on the User entity
  • As a consequence of the point above, users are not assigned a role upon registration at this time
  • Having to go through MFA for local development has been not-ideal but there are apparently some workarounds to consider
  • We could also use login.gov's sandbox environment as a second oauth provider

@akuny akuny requested a review from danielnaab February 21, 2024 19:28
USER appuser
CMD ["/bin/sh", "./scripts/start_local.sh"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure on current best practices, but you could speed up the container start-up by precompiling the bytecode as the root user. https://kence.org/2022/05/23/python-bytecode-and-read-only-containers/


new_user = User(
email=email,
username=email.split("@")[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a separate user name? I might vote for omitting it. Or maybe this is a "display name"?

Also, I think this isn't a problem with the way the function is currently called, but I think the email address should probably be validated here.

@akuny akuny changed the title coud.gov login integration cloud.gov login integration Mar 4, 2024
@akuny akuny merged commit 2180570 into main Mar 4, 2024
1 check passed
@akuny akuny deleted the login-integration branch March 4, 2024 19:50
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.

2 participants