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

Allow to login into UI with token from api/v1/auth/login #2234

Merged
merged 12 commits into from Oct 9, 2020

Conversation

tdowgiel
Copy link
Contributor

@tdowgiel tdowgiel commented Sep 28, 2020

Motivation and context

Allow to automatically log into CVAT-UI by 3rd part apps on the same cluster

How has this been tested?

  1. In bash:
curl localhost:8080/api/v1/auth/login -X POST -H "Content-Type: application/json" -d '{"username": "some_name", "password": "some_password"}' -v # Get csrftoken & session id from the response
  1. Make URL: http://localhost:8080/auth/login-with-token/:sessionid/:csrftoken
  2. Paste it into a browser

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2020 Intel Corporation
#
# SPDX-License-Identifier: MIT

@coveralls
Copy link

coveralls commented Sep 28, 2020

Coverage Status

Coverage remained the same at 65.32% when pulling 24ceb3d on tdowgiel:login-ui-with-token into 678f5b9 on openvinotoolkit:develop.

@tdowgiel tdowgiel marked this pull request as ready for review September 28, 2020 09:49
@tdowgiel tdowgiel changed the title Allow to login into UI with token from api/v1/auth/login [WIP] Allow to login into UI with token from api/v1/auth/login Sep 28, 2020
@bsekachev
Copy link
Member

Hi, @tdowgiel
Is this PR ready? Can we review it?

@tdowgiel
Copy link
Contributor Author

tdowgiel commented Sep 30, 2020 via email

@bsekachev
Copy link
Member

@tdowgiel

Okay, nevertheless, let me comment some code-style issues and ideas about this PR.

@bsekachev
Copy link
Member

Also could you please increase minor version of cvat-ui (npm version minor in cvat/cvat-ui). You've checked this item in the PR description, but I don't see it is done

@tdowgiel tdowgiel changed the title [WIP] Allow to login into UI with token from api/v1/auth/login Allow to login into UI with token from api/v1/auth/login Oct 8, 2020

if ( cookies['sessionid'] && cookies['csrftoken']) {
window.location.reload();
<Redirect to="/tasks" />
Copy link
Member

Choose a reason for hiding this comment

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

I suppose redirect after reload is useless. Moreover it should be wrapped into return ();
Maybe something like window.location = window.location.origin; would be more suitable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason. I need to reload window to apply this changes. However, i have fixed issue with endless reloading when wrong token, by moving this to useEffect

@bsekachev
Copy link
Member

Hi, @tdowgiel, I hope you are doing well.

I tried this PR and it doesn't seem to be a working solution.
Maybe I did something wrong, but in this case could you please provide steps to test it? (I just copied sessionid and token from active session, made URL, pasted it, and browser infinitely reloads the page).

Also POST request to http://localhost:8080/api/v1/auth/login with credentials returns json with key field, which is actually used to authorization via cvat-core and stored in the local storage, but I do not see it is used somehow in this patch.

@bsekachev bsekachev changed the title Allow to login into UI with token from api/v1/auth/login [WIP] Allow to login into UI with token from api/v1/auth/login Oct 9, 2020
@tdowgiel
Copy link
Contributor Author

tdowgiel commented Oct 9, 2020

Hi, @tdowgiel, I hope you are doing well.

I tried this PR and it doesn't seem to be a working solution.
Maybe I did something wrong, but in this case could you please provide steps to test it? (I just copied sessionid and token from active session, made URL, pasted it, and browser infinitely reloads the page).

Also POST request to http://localhost:8080/api/v1/auth/login with credentials returns json with key field, which is actually used to authorization via cvat-core and stored in the local storage, but I do not see it is used somehow in this patch.

I have tested it in many ways. Response used in cvat-core is not only returning key but also setting cookie because of headers. Please look on example response and set-cookie header:

Date: Fri, 09 Oct 2020 13:35:16 GMT
Server: WSGIServer/0.2 CPython/3.8.3
Content-Type: application/json
Vary: Accept, Cookie, Origin
Allow: POST, OPTIONS
X-Frame-Options: DENY
Content-Length: 50
X-Content-Type-Options: nosniff
Referrer-Policy: same-origin
Set-Cookie: csrftoken=bAd9852nQZipwTvFnuELs29mdwowc3BGjuCsjpMfLrWT3txGVtaDp4jIboMBU1cq; expires=Fri, 08 Oct 2021 13:35:16 GMT; Max-Age=31449600; Path=/; SameSite=Lax sessionid=i8xqtviqysnfbi9n63rmhici3rb1nfo7; expires=Fri, 23 Oct 2020 13:35:16 GMT; HttpOnly; Max-Age=1209600; Path=/; SameSite=Lax

This is scenario:
I have access to the server with cvat. I want to integrate CVAT with my other webapp. And i want to share users between my app and cvat. Also core goal is to have user already logged in when he decide to use cvat.

  1. Create django user on CLI
  2. My backend send python request to cvat server to login django user
  3. CVAT django server returns key, but also mentioned before 'Set Cookie' params.
  4. User autologs in to CVAT thanks to /login-with-token/ parameters

@tdowgiel tdowgiel changed the title [WIP] Allow to login into UI with token from api/v1/auth/login Allow to login into UI with token from api/v1/auth/login Oct 9, 2020
import { Redirect, useParams } from 'react-router';
import { useCookies } from 'react-cookie';

export default function LoginWithTokenComponent(){
Copy link
Member

Choose a reason for hiding this comment

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

Please, add return type for the function JSX.Element

@@ -10,6 +10,7 @@ import notification from 'antd/lib/notification';
import Spin from 'antd/lib/spin';
import Text from 'antd/lib/typography/Text';
import GlobalErrorBoundary from 'components/global-error-boundary/global-error-boundary';
import LoginWithTokenComponent from './login-with-token/login-with-token';
Copy link
Member

Choose a reason for hiding this comment

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

Please, use absolute import: components/login-with-token/login-with-token, or put the line after all absolute (linter says that relative import should occur after absolute)

# Conflicts:
#	CHANGELOG.md
#	cvat-ui/package-lock.json
#	cvat-ui/package.json
@bsekachev
Copy link
Member

That's amazing, thanks for your patience and for the contribution!

@bsekachev bsekachev merged commit 2b221d2 into cvat-ai:develop Oct 9, 2020
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.

None yet

3 participants