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

SPA - OIDC integration following the design implemented on the PoC #558

Merged
merged 103 commits into from
Dec 20, 2024

Conversation

g-saracca
Copy link
Contributor

@g-saracca g-saracca commented Dec 3, 2024

What this PR does / why we need it:

Change SPA authentication method to use Keycloak as OIDC provider using PKCE based on an initial PoC.

Which issue(s) this PR closes:

Special notes for your reviewer:

This PR does not point to the develop branch but to a separate branch ( authentication-oidc ), the idea is in another PR to configure the deployment to another environment that is not Beta maybe to be able to test this not only locally before merging to develop.

All unit tests and e2e tests are working now with this new authentication method.
Application terms of use are empty for now as we don't have an API endpoint for it yet.

Suggestions on how to test this:

Step 1: Run the Development Environment

  1. Execute npm i.
  2. Navigate with cd packages/design-system && npm i && npm run build.
  3. Return with cd ../../.
  4. Ensure you have a .env file similar to .env.example, with the variable VITE_DATAVERSE_BACKEND_URL=http://localhost:8000.
  5. Make sure your dev-env/.env file is filled with the appropriate data. For the REGISTRY variable, please set ghcr.io, as we will use a PR-generated dataverse image.
  6. Navigate with cd dev-env.
  7. Start the environment using ./run-env.sh 10959-bearer-token-auth-ext .
  8. To verify the environment, visit http://localhost:8000 and check your local Dataverse installation.

Step 2: Test the feature in the SPA

  1. Click the Log In button in the header.
  2. You will be redirected to the Keycloak login page
  3. Enter the credentials, there are 4 pre-set users in keycloak, you can log in with any of those users:
  1. After the first login, you will be redirected to finish the registration process, where you will have to accept the terms (although there are none for now) and you will also be able to optionally add the affiliation and position.
  2. After submitting the form, you will be redirected to the “root” collection page with a welcome success alert and some confetti. 🎉 .
  3. Click on the user drop-down menu in the header and go to Account Information, check that the data displayed there matches the data you have entered during the registration process.
  4. Try creating a collection and a dataset as a smoke test.
  5. Now log out and log in again, the registration process should not be required now 👍🏼

Another path:

  1. Follow the first 4 steps as before (but with a different user that didn't complete the registration process yet), but now click the Cancel button in the Sign Up page, you should be able to navigate through the SPA as an unauthenticated user.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

sign-up-page

signup-success.mov

Is there a release notes update needed for this change?:

No

Additional documentation:

No

GPortas and others added 30 commits October 3, 2024 13:51
@g-saracca
Copy link
Contributor Author

@ekraffmiller Back to Ready For Review .. There was a single test failing when checking a checkbox (Terms Of Use), this was only happening in github actions, this was because the interactivity was happening too fast, I added a 300ms cy.wait and it's working as expected again.

@g-saracca g-saracca removed their assignment Dec 5, 2024
@cmbz cmbz added the FY25 Sprint 12 FY25 Sprint 12 (2024-12-04 - 2024-12-18) label Dec 5, 2024
@ekraffmiller ekraffmiller self-assigned this Dec 5, 2024
Copy link
Contributor

@ekraffmiller ekraffmiller left a comment

Choose a reason for hiding this comment

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

This looks great, it's so nice to have OIDC in the SPA! 🎉

Regarding the testing, would it simplify things to add a script to docker-compose that completes the signup process for the OpenID users, and updates their permissions? That way it wouldn't have to be done for individual tests.

I did find one issue when I tried to give the Dataverse User account permissions through JSF. The Dataverse User isn't shown on the DataverseAdmin's dashboard, even though a user that I create within Dataverse is there. I can create a separate issue for that.

Also, what is the plan for Dataverse built-in users when we deploy to production? Will Dataverse be configured to do OpenID authentication for those users?

…ary api config inits on all integration tests
@g-saracca
Copy link
Contributor Author

@ekraffmiller thanks for the review!

Regarding the testing, would it simplify things to add a script to docker-compose that completes the signup process for the OpenID users, and updates their permissions? That way it wouldn't have to be done for individual tests.

Also, what is the plan for Dataverse built-in users when we deploy to production? Will Dataverse be configured to do OpenID authentication for those users?

For these two blocks I think we can keep iterating and thinking about it on this same branch that this PR points to (remember it doesn't point to develop).

I did find one issue when I tried to give the Dataverse User account permissions through JSF. The Dataverse User isn't shown on the DataverseAdmin's dashboard, even though a user that I create within Dataverse is there. I can create a separate issue for that.

On this last comment, perhaps we can check with Guillermo on Monday to find out what is going on.

@g-saracca g-saracca removed their assignment Dec 6, 2024
Copy link
Contributor

@ekraffmiller ekraffmiller left a comment

Choose a reason for hiding this comment

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

Sorry, just noticed this, otherwise looks great!

@g-saracca g-saracca removed their assignment Dec 10, 2024
Copy link
Contributor

@ekraffmiller ekraffmiller left a comment

Choose a reason for hiding this comment

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

looks good, approved!

@ofahimIQSS
Copy link
Contributor

Came across an issue while testing this PR in local.

to reproduce:

  1. Follow test steps, go to registration page with test data: [email protected] - pass: curator
  2. In the affiliation field, enter a very long entry, ie. Harvard University - copy and paste that a thousand or more times
  3. Under Position Field, enter a very long entry, ie. Affiliate- copy and paste that a thousand or more times
  4. Click create account

image

Do we want to limit the amount of characters enterable in those 2 fields?

@ofahimIQSS
Copy link
Contributor

Another point I wanted to raise is that when we run the List User api - the new users aren't reflected in the API call:

https://guides.dataverse.org/en/6.5/api/native-api.html#list-users

This image is after I registered 3 of the users
image

@g-saracca
Copy link
Contributor Author

Do we want to limit the amount of characters enterable in those 2 fields?

@ofahimIQSS
I think we should do it, in the new endpoint recently added to register a user we could add a max length validation for these fields and also apply the same client side validation to also avoid the user to receive a similar error.

Since this PR is not pointing to develop, should we still merge it and create a backend and frontend issues to address the fix?

@ofahimIQSS
Copy link
Contributor

Do we want to limit the amount of characters enterable in those 2 fields?

@ofahimIQSS I think we should do it, in the new endpoint recently added to register a user we could add a max length validation for these fields and also apply the same client side validation to also avoid the user to receive a similar error.

Since this PR is not pointing to develop, should we still merge it and create a backend and frontend issues to address the fix?

I agree with moving forward with this PR and merging it and tracking the issue separately. I went ahead and created an issue for this - please modify it to our needs: #574

@ofahimIQSS
Copy link
Contributor

Merging PR as it has passed testing

@ofahimIQSS ofahimIQSS merged commit 9fb1eb2 into authentication-oidc Dec 20, 2024
10 of 14 checks passed
@ofahimIQSS ofahimIQSS deleted the feat/528-oidc-integration branch December 20, 2024 19:29
@ofahimIQSS ofahimIQSS removed their assignment Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 11 FY25 Sprint 11 (2024-11-20 - 2024-12-04) FY25 Sprint 12 FY25 Sprint 12 (2024-12-04 - 2024-12-18) GREI Re-arch GREI re-architecture-related Original size: 10 Size: 10 A percentage of a sprint. 7 hours. SPA.Q4.4 OIDC login + API authentication
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

6 participants