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

Fix client refresh and url redirection #281

Merged
merged 15 commits into from
Jul 21, 2023

Conversation

timbot1789
Copy link
Contributor

@timbot1789 timbot1789 commented Jun 27, 2023

The goal of this PR is to remove the # parameter in the PASS url so we are able to use server-side redirects. These redirects will make it easier to have multiple different login flows (e.g. existing vs new user). Along the way we rebuilt and simplified the login and routing flow of the app.

I have a production build of this running up at https://279-remove-hash-from-url--splendorous-nougat-3f0062.netlify.app

That should let you test prod behavior without difficulty.

A few changes in this PR:

  1. Removed @inrupt/solid-ui-react and added the components we use as native code. We really only use this library as a convenient way to manage the session provided by solid-client-authn-browser. However, that auth library is fairly buggy, and we need to implement a few workarounds to get it running properly. Having it hidden behind a react library makes that difficult.
  2. Updates solid-client-authn-browser to get a fix for restorePreviousSession. This allows the solid client to handle session restoration for us, and makes the silent authentication that happens on page refresh faster. This also made some code redundant, so I removed that. Less code ≈ fewer bugs.
  3. Added code in vite.config.js to force vite to build solid-client as a ES module instead of a CommonJS bundle. This is a workaround for this bug: ESM distro for @inrupt/solid-client-authn-browser inrupt/solid-client-authn-js#3001
  4. Renamed the routes folder to pages to keep it consistent with how the files are referred to internally.
  5. Removes the hash router and replaces it with browser router. This is to enable server redirection and other features used by PASS login. The main use case for this right now is in user registration. To register a user, we need them to log into a pod. That requires sending a redirect URL to the pod server. If the user registration page is hosted on www.pass.com/PASS/#/signup, the redirect link could be www.pass.com/PASS/#/signup, and when we get back to that page with a valid login, we continue with any user registration flow. However, URLs with a # in them are not valid redirect links. Sending such a link to the server will cause an error. If we instead put signup at www.pass.com/signup, we can redirect to it without issue after we've finished logging into the pod.

NOTE: This will break github code pages, but we can deploy the project to another site without too much difficulty, We will need to do that at some point anyway.

  1. Updated routes so that they don't have # in them anymore. Routes in this PR are:
  • www.pass.com - login/landing page
  • www.pass.com/clients
  • www.pass.com/documents
  • www.pass.com/messages
  • www.pass.com/profile
  1. Added mappings of a lot of the src modules in vite.config.js so they have absolute references. This makes it easier to refer to them throughout the project, since they will always have the same path no matter what folder you're in.

No new unit tests because this doesn't implement a new feature, just refactors. The fact that there's no regression in the unit tests should demonstrate that the refactor is working correctly.

We could add some unit tests for routes, but that can be in a future PR.

@timbot1789 timbot1789 changed the base branch from Master to Development June 27, 2023 00:02
@timbot1789 timbot1789 force-pushed the 279/remove-hash-from-url branch from 14d70c7 to ccac512 Compare June 27, 2023 17:04
// Constants Imports
import { ENV } from '../../constants';

const OidcLoginComponent = () => {
const [oidcIssuer, setOidcIssuer] = useState(ENV.VITE_SOLID_IDENTITY_PROVIDER);
const redirectUrl = useRedirectUrl();
const defaultOidc = ENV.VITE_SOLID_IDENTITY_PROVIDER || '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were getting a controlled component error in dev console if the ENV didn't set this value. This guard cleans up the error.

@xscottxbrownx
Copy link
Contributor

xscottxbrownx commented Jun 27, 2023

I haven't looked at this at all yet, but if it does anything to help the redirects or logged in ? check - then I'm all for it. Constantly get frustrated with getting stuck at splash/intro page of PASS and nothing happens when clicking login. I mean I never log out of solid, it should be able to see that I am logged into solid and bypass the intro screen altogether I would think.

@timbot1789
Copy link
Contributor Author

@xscottxbrownx correct, we are able to do that right now. If you leave the site without logging out and then later return (within a certain amount of time), you will automatically log back in. We've actually had that feature for a while, but this fixes some things and makes the refresh a bit faster.

The more significant change is the removal of the hash router.

I think I've updated the PR description since your comment to make things a bit clearer.

@timbot1789 timbot1789 linked an issue Jun 28, 2023 that may be closed by this pull request
@timbot1789 timbot1789 force-pushed the 279/remove-hash-from-url branch 2 times, most recently from ec57c85 to 80695e9 Compare July 4, 2023 20:09
@leekahung
Copy link
Contributor

leekahung commented Jul 7, 2023

Hey @timbot1789, just want to check in on this PR. After this updating this, are there plans to migrate hosting services from GitHub Pages?

If we are, I think I know of a good alternative. I've been using Netlify before for some of my personal projects and it seems to work great with BrowserRouter and integrates well with GitHub repositories.

We just need to take into account Netlify specific files like netlify.toml for setting things up, but it should be relatively simple from my experience with that platform. Let me know what you think. Think we could speak with Hugh about getting Netlify integrated to our project if we decide to pursue this route.

<CssBaseline />
<ThemeProvider theme={theme}>
<UserDataContextProvider>
<BrowserRouter>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a bit curious, is there a reason for moving the Router from the uppermost later to just before Layout compared to before where the Router is wrapping around everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong reason, just thought it looked cleaner. Based the idea off of bulletproof react:

https://github.com/alan2207/bulletproof-react/tree/master

Which just contains a lot of good react programming patterns, and does the routes and provider structure that way.

@timbot1789
Copy link
Contributor Author

Hey @timbot1789, just want to check in on this PR. After this updating this, are there plans to migrate hosting services from GitHub Pages?

If we are, I think I know of a good alternative. I've been using Netlify before for some of my personal projects and it seems to work great with BrowserRouter and integrates well with GitHub repositories.

We just need to take into account Netlify specific files like netlify.toml for setting things up, but it should be relatively simple from my experience with that platform. Let me know what you think. Think we could speak with Hugh about getting Netlify integrated to our project if we decide to pursue this route.

Yeah I've been looking at a few different hosting services. Whatever hosting service we go with (or if we need one at all) is something we can talk about in a dev meeting. Netlify is definitely a good option.

Haven't been pushing this because there's a bug in the PR now that I can't track down. The routing works when I run the dev server, but fails when I do the production build and preview it with npm run preview. It seems to be an issue with the inrupt auth library, but not 100% sure.

@timbot1789
Copy link
Contributor Author

timbot1789 commented Jul 10, 2023

@leekahung I did some more research, and to clarify, the delay in the PR is due to this bug in the solid-client-authn-browser: inrupt/solid-client-authn-js#3001

Basically, the most recent versions of the authn library aren't compatible with vite, and break when passed through vite build. I'll investigate some workarounds, but really it would need to be fixed in the library itself.

@timbot1789 timbot1789 force-pushed the 279/remove-hash-from-url branch from 80695e9 to e856a6b Compare July 10, 2023 18:47
@timbot1789 timbot1789 force-pushed the 279/remove-hash-from-url branch from e856a6b to 7368c22 Compare July 10, 2023 19:07
public/_redirects Outdated Show resolved Hide resolved
src/index.jsx Outdated Show resolved Hide resolved
netlify.toml Outdated
@@ -0,0 +1,4 @@
[[redirects]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a netlify config file. The [[redirects]] settings tell the netlify server to let react handle the routing.

netlify.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@leekahung leekahung left a comment

Choose a reason for hiding this comment

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

I believe I've gone over all the files. The underlying code seems to be working fine for redirects and refreshing.

One thing I've noticed is that the auto-populate for adding webId to clients is gone in the production build (things work as intended on the local build on my machine).

Screen.Recording.2023-07-10.at.18.30.05.mov

Should be fixed as soon as we got the environment variables set up in Netlify. Outside of that, I think I'm ready to approve this. Think it's ready for our switch from GitHub Pages to another hosting service.

@timbot1789
Copy link
Contributor Author

@leekahung Sorry, I should clarify about the netlify. That's just a demo environment that I set up to show that the code will work in a certain environment. It's not a final PASS prod environment. I mean to compare it to github pages. If you push this branch to github pages and test it there, it will fail.

That netlify environment isn't PASS'S. It's my own on my own account. It's not even using code taken from the PASS repo. I don't have permission to have netlify deploy off of PASS itself; github blocks my connection request. Instead, I made an identical fork and pushed it up.

If we want to use netlify as a prod environment, we'll need to set it up and configure it outside this PR. I just want to show that we can use it.

@leekahung
Copy link
Contributor

Ah okay. No worries. I believe we could get @htharker42 to help us with getting the service set up at least with regards to GitHub integration?

@timbot1789
Copy link
Contributor Author

Ah okay. No worries. I believe we could get @htharker42 to help us with getting the service set up at least with regards to GitHub integration?

Yeah we can talk about it in a dev meeting

@timbot1789 timbot1789 force-pushed the 279/remove-hash-from-url branch from 6931164 to cbf325f Compare July 18, 2023 22:16
@@ -54,6 +53,7 @@
"last 3 edge versions"
],
"devDependencies": {
"@rollup/plugin-node-resolve": "^15.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 3 dev dependencies are used to get the @hooks, @components, etc... mappings working. This one runs on the prod build, the eslint ones run in eslint.

@timbot1789 timbot1789 requested a review from leekahung July 20, 2023 16:22
Copy link
Contributor

@leekahung leekahung left a comment

Choose a reason for hiding this comment

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

This is fine. I'm okay with merging this.

@Jared-Krajewski Jared-Krajewski self-requested a review July 20, 2023 20:43
@leekahung
Copy link
Contributor

Ah right, we'll have to adjust to using Heroku after speaking with Hugh. I'm open to reviewing this again after Heroku related configs are set up.

@leekahung leekahung self-requested a review July 20, 2023 22:57
@timbot1789
Copy link
Contributor Author

Ah right, we'll have to adjust to using Heroku after speaking with Hugh. I'm open to reviewing this again after Heroku related configs are set up.

I'd like to get this merged as soon as possible. I think there are useful changes here that other developers would benefit from having access to. And the current codebase is already beginning to diverge from the code in this PR. We can put up the heroku config in a separate PR once Hugh gets done setting that up. It's not super complex. I wrote one when I was looking at Heroku for this PR and it's only a few lines at most. If Hugh doesn't get back to us before the next time we want to deploy to prod, I can just put a fork of the project up on my own heroku account and we can use that for a little while.

@leekahung
Copy link
Contributor

Ah right, we'll have to adjust to using Heroku after speaking with Hugh. I'm open to reviewing this again after Heroku related configs are set up.

I'd like to get this merged as soon as possible. I think there are useful changes here that other developers would benefit from having access to. And the current codebase is already beginning to diverge from the code in this PR. We can put up the heroku config in a separate PR once Hugh gets done setting that up. It's not super complex. I wrote one when I was looking at Heroku for this PR and it's only a few lines at most. If Hugh doesn't get back to us before the next time we want to deploy to prod, I can just put a fork of the project up on my own heroku account and we can use that for a little while.

I'm good to merge as is now if you think it's ready. If there's other devs you think should need to review the code, I'll wait for their approval first.

Copy link
Contributor

@xscottxbrownx xscottxbrownx left a comment

Choose a reason for hiding this comment

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

Went over this PR in discord VC - I had already looked over most files, but was a little hung up on the SessionContext and useSession hook, but good now.

@timbot1789 timbot1789 merged commit 414a09b into Development Jul 21, 2023
@timbot1789 timbot1789 deleted the 279/remove-hash-from-url branch July 21, 2023 02:56
leekahung pushed a commit that referenced this pull request Jul 21, 2023
* update routing to remove hash parameter and improve speed of refresh
leekahung pushed a commit that referenced this pull request Jul 21, 2023
* update routing to remove hash parameter and improve speed of refresh
timbot1789 added a commit that referenced this pull request Aug 3, 2023
* testing the styling of pagination per their documentation

* margin for PREV and NEXT pagination buttons

* styling PREV and NEXT differently than rest

* Adding chevron icons to prev and next buttons

* deleting yellow option for messages pagination

* Removed unused state.file from statusReducer, initial state for reducer, and action type for CLEAR_FILE; Included optional prop for file in FormSection; Included file for StatusNotification in place of state.file; Updated lines for UploadDocumentModal; Updated relevant typedefs

* Grouping all MUI imports together

* Issue 220 sort client list (#275)

* sort function in client list table

* takes away unused variable

* copy and sort userList copy

* delete console log

* run prettier check

* sort by family name and case fix

* fix unexpected console statement

* changed compare to arrow function

* formatting chage

* unit test added

* format changes

* testing template for russell

* wrapped component with providers attempt

* fix eslint errors

* prettier run fix attempt

* clean up test

* sort clients test added

* formatted client list table test

* sort clients test redo

* prettier format fix error

---------

Co-authored-by: Andy Williams <[email protected]>
Co-authored-by: Tim Standen <[email protected]>

* updated instructions for creating a branch and minor formatting changes

* further formatting to branching instructions

* updated terminal instructions for cloning and creating a branch

* added link to figma board in resources.md

* updated figma board to view only link

* updated instructions for CLI checking out branch

* Created new route to ClientsProfile; Created ClientsProfile page; Updated fetchProfileInfo to accept other webIds; Modified Link to Pod Profile in Clients to go to new ClientsProfile page; Updated handleSelectClient to fetch client profile data

* Create new ClientProfileInfo component for ClientsProfile page

* Increased size of profile image for client profile

* Navbar Messages route tab -> icon button

* Modified Profile page to incorporate parts of Documents page; Updated ProfileImageField component and form element styling; Minor change to DocumentTable margin

* Created new UserDocumentListContext exclusively for user documents in profile page; Created new prop for user='personal' to indicate user documents context for UploadDocumentModal, DocumentTable, and DocumentTableRow components

* Combining ClientsProfile with Profile page from PR #322; Created new prop for user in Profile page to differentiate between user and client profiles as done in PR #322; Removing unused ClientsProfile

* removing second dropdown menu in navbar for tabs/routes (no longer necessary with less routes)

* fix for navbar width

* Updated typedefs for relevant components; Included user prop to distinguish between set and request permissions for Documents and document files between Client and User Profiles; Placed dummy functions for request permissions forms in client profile page; Removed Documents page and route; Modified WebID columns in ClientListTable to Client Profile

* Updating unit test for ClientListTable to call the correct item

* Utilized user to set value and placeholder for set permission forms depending on condition

* Fix to display Tabs on mobile too

* adding same spacing for all icons

* Show verify file toggle only for clients profile page

* Made client/user distinction more clear by changing DocumentListContext to ClientDocumentListContext; Updated relevant files dependent on DocumentListContext to ClientDocumentListContext; Updated the context provider for DocumentListContext to ClientDocumentListContext

* Removing comment for webId column in ClientListTableRow

* Fix client refresh and url redirection (#281)

* update routing to remove hash parameter and improve speed of refresh

* Fix client refresh and url redirection (#281)

* update routing to remove hash parameter and improve speed of refresh

* Replaced removed solid-ui-react library with new custom hook for useSession

* Replacing static link /clients/profile to /clients/:id

* Fixed getPodUrl to use ENV; Included empty string for username as part of initial selectedUserContext to handle MUI error; Passing selectedUser directly to set permission forms as props

* Refactored Profile page to have UserProfile and ClientProfile components and render based on user; Incorporated ClientProfileInfo into ClientProfile

* Moved JSDoc typedefs from components to typedefs.js; Included JSDoc for UserProfile

* Removed unit tests for functions that utilizes username, will be phased out soon when replaced with podUrl

* Moved additional typedefs to typedefs.js; Replaced SelectedUserContext with router state to pass data to generate profile information; Reverted back to using DocumentListContext; Included function to clear selectedUser context when entering user profile; Revert back to use DocumentListContext; Included loading animation for loading profile; Included additional animation for LoadingAnimation component

* Setting restore path for profile pages

* Placed temporary solution for clearing documentList after other user revoke permissions; Include username in initial state for SelectedUserContext

* Moved setTimeout for clearInputFields for set permission forms; Removed TODO in Line 84-85 from Profile.jsx

* fix Navbar width issues on mobile

* Removed SelectedUserContext and relevant provider; Updated DocumentListContext to take in client as a state to use for loading documents; Included revokeObjectUrl function as a clean-up function to the file Blob

* Changed typedef for clientProfileProps to profileComponentProps; Combined ClientProfile with UserProfile into component called ProfileComponent; Included clientProfile as prop for ProfileComponent; Allow LoadingAnimation to take in children components for animation; Removed unused labelId for ClientListTable and ClientListTableRow; Switched link to profile to Client column in ClientList and removed Client Profile column

* Updating unit test for ClientListTable

* Dropping Go back button for client profile

* Updating long paths with Vite aliases

* Removing unused props and typedefs

* Created routine to clear Blob from browser memory after usage

* Replaced user with client object for rendering condition for set permission forms; Updated typedefs for set permission form components

* Removing unused props for DocumentTable and UploadDocumentModal

* Included finally block to run setTimeout for clearInputFields for permission forms

* Removed unused props from uploadDocumentModalProps typedefs

* Optimized parts of ProfileComponent

* Simplified ternaries in fetchProfileInfo

* Organizing states in Profile page

* Corrected new typedefs in typedefs.js

* Add Heroku Config (#326)

* updated route pathing

* Revert changes for handleShowDocumentLocal and moving changes to separate PR

* Minor change to ClientListTable margins

* Refactoring NavBar into 3 separate components

* Fixing bug where Notifications Menu did not show when clicking Notifications in mobile menu

* setting up skeletons of tests for navbars

* removing unused imports

* removing unused imports

* adding function to mock proper window size for mobile/desktop tests

* clearing unused functions and imports

* creating a user logged in/out session context to push to tests

* fixing value keys on SessionContextProvider

* destructuring in navbar children components

* fix up some tests for Scott

* Adjusted styling for three columns for ClientList

* Adjusted styling for 3 column in ClientList

* Removing extra React fragment from ProfileComponent

* Moving /clients/:webId routes to /profile/:webId with base route as route to user profile

* Adjusted fetchProfileInfo function to just take in as webId and updated relevant lines using fetchProfileInfo

* Updated JSDoc for clientProfile

* Updated ProfileComponent to allow for potential null in ProfileInputField inputValue; Included unit tests for components in /src/components/Profile; Included devDependency for @testing-library/jest-dom/extend-expect to help with component unit tests

* Removed condition for permission forms for request permissions; Restrict permission forms to user profile page with client object

* resolving errors in tests

* prettier formatting

* Combined first and second unit test for ProfileComponent

* Updated unit test for ProfileComponent

* Removing unused typedefs

* Removing unused typedefs

* Refactored ProfileEditButtonGroup out of ProfileComponent

* clean up some warnings in tests (#328)

* Included fixes for unit tests

* Fixed issue with localStorage.setItem for oidcIssuer in OidcLoginComponent; Revert temporarily fix to getPodUrl to use localStorage.getItem

* Fixed unit test; Moved localStorage.setItem for oidcIssuer outside loginHandler

* Moving localStorage.setItem inside onClick and onKeyUp for Login Button; Updated button type to button instead of submit

* Revert changes to session-core.test.js to original version

* Updated InactivityMessage.jsx

* Post update clean up with lint and prettier; Removed unused contexts; Cleaned up routes

* Fixed localStorage.setItem to trigger upon login by moving function outside logoutHandler

* Revert fix to separate PR

* Merge branch '251-update-the-inactivity-message-and-add-force-logout-on-inactivity-1' of https://github.com/codeforpdx/PASS into 251-update-the-inactivity-message-and-add-force-logout-on-inactivity-1

* Updated InactivityMessage.jsx

* Fixed feature: LoginHanlder working with enter key

* Updated InactivityMessage.jsx with useSession, and created logout timer.

* linted

* ran prettier

* Update pod server script (#344)

* add easier pod startup script

* fix link structure

* Removed onClose from modal

* clear localStorage on logout

* prettier

* Logout clears localStorage

* Updating package.json from v0.6.0 to v0.7.0

* fixed url template

* removed template

* cleaner format for github issue templates

* create PR template

* Cleaning up Development for merge to Master

* changed renderWebId function

* Revert "Fixed template in AddClientModal"

* Updated fix for OidcLoginComponent

* Updated renderWebId function

---------

Co-authored-by: veganedge <[email protected]>
Co-authored-by: Scott Brown <[email protected]>
Co-authored-by: Russell Fraze <[email protected]>
Co-authored-by: Andy Williams <[email protected]>
Co-authored-by: Tim Standen <[email protected]>
Co-authored-by: Jared Krajewski <[email protected]>
Co-authored-by: Tim Standen <[email protected]>
Co-authored-by: brancwill <[email protected]>
Co-authored-by: Radhey Chitroda <[email protected]>
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.

PASS URLs should not contain # symbol
3 participants