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

Keycloak integration #48

Merged
merged 23 commits into from
Nov 25, 2021
Merged

Keycloak integration #48

merged 23 commits into from
Nov 25, 2021

Conversation

ali-everest
Copy link
Member

Proposed changes

Replace the Spring Security OAuth with Keycloak.
Keycloak is an open source identity and access management solution. It will take care of handling the authentication and authorisation part. No extra maintenance is required. It will take care of managing the user details and sessions information etc,.

Checklist

  • My local build passed (You ran ./gradlew clean build without failures)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have assigned a label to the PR

ali-everest and others added 10 commits October 29, 2021 17:21
Update org registration saga to invoke keycloak to update user attributes
Add custom user attributes updation code
Separate keycloak into a module.
Update swagger UI to work with keycloak.
Update an user details in keycloak when a new user is added to an existing organization
Handle keycloak user roles update, delete interactions through saga.
Fix database connection closed issue and functional tests
@ali-everest ali-everest added the enhancement New feature or request label Oct 29, 2021
@ali-everest ali-everest requested a review from ywangd October 29, 2021 18:44
@sluehr sluehr self-requested a review October 29, 2021 21:13
@sluehr
Copy link
Member

sluehr commented Oct 29, 2021

This would close lhotse#30, close lhotse#43 and close lhotse#33

Also deprecates the security module which can then be archived.

Once merged, allows for all dependencies to be brought up to date.

Copy link
Member

@sluehr sluehr left a comment

Choose a reason for hiding this comment

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

More to come later....

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@sluehr sluehr left a comment

Choose a reason for hiding this comment

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

Functionality is broken. It is not possible to self register yourself.

Copy link
Contributor

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

It's a big PR. I didn't read everything. Here are some comments before I can commit more time to read it again.

One question that I have is about how keycloak should be positioned in the architecture, or more specifically, the command/event flow. My take is that keycloak should still be secondary because it is not the core of the business model. The principle is that we should be able to rebuild it as long as we have the full events. With the proposed changes, it seems that passwords are known only to keycloak and bypass the core application. This means we cannot replay events for full recovery. The code currently does not deal with user roles, but I believe it would have the similar issue with the current approach.

@sluehr
Copy link
Member

sluehr commented Nov 23, 2021

We have some remaining issues before we can close this off and start picking up the issues that we have identified as part of this review:

  • every time a user visits the site we trigger the process to create a default organisation for them
  • APIs are dispatching commands to aggregates that do not exist. User/org IDs are not being validated like they were in the past due to the underlying authentication changes
  • The account created in Keycloak through the application admin flow cannot be used to log in because the password is never set and the account is missing something else. Attempting to login gives me "account not fully set up".

The first two points need some changes applied to avoid the issues.

The third point I think we can resolve by removing the "admin does a thing for you" flow.

Move role management to keycloak.
@ali-everest ali-everest force-pushed the keycloak-integration branch 2 times, most recently from 8aaeefc to 06830b8 Compare November 23, 2021 14:48
sluehr and others added 4 commits November 24, 2021 11:20
…er bootstrapping.

Removed application admin account creation flow from Juptyer notebook.
Fixed 'memberOfOrg' pre-authorisation expression.
Updated example flows in Jupyter notebook
…Jupyter notebook with example of org user creation and email verification via Keycloak
@sluehr sluehr merged commit 50e0623 into main Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants