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

Enable and Fix keycloak User Registration Config along with defaulting to 'User' group for newly registered users #24765

Merged
merged 3 commits into from May 9, 2024

Conversation

RawSanj
Copy link
Contributor

@RawSanj RawSanj commented Jan 6, 2024

Enable and Fix Keycloak User Registration Config along with defaulting to the 'User' group for newly registered users:

  1. This fixes the error in the Jhipster Realm config which throws an error when enabling the User Registration in Keycloak.
  2. The User Registration is now by default enabled and newly registered accounts by default are assigned to the User group to get the USER Role.

Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@CLAassistant
Copy link

CLAassistant commented Jan 6, 2024

CLA assistant check
All committers have signed the CLA.

@RawSanj RawSanj marked this pull request as ready for review January 6, 2024 08:21
@RawSanj
Copy link
Contributor Author

RawSanj commented Jan 6, 2024

This PR belongs to the label theme: keycloak. Not sure if I can edit the labels.

Copy link
Member

@atomfrede atomfrede left a comment

Choose a reason for hiding this comment

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

LGTM. Should we document this as a breaking change, such that existing applications can disable user registration as they might rely on the old default configuration?

@RawSanj
Copy link
Contributor Author

RawSanj commented Jan 9, 2024

LGTM. Should we document this as a breaking change, such that existing applications can disable user registration as they might rely on the old default configuration?

Would it go under - https://github.com/jhipster/jhipster.github.io/blob/main/pages/security.md#keycloak or in the release notes?

Or I can disable the user registration to false by default. But we might still require a note to inform users that User Registration is now fixed and can be enabled.

@atomfrede
Copy link
Member

I would say lets keep it enabled as one can register without keycloak too by default.

@RawSanj
Copy link
Contributor Author

RawSanj commented Feb 10, 2024

@atomfrede Is there anything else I should work upon to get this merged?
If not, can you please merge this PR?

@idNoRD
Copy link

idNoRD commented Apr 25, 2024

There are two "users" configuration keys in the json.
Can you please refactor to have only one "users" key to avoid json validation warning "Object contains duplicate keys 'users'"? Thanks

@mraible
Copy link
Contributor

mraible commented Apr 29, 2024

@idNoRD I don't see two users keys being added in this PR, so I believe your request is unrelated. Please open a new issue/PR for this.

@mraible
Copy link
Contributor

mraible commented Apr 29, 2024

@RawSanj I QA'd this PR today and I can confirm it enables registration by default and puts users in the proper group.

However, if I create a brand new app with JHipster 8.3.0 (using jhipster jdl blog-oauth2), I can enable user registration successfully and register just fine. Because of this, it seems this PR only enables user registration.

Can you please provide the steps needed to reproduce the issue where Keycloak throws an error when enabling user registration?

@RawSanj
Copy link
Contributor Author

RawSanj commented Apr 30, 2024

@mraible Looks like the registration issue is fixed in another PR-25679 merged (30 March) after this PR was raised (6 Jan).

This PR now only enables the User Registrations by default and Puts newly registered users into User Group/Role.
Let me know if we should skip this PR or sync up the changes and merge this PR?

@mraible
Copy link
Contributor

mraible commented Apr 30, 2024

@RawSanj If I create a new app with jhipster jdl blog-oauth2 with JHipster 8.4.0, enable registration in Keycloak, and create a new user, it has access to the Entities menu.

Screenshot 2024-04-30 at 9 43 38 AM

I thought this implies the new user is added to the correct group, but the new user has no groups.

Screenshot 2024-04-30 at 9 45 58 AM

Please sync this PR with main and we'll get it merged.

@mraible mraible merged commit bef1c4d into jhipster:main May 9, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants