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

fix(auth): Rolling back group configuration to a map format #526

Merged
merged 3 commits into from
Dec 8, 2020

Conversation

ronrother
Copy link
Contributor

@ronrother ronrother commented Nov 24, 2020

Hi @tchiotludo

This is a proposed fix for an issue with 0.16.0 that happens when you configure security groups (akhq.security.groups). I believe it's related to this issue.

The root cause is, since the groups are now configured as a list (and not a map) the configuration that takes precedence (according to the Micronaut rules for precedence) will override all other configurations for that key. In other words, if you configure any group under akqh.security.groups, the default admin, reader and no-roles groups (defined in the resources/application.yml) will be overriden.

A mitigation for the users waiting for a fix is to add the basic groups to their own configuration (basically copy them from https://github.com/tchiotludo/akhq/blob/dev/src/main/resources/application.yml).

The ideal solution in my opinion would be to revert to using a map instead of a list for configuring groups, but I understand that might be troublesome for you.

The solution in this PR is a middle-ground. It renames the default groups configured in resources/application.yml to basic-groups and merges them with the ones configured by the user. There is a limitation though, only one environment is supported (https://docs.micronaut.io/latest/guide/index.html#environments). This should cover 90% if not all use cases, though, and is already a limitation in the current 0.16.0.

Hope this is helpful and apologies for not introducing myself first.

@tchiotludo
Copy link
Owner

Hi @ronrother,
Sorry for the late reply ! and thanks for this PR !

I think there is a lot of issue about that on last release !
To be honest, i was not aware that micronaut will overwrite all the key from on array !
I was think it twice before using array in configuration.

To be transparent, knowing that, I will think reverting to map will be the best option in a long term !

Do you want to move to this PR to a rollback to a map ?

If yes, just on this map the key of the map to a special properties key and not the name here.
There is a lot of issue with ldap since micronaut will change the case of the key.

WHat do you think about that ?

@ronrother
Copy link
Contributor Author

ronrother commented Dec 2, 2020

Hi @tchiotludo

Sure, I will look into that still this week.

@ronrother ronrother changed the title fix(auth): Merging basic groups with configured groups fix(auth): Rolling back group configuration to a map format Dec 7, 2020
@ronrother
Copy link
Contributor Author

Hey @tchiotludo

I modified this PR to make the group configuration use a map instead of a list, as you requested. Note that the users that switched to the list format will have to rewrite their configurations again once this change is picked up in a release.

Sorry for the late reply.

Copy link
Owner

@tchiotludo tchiotludo left a comment

Choose a reason for hiding this comment

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

Can you just add the name & the key please ?

I know for the configuration files changed ... I love to have issue about that 🤔 but a mistake was done and need to be corrected.

Thanks 👍

- connect/read
- name: no-roles
roles: []
admin:
Copy link
Owner

Choose a reason for hiding this comment

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

Just a minor changes here :

admin: 
  name: Admin
   roles: 

And everywhere else.

We need to have a name & a key here because people with LDAP have group case sensitive.
I have a lot of issues about that in the past that lead me to go to array (that was not a good idea at the end).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a name property to the default groups with the same name as the key. I also changed the PostConstruct of SecurityProperties so that, when a name is not specified in the configuration, the key will be used as default.

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