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 #698 - Added logic to map LDAP-Groups to internal roles. #699

Merged
merged 24 commits into from
May 27, 2020

Conversation

MariusBrill
Copy link
Member

@MariusBrill MariusBrill commented Apr 23, 2020

For testing add
roles: write: - *LDAP_WRITE_GROUP* commit: - *LDAP_COMMIT_GROUP* admin: - LDAP_ADMIN_GROUP
to inspectitc-config-server.security.ldap in the application..yml


This change is Reviewable

@mariusoe mariusoe assigned JonasKunz and unassigned JonasKunz Apr 24, 2020
@mariusoe mariusoe self-assigned this Apr 29, 2020
@mariusoe mariusoe assigned JonasKunz and unassigned mariusoe May 4, 2020
Copy link
Member

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

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

Reviewed 18 of 19 files at r1.
Reviewable status: 18 of 19 files reviewed, 19 unresolved discussions (waiting on @JonasKunz and @mbrill-nt)


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/config/model/LdapRoleResolveSettings.java, line 22 at r1 (raw file):

     * Roles defined in this list are granted read access.
     */
    private List<String> read;

Not needed anymore, right? Because everyone has read access anyway.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/agent/AgentController.java, line 45 at r1 (raw file):

     * @return The configuration mapped on the given agent name
     */
    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed anymore? Everyone who is authenticated also has read access.
You can in general remove all @Secured annotations with ReadAccess.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/agentstatus/AgentStatusController.java, line 28 at r1 (raw file):

    private AgentStatusManager statusManager;

    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

See other comments, not needed anymore.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/autocomplete/AutocompleteController.java, line 30 at r1 (raw file):

    private List<AutoCompleter> completers;

    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed anymore.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/configschema/ConfigSchemaController.java, line 23 at r1 (raw file):

    ConfigurationSchemaProvider provider;

    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed anymore.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/file/DefaultConfigController.java, line 16 at r1 (raw file):

public class DefaultConfigController extends AbstractBaseController {

    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/file/DefaultConfigController.java, line 22 at r1 (raw file):

    }

    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/file/DirectoryController.java, line 22 at r1 (raw file):

public class DirectoryController extends FileBaseController {

    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/file/FileController.java, line 47 at r1 (raw file):

    }

    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/mappings/AgentMappingController.java, line 42 at r1 (raw file):

     * @return List of {@link AgentMapping}s.
     */
    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/mappings/AgentMappingController.java, line 71 at r1 (raw file):

     * @return The {@link AgentMapping} with the given name or a 404 if it does not exist
     */
    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/users/AccountController.java, line 65 at r1 (raw file):

    }

    @Secured(UserRoleConfiguration.READ_ACCESS_ROLE)

Not needed


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/security/config/SecurityConfiguration.java, line 81 at r1 (raw file):

                .and()
                .authorizeRequests()
                .anyRequest().hasAnyRole(getAccessRoles())

Here you can replace this check by making sure that the User has the authorityREAD_ACCESS.
Every WRITER and ADMIN should also have this role and tehrefore every non-ldap User should also have it.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/security/config/SecurityConfiguration.java, line 105 at r1 (raw file):

     * @return the role names to use as an array of Strings.
     */
    private String[] getAccessRoles() {

Wit hthe comment above, this method should not be needed anymore.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/security/config/UserRoleConfiguration.java, line 58 at r1 (raw file):

     * Permission required for read access.
     */
    public static final String READ_ACCESS_ROLE = ROLE_PREFIX + READ_ACCESS;

Do we need these roles at all? Can't we use just permissions?


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/security/userdetails/CustomLdapUserDetailsService.java, line 32 at r1 (raw file):

public class CustomLdapUserDetailsService extends LdapUserDetailsService {

    public static final List<String> OCELOT_ACCESS_USER_ROLES = Arrays.asList(UserRoleConfiguration.ADMIN_ROLE_PERMISSION_SET);

You can remobe this constant, right?


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/security/userdetails/CustomLdapUserDetailsService.java, line 54 at r1 (raw file):

        }
        UserDetails user = super.loadUserByUsername(username);
        String[] roles = resolveAccessRoleSet(user);

Rename roles and resolveAccessRoleSet to permissions and resolvePermissionSet.


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/security/userdetails/CustomLdapUserDetailsService.java, line 73 at r1 (raw file):

        LdapRoleResolveSettings role_settings = settings.getSecurity().getLdap().getRoles();
        Collection<? extends GrantedAuthority> authorities = user.getAuthorities();
        if (containsAuthority(authorities, role_settings.getAdmin())) {

For backwards-compatibility, can you make sure that if the user has the role specified via security.ldap.admin-group, he also gets admin rights?
In addition, can you mark this proeprty as deprecated in favor of your new "admin" roles list?


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/security/userdetails/LocalUserDetailsService.java, line 72 at r1 (raw file):

                .username(user.getUsername())
                .password(user.getPasswordHash())
                .roles(accessRoles)

You can remove accessRoles as well as the postCosntruct method and simply use the ADMIN_ROLE_PERMISSION_SET here.

Copy link
Member Author

@MariusBrill MariusBrill left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 15 files reviewed, 1 unresolved discussion (waiting on @JonasKunz)


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/security/config/UserRoleConfiguration.java, line 58 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

Do we need these roles at all? Can't we use just permissions?

For now i left these roles as we discussed.

@mariusoe mariusoe linked an issue May 7, 2020 that may be closed by this pull request
JonasKunz added 3 commits May 27, 2020 09:33
# Conflicts:
#	components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/file/DirectoryController.java
#	components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/file/FileController.java
#	components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/file/MoveController.java
Copy link
Member

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 19 files at r2, 9 of 9 files at r4, 1 of 1 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@JonasKunz JonasKunz merged commit 45a4c48 into inspectIT:master May 27, 2020
@MariusBrill MariusBrill deleted the Role_Based_Auth branch May 25, 2021 10:47
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.

Add Role-Based Authentication and make it work for LDAP.
3 participants