Added test for missing authentication token #5450
Conversation
|
Test looks fine, but can you look at other tests? Something wrong with them. |
klakhov
changed the title
|
/check |
|
❌ Some checks failed |
klakhov
changed the title
|
There is one failed test: actions_users/issue_1810_login_logout.js |
cvat-core/src/server-proxy.ts
Outdated
| // At first we check if authentication token is present | ||
| // Request in getSelf will provide correct authentication cookies | ||
| if (!store.get('token')) { | ||
| await logout(); |
There was a problem hiding this comment.
Can we just remoive cookies?
There is one big difference between server logout and removing cookies from the client.
In the first case we also logout all the authorized sessions on the server. Do you think it is expected behaviour?
There was a problem hiding this comment.
We can't just remove cookies. sessionid is protected by HttpOnly flag which prevents browser from reading/writing this cookie.
But I would agree that server logout is overkill in that situation, even if we cant remove cookie we can still redirect user to login page, so if he logins once again his session would be restored and no logout needed.
| @@ -18,7 +18,6 @@ context('When clicking on the Logout button, get the user session closed.', () = | |||
| } | |||
|
|
|||
| before(() => { | |||
| // TMP fix for login tests, need to change login logic with sessions | |||
There was a problem hiding this comment.
Should we remove also:
cy.clearAllCookies();
cy.clearAllLocalStorage();
???
There was a problem hiding this comment.
That paticular test depends on const task. Prepare to testing logins and creates task, and then user is loggined, and we cant login once again without logout, so we need to clear data before it. Frankly, im not sure why this test was passing before.
tests/cypress/e2e/issues_prs/pr_5331_missing_authentication_data.js
Outdated
Show resolved
Hide resolved
tests/cypress/e2e/issues_prs/pr_5331_missing_authentication_data.js
Outdated
Show resolved
Hide resolved
klakhov
changed the title
klakhov
changed the title
|
@bsekachev Could you look on this once again? |
<!-- Raised an issue to propose your change (https://github.com/cvat-ai/cvat/issues). It helps to avoid duplication of efforts from multiple independent contributors. Discuss your ideas with maintainers to be sure that changes will be approved and merged. Read the [CONTRIBUTION](https://github.com/cvat-ai/cvat/blob/develop/CONTRIBUTING.md) guide. --> <!-- Provide a general summary of your changes in the Title above --> ### Motivation and context <!-- Why is this change required? What problem does it solve? If it fixes an open issue, please link to the issue here. Describe your changes in detail, add screenshots. --> Related cvat-ai#5331 Added test, changed fix because of temporary solutions in cvat-ai#5344 ### How has this been tested? <!-- Please describe in detail how you tested your changes. Include details of your testing environment, and the tests you ran to see how your change affects other areas of the code, etc. --> ### Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. If an item isn't applicable by a reason then ~~explicitly strikethrough~~ the whole line. If you don't do that github will show an incorrect process for the pull request. If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [ ] I submit my changes into the `develop` branch - [ ] I have added a description of my changes into [CHANGELOG](https://github.com/cvat-ai/cvat/blob/develop/CHANGELOG.md) file - [ ] I have updated the [documentation]( https://github.com/cvat-ai/cvat/blob/develop/README.md#documentation) accordingly - [ ] I have added tests to cover my changes - [ ] I have linked related issues ([read github docs]( https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)) - [ ] I have increased versions of npm packages if it is necessary ([cvat-canvas](https://github.com/cvat-ai/cvat/tree/develop/cvat-canvas#versioning), [cvat-core](https://github.com/cvat-ai/cvat/tree/develop/cvat-core#versioning), [cvat-data](https://github.com/cvat-ai/cvat/tree/develop/cvat-data#versioning) and [cvat-ui](https://github.com/cvat-ai/cvat/tree/develop/cvat-ui#versioning)) ### License - [ ] I submit _my code changes_ under the same [MIT License]( https://github.com/cvat-ai/cvat/blob/develop/LICENSE) that covers the project. Feel free to contact the maintainers if that's a concern. --------- Co-authored-by: Maya <maya17grd@gmail.com>
Motivation and context
Related #5331
Added test, changed fix because of temporary solutions in #5344
How has this been tested?
Checklist
developbranchcvat-core, cvat-data and cvat-ui)
License
Feel free to contact the maintainers if that's a concern.