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

[IOPID-536] : Migration from cookies to session storage #10

Merged
merged 11 commits into from
Jul 27, 2023

Conversation

claudiodall
Copy link
Contributor

@claudiodall claudiodall commented Jul 25, 2023

Short description

As discussed in the meeting, we are not allowed to store tokens and user info inside the cookie. The purpose of these fixes is to change the logic where the cookies were intercepted from the header of the request in the middleware. Since the middleware has no chance to get data from session storage, we migrated the logic to the session provider to retrieve the session storage from there.

List of changes proposed in this pull request

  • Removed logic from middleware.
  • Added session provider.
  • Added IdP (Identity Provider) list selection to /access (SPID) to enable login functionality.
  • Updated routes.

How to test

Try to log in through /access or /logoutInit using the SPID and check if the token and user object are stored in session storage instead of cookies.

To log in, use hub-spid-login as SP or force it with the following link+token:

http://localhost:3000/validateSession#token=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6Imp3dF84Njo3NDoxZTozNTphZTphNjpkODo0YjpkYzplOTpmYzo4ZTphMDozNTo2ODpiNSJ9.eyJlbWFpbCI6InBpcHBvQHRlc3QuZW1haWwuaXQiLCJmYW1pbHlfbmFtZSI6InF3ZXJ0eSIsImZpc2NhbF9udW1iZXIiOiJRV1JQUFA4MEEwMUg1MDFGIiwibmFtZSI6InBpcHBvIiwiZnJvbV9hYSI6ZmFsc2UsInVpZCI6IjgzODQzODY0LWYzYzAtNGRlZi1iYWRiLTdmMTk3NDcxYjcyZSIsImxldmVsIjoiTDIiLCJpYXQiOjE2ODk2OTI4ODcsImV4cCI6MTY4OTcyNTI4NywiYXVkIjoiYXBpLmRldi5zZWxmY2FyZS5wYWdvcGEuaXQiLCJpc3MiOiJTUElEIiwianRpIjoiXzk0ZDJmZTYyMDQ2NTUyODRjMGRjIn0.EDbsdpQgXlJSzyVgRqZy7yuUILe5FUlaerC3n1gv6SQrNvljXJxgm3GTv0912UQ6VV85e4oxGgc4LrcvpyLYZcgVe-5-2gNfbYNIPbIWqicaX4GPucQrSq47H0NEIaAv6-3qI2l1IhdH--72zUls_911RoAg_JdINr7em0vxy7wEoqjWOxgEsfQhEauT8oyRV6dDDied5zA9YQPy7a7KlhvI6juwS4sCdFnaonNzhBcZnqW4qzpec2NaAb1xJuHnnTp_tdMz6zExEhupeopmYdtIzYHUvxfohHr1L7eDRzi5RKEUfnRIldajfQEX_NUL9UTU4EkCcwUs-rSIKskQog

@claudiodall claudiodall requested a review from a team as a code owner July 25, 2023 16:12
@github-actions
Copy link
Contributor

Jira Pull Request Link

This Pull Request refers to the following Jira issue IOPID-536

Copy link
Member

@shadowsheep1 shadowsheep1 left a comment

Choose a reason for hiding this comment

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

A couple of suggestions ;)

src/app/[locale]/(pages)/logoutInit/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(pages)/access/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(pages)/logoutInit/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(pages)/validateSession/page.tsx Outdated Show resolved Hide resolved
src/app/[locale]/_component/selectIdp/idpList.tsx Outdated Show resolved Hide resolved
src/app/[locale]/_component/selectIdp/idpList.tsx Outdated Show resolved Hide resolved
src/app/[locale]/_component/selectIdp/selectIdp.tsx Outdated Show resolved Hide resolved
src/app/[locale]/_component/sessionProvider.tsx Outdated Show resolved Hide resolved
src/app/[locale]/_hooks/useLogin.tsx Outdated Show resolved Hide resolved
src/app/[locale]/_hooks/useToken.tsx Outdated Show resolved Hide resolved
@claudiodall claudiodall requested a review from shadowsheep1 July 26, 2023 15:39
Copy link
Member

@shadowsheep1 shadowsheep1 left a comment

Choose a reason for hiding this comment

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

Added few suggestions

src/app/[locale]/_hooks/useLogin.tsx Outdated Show resolved Hide resolved
src/app/[locale]/_component/sessionProvider.tsx Outdated Show resolved Hide resolved
src/app/[locale]/(pages)/access/page.tsx Show resolved Hide resolved
src/app/[locale]/(pages)/logoutInit/page.tsx Show resolved Hide resolved
Copy link
Member

@shadowsheep1 shadowsheep1 left a comment

Choose a reason for hiding this comment

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

LGTM

@claudiodall
Copy link
Contributor Author

After accepting the hint to use isBrowser() in useToken hook, I realized that the utility function wasn't automatically imported.

After rebuilding the solution, I noticed that the useEffect in the session provider was being triggered too many times due to the various changes made. To avoid this problem, I had to change the dependency array items, we had also introduced pathName instead of window.location.

After testing, everything now seems to be working fine.
Additionally, I took the opportunity to fix the authorization type in sessionProvider using a discriminated union, as advised.

I kindly request a final review before proceeding with the merge. Thank you!

Copy link
Member

@shadowsheep1 shadowsheep1 left a comment

Choose a reason for hiding this comment

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

From a static code check POV, LGTM!

@claudiodall claudiodall merged commit 73777ad into master Jul 27, 2023
@shadowsheep1 shadowsheep1 deleted the IOPID-536-switching-to-session-storage branch August 30, 2023 12:13
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.

3 participants