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

Client authentication #500

Merged
merged 34 commits into from
Mar 8, 2022
Merged

Client authentication #500

merged 34 commits into from
Mar 8, 2022

Conversation

MaybeJustJames
Copy link
Collaborator

@MaybeJustJames MaybeJustJames commented Dec 9, 2021

Integrates login to the client.
Also updates to react-routerv6, changes are as lazy as possible (so break a lot of features)

@MaybeJustJames
Copy link
Collaborator Author

@KrisDavie note that the SemGrep issues weren't introduced in this PR

Copy link
Collaborator

@kverstae kverstae left a comment

Choose a reason for hiding this comment

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

Changes make sense to me, didn't really look into detail at the implementation of things since I am not that familiar with how code is structured in this project.

There are 2 things that might be useful to add to the authentication as well:

  1. Persisting authentication status. When you now refresh the page, authentication status is lost. Not really an issue since most users don't manually refresh there page, but would be nice if you could remain logged in after leaving the page.
  2. Ability to log out. For now you can just refresh the page to do this, but a dedicated button would be nicer.

These changes are not necessarily needed in this PR. If they seem interesting enough, they can be added to the TODOs aswell

client/config/webpack.prod.js Outdated Show resolved Hide resolved
client/src/api/fetch.ts Outdated Show resolved Hide resolved
client/src/components/AppHeader.tsx Outdated Show resolved Hide resolved
client/src/components/Viewer/reducer.ts Show resolved Hide resolved
client/src/redux/reducers/main.ts Show resolved Hide resolved
@MaybeJustJames
Copy link
Collaborator Author

MaybeJustJames commented Mar 2, 2022

@kverstae Thanks for having a look at this! Your review is very helpful and insightful.

There are 2 things that might be useful to add to the authentication as well:

  1. Persisting authentication status. When you now refresh the page, authentication status is lost. Not really an issue since most users don't manually refresh there page, but would be nice if you could remain logged in after leaving the page. (Persist redux model to localStorage #507)

I have some code ready to do this in a follow up PR.

  1. Ability to log out. For now you can just refresh the page to do this, but a dedicated button would be nicer.

Indeed of course. That will be a TODO item. (#508)

These changes are not necessarily needed in this PR. If they seem interesting enough, they can be added to the TODOs aswell

Indeed I'd like to avoid any further PR rot.

Introduce a `SCOPE_SERVER_URL` environment variable which may be used
at build time to change the default API server URL. If this Env Var is
not set then default to localhost:8000 in development and
scope.aertslab.org for production.
Copy link
Collaborator

@kverstae kverstae left a comment

Choose a reason for hiding this comment

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

Just 1 comment, otherwise good for me!

@@ -75,7 +76,7 @@ export function* uploadFile(endpoint: string, project: string, file: File) {
}

export function* uploadRequest(action: T.UploadRequest) {
const url = new URL(API_PREFIX + 'project/dataset');
const url = new URL(SERVER_URL + API_PREFIX + 'project/dataset');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this not use the API_URL from api.ts?

@MaybeJustJames MaybeJustJames merged commit 91e66a7 into develop Mar 8, 2022
@KrisDavie KrisDavie deleted the client-authentication branch September 26, 2022 07:34
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.

None yet

2 participants