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

fix JSON login example #6276

Merged
merged 5 commits into from
Apr 15, 2022
Merged

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Apr 28, 2021

What this PR does

Fixes #6274.
Fixes broken JSON api login example discussed at https://forum.image.sc/t/do-you-have-an-example-of/51887

EDIT: Also includes a big refactor of the JavaScript code to make it more readable and to improve usability of the example.

Testing this PR:

  • Serve the page locally:
$ cd openmicroscopy/examples/Training/javascript/
python -m http.server
  • Go to http://localhost:8000/ in a 'private' browser tab

  • Enter a server address to login to. This needs to be:

    • A server that is not https (cookies won't be set from https response to page hosted on http)
    • A server with CORS enabled (not merge-ci)
    • I have only managed to successfully test with a locally-hosted server
    • You need to enter the full base URL, e.g. http://localhost:4080/

Screenshot 2021-07-05 at 13 08 52

  • Click Prepare Login
  • This loads a page that contains the server address in the URL and shows a login page (with the various configured servers available)

Screenshot 2021-07-05 at 13 07 15

  • Enter login details and click Login
  • If successful, you should be logged-in and shown a list of the user's Projects.
  • Refreshing the page should again show the list of Projects (still logged in)
  • Clicking Log out should log you out and re-show the login form

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/do-you-have-an-example-of/51887/3

@jburel
Copy link
Member

jburel commented Jun 18, 2021

The example should probably be included in the doc. I cannot find anywhere.
We mention the Python/Java examples in https://github.com/ome/omero-documentation/blob/develop/omero/developers/json-api.rst

@will-moore
Copy link
Member Author

Did a big refactor to update the JavaScript code to use async functions instead of callbacks. Makes the code clearer.
Also moved util functions to utils.js and use fetch() to improve clarity.

@will-moore
Copy link
Member Author

@jburel Added link from docs in ome/omero-documentation#2187

@jburel
Copy link
Member

jburel commented Jul 7, 2021

Screenshot 2021-07-07 at 10 51 18

@jburel
Copy link
Member

jburel commented Jul 7, 2021

It works locally but I cannot get it to work using other servers

@jburel
Copy link
Member

jburel commented Jul 7, 2021

Using for example http://merge-ci.openmicroscopy.org/web/ and click prepare login does not direct me to the login page (Chrome)

Screenshot 2021-07-07 at 11 00 22

@jburel
Copy link
Member

jburel commented Jul 7, 2021

Same output when I use https://outreach.openmicroscopy.org/web/ (Chrome)

@jburel
Copy link
Member

jburel commented Jul 7, 2021

Same problem on Firefox

@will-moore
Copy link
Member Author

I think that the CORS issue above was due to cached data/js, but the CORS issue we're now seeing is because you need to host locally on https, for cookies to work (reading https://web.dev/when-to-use-local-https/)...
Going to try that...

Also, url = url + '/'; was failing when the trailing slash was missing (Fixed).
Another issue is that the URLs returned in the JSON responses don't include https even if that's the URL you use. e.g. https://merge-ci.openmicroscopy.org/web/api/ (https) returns URL http://merge-ci.openmicroscopy.org/web/api/v0/ (http).

@will-moore
Copy link
Member Author

will-moore commented Jul 7, 2021

Hosting locally under https as described https://web.dev/how-to-use-local-https/ using https://www.npmjs.com/package/http-server still gives insecure warnings in Chrome.

But the csrftoken cookie is still not set when coming from https. Need additional cookie flags SameSite=None and Secure. Did this locally (and will now apply to merge-ci):

$ pip install django-cookies-samesite
$ omero config append omero.web.middleware '{"index":0.1,"class":"django_cookies_samesite.middleware.CookiesSameSite"}'
$ omero config append omero.web.django_additional_settings '["SESSION_COOKIE_SAMESITE","None"]'
$ omero config append omero.web.django_additional_settings '["CSRF_COOKIE_SECURE","True"]'

EDIT - This breaks login on webclient! Reverting...

@will-moore
Copy link
Member Author

After updating merge-ci server with config above, I see the csrftoken cookie should be set with this header: set-cookie: csrftoken=cmeDT1SCkvFU16tW3wLWWmBj6h2AHrooyq4PoTXOuoBppQSBaeGEc8Oi6U5bjKaP; expires=Wed, 06-Jul-2022 12:39:13 GMT; Max-Age=31449600; Path=/; SameSite=None; Secure

The cookie is set, since subsequent requests include the cookie (and the same cookie is returned each time) BUT document.cookie is still empty (not accessible to JavaScript) so we can't access it to include in the x-csrftoken header.

Tried another approach to include the csrf token in the token response, so it's accessible to JS:

def api_token(request, api_version, **kwargs):
    """Provide CSRF token for current session."""
    token = csrf.get_token(request)

    from django.template import engines
    django_engine = engines['django']
    template = django_engine.from_string("Hello {% csrf_token %}")
    html = template.render({}, request)
    return {"data": token, "html": html}

but that csrf token is different from what's in the csrftoken cookie and changes on each refresh, same as the token which was already there (and doesn't work for POST etc).

Don't know what else to try. So it seems like we don't have a way to support https servers. :(

@will-moore
Copy link
Member Author

@jburel So, I didn't manage to get it working with https, but at least if it works with http then it's an improvement on the current example code (which is actually broken), so it would still be good to get this PR merged.

@jburel
Copy link
Member

jburel commented Sep 2, 2021

We switched to https and many did do. So not having working with https is not great. Any progress since last time you looked at the issue

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/do-you-have-an-example-of/51887/12

@jburel
Copy link
Member

jburel commented Apr 15, 2022

Comment added to the issue #6274

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.

Fix JSON API JavaScript login example
3 participants