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

JSON API: add support for login using JSON data #607

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Feb 13, 2025

Discovered while using the JSON API and failing to authenticate using curl --json. It took me a while to figure out this undocumented limitation especially since other JSON API POST endpoints expect the data to be sent as JSON.

The JSON API login endpoint delegates to the WebGateway LoginView logic which only support form data defined in the login form. This commit proposes to extend the LoginView.post() logic to also support authentication sent at JSON data.

There are a few ways to test this change:

This change should not affect the ability to log in using the login page or the JSON API with form data.

The JSON API login endpoint delegates to the WebGateway LoginView logic
which only support form data defined in the login form.
This commit proposes to extend the LoginView.post() logic to also support
authentication sent at JSON data.
Copy link
Member

@knabar knabar left a comment

Choose a reason for hiding this comment

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

I had not seen the len(request.POST) > 0 check before, but it should do what we want here. Code looks good to me.

@sbesson
Copy link
Member Author

sbesson commented Feb 17, 2025

I had not seen the len(request.POST) > 0 check before, but it should do what we want here.

I found through debugging this was one of the ways to distinguish between a HTTPRequest form data and JSON data. I am still not 100% happy with this approach which looks fragile and I am considering using the value of request.content_type instead. Thoughts?

@will-moore
Copy link
Member

If I try something invalid like:

r = session.post(login_url, json={'abc': 123})

I get a 500 html response (since I have DEBUG true) with local variable 'form' referenced before assignment at

        return self.handle_not_logged_in(request, error, form) 

This allows to use the consume the form validation logic when
the login request is sent as JSON
@sbesson
Copy link
Member Author

sbesson commented Feb 17, 2025

Thanks @will-moore, I hadn't tested the failing scenarios. After discussion with @knabar, pushed 42790d5 to 1- use the content type to detect JSON POST requests, 2- pass the JSON as a dictionary as an input to LoginForm. This significantly reduces the diff and means the login logic is largely unchanged and can use form.

ome/openmicroscopy#6420 should included updated integration tests for both valid and rejected JSON POST requests

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

Working fine, thanks.
Tested successful login with data and json and invalid login with data and json. All with expected results.

Copy link
Contributor

@kkoz kkoz left a comment

Choose a reason for hiding this comment

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

Works as expected

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.

4 participants