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

[PUI] Session authentication #6970

Merged
merged 40 commits into from
Apr 17, 2024

Conversation

SchrodingersGat
Copy link
Member

@SchrodingersGat SchrodingersGat commented Apr 7, 2024

This PR provides session based authentication for the react frontend. A number of backend changes are also required for this to work correctly. A lot of testing has been performed to ensure that it covers various login / logout edge cases.

Major Features

  • Remove API token support from frontend entirely
  • ApiImage now just uses regular <img> tag (and works!)
  • Attachments can now be downloaded from PUI frontend
  • Attachments can now be uploaded successfully in PUI frontend
  • Opens up pathway for label and report printing in PUI frontend
  • Custom implementation of /api/auth/login/ to prevent login failure with stale sessionid cookie
  • Fix backend issue resulting from our custom token implementation
  • Refactor / simplify frontend authentication implementation

- Remove API token functions
- Simplify cookie approach
- Add isLoggedIn method
- Existing (invalid) session token causes 403
- Point to the right host
- Simplify code
- Now we use session cookies, so it *Just Works*
- Now works with remote host
@SchrodingersGat SchrodingersGat added api Relates to the API Platform UI Related to the React based User Interface labels Apr 7, 2024
@SchrodingersGat SchrodingersGat added this to the 0.15.0 milestone Apr 7, 2024
Copy link

netlify bot commented Apr 7, 2024

Deploy Preview for inventree-web-pui-preview canceled.

Name Link
🔨 Latest commit b8cfe8c
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/661fadea23bc590008834966

@SchrodingersGat
Copy link
Member Author

@matmair I was playing around with #6399 - trying to get it to work reliably. In the end (and after reading way too many docs on cookies / cross site stuff) I have discovered that we need some backend changes too. So, I have written this new implementation which replaces #6399

This has been tested with attachment upload / download and handles it all quite cleanly (as far as I have been able to test)

Let me know what you think!

@SchrodingersGat
Copy link
Member Author

@matmair can you give some insight into the playwright tests failing?

Copy link
Contributor

@wolflu05 wolflu05 left a comment

Choose a reason for hiding this comment

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

I never used session authentication with a SPA. I always used api tokens. How does this session cookie work when the backend is hosted somewhere else than the frontend? E.g. like the netlify previews with the demo server. Do there need to be something configured specifically?

src/frontend/src/components/items/AttachmentLink.tsx Outdated Show resolved Hide resolved
Co-authored-by: Lukas <76838159+wolflu05@users.noreply.github.com>
@SchrodingersGat
Copy link
Member Author

I never used session authentication with a SPA. I always used api tokens. How does this session cookie work when the backend is hosted somewhere else than the frontend? E.g. like the netlify previews with the demo server. Do there need to be something configured specifically?

The host needs to have the correct CORS and CSRF settings configured. I have tested that this works on the devcontainer setup (which is effectively a CORS request from localhost:5173 to localhost:8000). It works well there and also in the "compiled" version when the javascript files are served statically from localhost:8000.

The current netlify previews won't work because the demo server code needs updating (based on this PR). But once that is updated it will work fine

@wolflu05
Copy link
Contributor

wolflu05 commented Apr 7, 2024

So you need to explicitly configure the netlify domain in the demo server then? Or is there a wildcard allowed currently on the demo server?

@SchrodingersGat
Copy link
Member Author

The demo server already allows cross origin requests from the netlify preview site

@matmair
Copy link
Contributor

matmair commented Apr 8, 2024

To me, the frontend tests make it look like the username field are not available. @SchrodingersGat Have you checked that the tests run on your dev setup?

@SchrodingersGat
Copy link
Member Author

@matmair I cannot run these tests locally, I believe that it is due to incompatibility with the alpine devcontainer. I get errors like:

Error: browserType.launch: Failed to launch: Error: spawn /home/vscode/.cache/ms-playwright/firefox-1440/firefox/firefox ENOENT

@matmair
Copy link
Contributor

matmair commented Apr 8, 2024

It seems to run on my dev setup, I will try to fix this CI on the weekend

@SchrodingersGat
Copy link
Member Author

It seems to run on my dev setup, I will try to fix this CI on the weekend

Thanks @matmair , much appreciated.

@matmair
Copy link
Contributor

matmair commented Apr 16, 2024

Looks like it is working better this way - maybe increasing the timeouts might help a bit

@SchrodingersGat
Copy link
Member Author

Still not wrapping my head around these tests :|

@matmair
Copy link
Contributor

matmair commented Apr 17, 2024

@SchrodingersGat it seems like there are problems with login on tests that do not specifically cover logins. Maybe we can bypass logging in there similar to #7022 to solve this? Testing login once specifically should be enough.

@SchrodingersGat
Copy link
Member Author

Woo! It works, finally :) Thanks for the help with the tests @matmair

@SchrodingersGat SchrodingersGat merged commit 0ba7f7e into inventree:master Apr 17, 2024
26 of 28 checks passed
@SchrodingersGat SchrodingersGat deleted the session-auth branch April 17, 2024 11:35
@matmair
Copy link
Contributor

matmair commented Apr 17, 2024

Seeing that coveralls coverage for PUI was disabled here - should we discuss #6986?

matmair added a commit to matmair/InvenTree that referenced this pull request Apr 17, 2024
@SchrodingersGat SchrodingersGat mentioned this pull request May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Relates to the API Platform UI Related to the React based User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PUI] Use sessions backend for login [PUI] How to download attachments and reports
3 participants