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

treestatus: check for user authentication before require_auth0 API calls (Bug 1896642) #203

Merged
merged 2 commits into from
May 14, 2024

Conversation

cgsheeh
Copy link
Member

@cgsheeh cgsheeh commented May 14, 2024

In other parts of Lando, before making an API call that uses
the require_auth0 kwarg, we do an explicit check that the
user is authenticated and handle unauthenticated requests
appropriately. This was overlooked in implementing the Treestatus
UI and is resulting in errors in Sentry since we instead
only discover the missing credentials when making the request,
resulting in an unhandled exception. Add an explicit check
to each handler which makes an auth0_required API call.

…calls (Bug 1896642)

In other parts of Lando, before making an API call that uses
the `require_auth0` kwarg, we do an explicit check that the
user is authenticated and handle unauthenticated requests
appropriately. This was overlooked in implementing the Treestatus
UI and is resulting in errors in Sentry since we instead
only discover the missing credentials when making the request,
resulting in an unhandled exception. Add an explicit check
to each handler which makes an `auth0_required` API call.
@cgsheeh cgsheeh requested a review from zzzeid May 14, 2024 14:11
landoui/treestatus.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zzzeid zzzeid left a comment

Choose a reason for hiding this comment

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

There is still the issue of the incorrect validation that was causing the ValueError. We should file a separate bug for that and fix it as a P3.

@cgsheeh
Copy link
Member Author

cgsheeh commented May 14, 2024

There is still the issue of the incorrect validation that was causing the ValueError. We should file a separate bug for that and fix it as a P3.

That validation is happening at the Flask-WTForms level. This PR will convert the issue of invalid input into that form handler from an un-authed bug to an authed bug, which should be enough to resolve this, at least until we move to Django-Lando. We create other FlaskForm instances without error handling in other parts of Lando FWIW.

@cgsheeh cgsheeh requested a review from zzzeid May 14, 2024 15:06
@zzzeid
Copy link
Contributor

zzzeid commented May 14, 2024

There is still the issue of the incorrect validation that was causing the ValueError. We should file a separate bug for that and fix it as a P3.

That validation is happening at the Flask-WTForms level. This PR will convert the issue of invalid input into that form handler from an un-authed bug to an authed bug, which should be enough to resolve this, at least until we move to Django-Lando. We create other FlaskForm instances without error handling in other parts of Lando FWIW.

What I'm saying is that this ValueError appears to be new unhandled behaviour that is specific to the treestatus migration. As far as I can tell, no other endpoints appear to cause this error (including ones that initialize the form before validating it, similar to how this was before this PR). It should be tracked in a bug so we can fix it, either here or when it is ported to Django (hence P3). Validation errors should happen when form.is_valid() is called, not when it is initialized.

@cgsheeh cgsheeh merged commit b17885d into mozilla-conduit:main May 14, 2024
1 check passed
@cgsheeh cgsheeh deleted the check-auth branch May 14, 2024 15:19
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