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

Closes #16 Implement o auth Google #61

Merged
merged 8 commits into from
Mar 5, 2024
Merged

Conversation

Sojusan
Copy link
Contributor

@Sojusan Sojusan commented Feb 16, 2024

External OAuth providers added: Google, Twitter/X, Facebook, GitHub.
It is done via the Keycloak service. More details can be checked in our docs repo.

I have changed the way we handle .env and appsettings.Development.json. They are now not included by default in the repo as they will contain sensitive data like OAuth providers secrets.

⚠️ If you do not want to create your own external OAuth provider apps, contact me and I can provide the needed credentials.

ℹ️ I did my changes based on your (@mtracewicz ) branch for this feature, so it includes some of your improvements from there, e.g., Serilog implementation.

Stop tracking .env and appsettings.Development.json
Add Keycloak and smtp4dev to docker compose.
Backend integration with Keycloak tokens.
Frontend integration with Keycloak.
Fix eslint config after enabling ESM.
@Sojusan Sojusan requested a review from mtracewicz February 16, 2024 16:03
@Sojusan Sojusan self-assigned this Feb 16, 2024
Update Java version for SonarScanner.
Copy link
Contributor

@mtracewicz mtracewicz left a comment

Choose a reason for hiding this comment

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

As We do not have existing data I would sugest deleting everything from Migrations directory and creating a single Init migration.

backend/src/api/Data/DTO/UserDTO.cs Outdated Show resolved Hide resolved
backend/src/api/Data/KeycloakJwtOptions.cs Outdated Show resolved Hide resolved
backend/src/api/Data/UsersDbContext.cs Outdated Show resolved Hide resolved
backend/src/api/Services/Users/UserService.cs Outdated Show resolved Hide resolved
backend/src/api/Program.cs Show resolved Hide resolved
frontend/src/hooks/useAuth.tsx Show resolved Hide resolved
frontend/package.json Outdated Show resolved Hide resolved
scripts/seed.sql Outdated Show resolved Hide resolved
@mtracewicz
Copy link
Contributor

Can We configure this not to require name and surname, just email?
Screenshot 2024-02-22 at 22 57 35
same goes for external providers (the app wants even more data)
Screenshot 2024-02-22 at 23 03 44
Screenshot 2024-02-22 at 23 04 33
GitHub one is doing well in this tho:
Screenshot 2024-02-22 at 23 06 22

Also a very small nitpick You have removed Login/Register from router but kept them on the page
Screenshot 2024-02-22 at 23 00 36

@mtracewicz
Copy link
Contributor

@Sojusan I think I'm done with the initial review of the code. Sorry for the amount of things pointed out. Feel free to point out disagree with the points 😉

Now going to the functionality:
I have verified that it runs on my machine, I was able to login with google and create a local account, for Twitter and GitHub I was able to initiate the process as well.

However I'm very concerned about one thing. It seams that I can login using Keycloak and frontend recognizes it, but the backend does not. I have run the application using docker compose with both the env file and appsettings file You have provided. This are the three issues I have encoutered:

  • I was unable to authentiacte using the token from the forntend, it throws exception:
Screenshot 2024-02-22 at 23 32 25
  • no user is created in our application DB (this is most likely caused by the first issue) - I have verified they are present in the keycloak db

  • I was able to see the site wich requires auth even though I got 401 from the create user endpoint. While I know that keycloak is what is authenticating us and not our backend it still feals very wrong

Screenshot 2024-02-22 at 23 24 20

On the brightside:

  • I was able to verify keycloak + frontend working with both local and external accounts
  • the dev smtp works prefectly and is such a cool addition! 🚀

@Sojusan
Copy link
Contributor Author

Sojusan commented Feb 23, 2024

Can We configure this not to require name and surname, just email?

I have created a separate issue #62 in order to investigate how to create our own template for Keycloak and then to create it and add our UI style to it. Will work on it after this PR.

Also a very small nitpick You have removed Login/Register from router but kept them on the page

Deleted.

Renamed backend database to ksummarized. Changed the db context to application.
Make RSA from keycloak constant.
Copy link

Quality Gate Passed Quality Gate passed for 'ksummarized_ksummarized.com_frontend'

Issues
1 New issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

Quality Gate Failed Quality Gate failed for 'ksummarized_ksummarized.com_backend'

Failed conditions
2 Security Hotspots
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@Sojusan
Copy link
Contributor Author

Sojusan commented Feb 27, 2024

  • I was unable to authenticate using the token from the frontend, it throws an exception:

Yeah, there was an error in the configuration for our realm in the Keycloak. I didn't set explicitly the RSA Private and Public keys in the export-realm.json file. This way when you started a fresh instance of the Keycloak on your machine the Keycloak just generated a new pair of these keys, so the backend was dependent on the old one. In order if my fix is working correctly now, please take the newest environment settings (both appsettings and .env) from our docs, prune containers, and volumes, then start once again. The Keycloak should now take keys from the environment variable and validation on the backend site should work correctly.

  • no user is created in our application DB (this is most likely caused by the first issue) - I have verified they are present in the keycloak db

It is related to the previous error. If the token is detected as invalid then the backend refuses that connection without creating a user.

  • I was able to see the site which requires auth even though I got 401 from the create user endpoint. While I know that keycloak is what is authenticating us and not our backend it still feels very wrong

I'm not sure what approach should be there. We changed the authenticating service from our backend to the Keycloak service so I think it is correct to see the frontend after successful authentication in the Keycloak service. The backend is now validating tokens just for security and I think it shouldn't block users from visiting the website when it has access to per the Keycloak service. I think it is enough to just prompt an error when we will be working on the error handling on the frontend.

@mtracewicz
Copy link
Contributor

mtracewicz commented Mar 4, 2024

The changes look great, I wiil approve 😄 Have You checked this failing pipeline do We still need to fix it or is it a false positvie?

EDIT: I have read the errors, We can safely ignore them 👍🏼

@Sojusan Sojusan merged commit 0d4e7de into master Mar 5, 2024
5 of 6 checks passed
@Sojusan Sojusan deleted the 00016_Implement_OAuth_Google branch October 31, 2024 07:31
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.

2 participants