Fix configuration-specific CORS issues & cleanup session endpoints

Joshua-Douglas commented 6 months ago

Closes #633, and adds frontend/backend test suites that will be very useful for completing #577 (API Authentication).

Rationale behind the changes?

This change ensures that user sessions work for all same-origin frontend and backend deployment configurations.

Before this change the session and refresh endpoints would only work with our frontend app if the VITE_HUU_API_BASE_URL configuration values were set to exactly match the web application's base url.

Web App Base Url	VITE_HUU_API_BASE_URL	Did Sessions Work?
127.0.0.1:4040	localhost:8080	No
localhost:4040	127.0.0.1:8080	No
127.0.0.1:4040	127.0.0.1:8080	Yes
localhost:4040	localhost:8080	Yes

This misconfiguration was causing users to get logged out anytime the application reauthenticated the session or reloaded the page. The session endpoints failed to authenticate users because our session cookies were not being sent, due to browser cross-origin security policies.

Despite the fact that 127.0.0.1 and localhost resolve to the same value in our case, browser same-origin policies consider 127.0.0.1 and localhost as two different domains. Since our backend does not set CORS headers, these same-origin policies were preventing cookies from being shared with the backend.

See #633 for a more in depth discussion of the root cause.

What changes did you make?

To fix this brittle configuration I updated the base query to use a relative API URL, which is always considered 'same-origin' by the browser. It is worth noting that we do use a proxy to forward the requests from the frontend to the backend, but same-origin policies do not apply to the proxy - they only apply within the browser.

Here is the key fix:

// app/src/services/api.ts
const baseQuery = fetchBaseQuery({
-  baseUrl: import.meta.env.VITE_HUU_API_BASE_URL,
+ baseUrl: '/api',

Other, Related Changes

While making these changes, I also refactored the frontend CORS related code and session endpoints. Most of these changes were driven by minor bugs discovered during testing.

Correct the default API URL. Previously it was set to http://localhost:4040/api/, but the real location is http://localhost:8080/api/.
Update development server API proxy to work with any backend origin, by using the corrected API URL value to set the proxy target
Remove unnecessary 'Access-Control-Allow-Origin' headers from the API requests. These headers have no effect when added to requests. They are designed to be attached to responses.
Correct session endpoint OpenAPI spec to apply JWT authentication. The JWT was being passed, but never used
Correct the refresh & session endpoints OpenAPI specification to require a request session cookie, and to document the 401 error
Correct signin endpoint OpenAPI spec to define the response json & header. Also, apply a format constraint on the email input and define the 401 error

Testing done for these changes

Frontend

To test these changes I added a suite of Cypress e2e test cases that support a real and mocked backend. See the updated app README for full instructions on how to run the e2e test cases without mocking.

The e2e test cases verify that sessions work by ensuring that the user stays logged in during a page refresh, and by checking for the signin request cookies.

Backend

The backend test infrastructure does not support AWS Cognito mocking, so the suite of backend authentication tests are configured to only run in release mode. I focused on checking failure states since the frontend test suite covers the success cases. The plan is to eventually expand the backend suite to include successful logins, but this will require some additional modifications to our test infrastructure.

erikguntner commented 5 months ago

@Joshua-Douglas looking at the code and the changes look good. However, I tried out the branch on my machine and I'm having trouble logging in. If I try to log in using the email: testuser@email.com and password: Test1234! like I would prior to the configuration changes, then I get this error:

raise NotImplementedError("The current application configuration does " NotImplementedError: The current application configuration does not support AWS cognito. In the future we will mock this functionality to enable for all configurations. This feature is planned in Issue #577

And if I try to log in using Google I get this screen:

My .env has ENV='development'. Maybe I have my configuration wrong. What credentials have you been using to sign in to the app?

Joshua-Douglas commented 5 months ago

Hey @erikguntner,

Can you try the staging configuration instead? AWSCognito features are temporarily disabled on the development configuration while we wait on mocking.

Bright side is that I have a branch with mocking enabled so we should be able to use the development configuration soon!

Let me know if you are still having issues while using staging.

erikguntner commented 5 months ago

@Joshua-Douglas staging config worked! Thanks!

agosmou commented 5 months ago

Hi @Joshua-Douglas

Successfully reproduced the error in #633 per Joshua's instruction and then checked out his branch.

Signed in as testuser@email.com and the opened a fresh (cleared) "Network" side-panel. I refreshed the page and a bunch of failed refresh and session popped up. How come this happens?

Tests

API

api/tests/test_authentication.py passed on my machine

Cypress

app/cypress/e2e/test-authentication.cy.ts ran as npm run cypress:open

chrome
firefox

app/cypress/e2e/test-authentication.cy.ts ran as npm run cypress:open:nomock

chrome

Joshua-Douglas commented 5 months ago

Hey @agosmou,

You outlined several items in your last post, so I'll address them separately. Thanks for the great questions! I had to do a bit of research to answer.

test failures

I see the same test case keeps failing for you. I wasn't able to replicate on my machine, but it looks like a timing issue because the signin attempt succeeds but the url is incorrect.

I updated the test case to include assertion retries (similar to the other test cases) in 99a35b1. Give it a try and let me know if it is still failing.

Why the failed `session` and `refresh` calls during initial app load

This is by design. We use the API to check for a valid user session each time the app loads.

When the app loads it checks to see if the user is already logged in by querying the API session endpoint (see main.tsx::HuuApp(), it is called within the useEffect). The session query always fails because the API session cookies are currently designed to expire when the tab closes. If we decided to use persistent cookies in the future, then the session call here may succeed if the session cookie is stored in the browser.

refresh is called because of the logic we have in api.ts::baseQueryWithReAuth. This function defines the query logic that is used each time the API is queried. The logic here will attempt to refresh the user JWT anytime an authentication failure is encountered.

I think the session failure is a good design and we should keep it. The logic within baseQueryWithReAuth is subject to debate, but outside the scope of this issue. For example we could either of these two alternative designs to reduce the number of failed queries: 1) Instead of refresh each time a 401 is encountered, we can update the API to return an 'expired token' error code and only refresh when that error code is encountered. 2) We could design the API to automatically refresh expired JWTs if the request contains a valid refresh token. With this design the frontend wouldn't need to worry about token refreshes.

Why the failed `user` & `session` calls during refresh?

This is also by design.

We store the user data and JWT in memory as a redux store. We store the refresh token as a session cookie. When the web app refreshes while the user is logged in a few relevant things occur: 1) The redux store is cleared (and the session cookie is not cleared) 2) The HuuApp useEffect will call the session endpoint 3) The ProtectedRoute will reauthenticate the user by calling the user endpoint

Since the JWT has been cleared the user endpoint query will fail until the JWT is refreshed using the session cookie. We could probably optimize this by checking for the JWT before attempting authenticated queries, but making this change would require significant changes since the authAPI doesn't have a way to distinguish between queries that require a JWT and those that don't. So I think this optimization is out of scope.

Why is the `session` endpoint called twice each time?

The session is called within a React useEffect. We use React.StrictMode. Strict mode will automatically call useEffect functions twice for development builds as a tool to alert developers when the effect is not properly cleaning up after itself.

So the duplicate session call will not occur in production builds. Source

Joshua-Douglas commented 5 months ago

Hey @erikguntner, @agosmou,

PR ready for re-review in response to your comments. I'll keep the e2e infrastructure in mind as I finish up my next two auth PRs!

No rush on the review since December is our month off!

agosmou commented 4 months ago

@Joshua-Douglas

I think we're good to merge. Good job and thanks for being thorough

test failures

Having issues running cypress with my laptop. I'll try again when I have a different comp - I think for now we can assume it's good. These will run in Github Actions in the future or locally? Code change looks promising! Used your changes as reference to familizarize myself with the differences based on cypress [docs](https://docs.cypress.io/api/commands/then#:~:text=then()%20allows%20you%20to,no%20assertions%20throw%20within%20it.)

Why the failed session and refresh calls during initial app load?

Thanks for explaining the session and refresh logic. If this is by design, it's probably best to leave as-is for now, than to implement the alternative approaches. I can put some thought into this though - approach 1 being nice since we just edit the frontend, but approach 2 might be proper since it moves the load to the backend. hmm...

Why the failed user & session calls during refresh?

Understood and agreed! Thanks

Why is the session endpoint called twice each time?

Thank you for providing the docs for this. TIL!

erikguntner commented 4 months ago

@Joshua-Douglas This all looks great to me!

I think the session failure is a good design and we should keep it. The logic within baseQueryWithReAuth is subject to debate, but outside the scope of this issue. For example we could either of these two alternative designs to reduce the number of failed queries:

Instead of refresh each time a 401 is encountered, we can update the API to return an 'expired token' error code and only refresh when that error code is encountered.

We could design the API to automatically refresh expired JWTs if the request contains a valid refresh token. With this design the frontend wouldn't need to worry about token refreshes.

I've been thinking about what you outlined in 1 as well. I think this would be a good update.

Merging!

hackforla / HomeUniteUs