feat: implement authentication layer #18428

Ben-Sheppard · 2024-05-10T11:12:13Z

Closes https://github.com/camunda-cloud/identity/issues/2804

Description

This PR achieves a couple of tasks out of necessity so its a little larger than I'd often like, still, here goes:

Implements a new auth-starter module under Identity that provides a centralised configuration for auth related classes
Harmonizes some existing backend code with the new auth layer by way of a identity-common module
Introduces configuration support for postgres and H2 by way of a profile postgres, h2
Introduces the schema initializaiton for the user tables

Things that I want to highlight:

We need to look for database schema migration support, the changes in this PR are fine for now to get foundations in but longer term this will cause issues
I have tried to use a naming pattern of Camunda where there are classes that we extend from Spring so that its obvious its our code
The error handler is taken from the existing /dist approach and I've also tried to align the configurations
I have tested the code in this PR in two contexts, Identity + H2 and Identity + Standalone Broker + Postgres, my local tests have been successful for basic auth.

.../auth-starter/src/main/java/io/camunda/identity/authentication/config/WebSecurityConfig.java

github-actions · 2024-05-10T11:37:03Z

Tasklist Test Results

549 tests 544 ✅ 1h 31m 52s ⏱️
143 suites 5 💤
143 files 0 ❌

Results for commit bf50761.

♻️ This comment has been updated with latest results.

github-actions · 2024-05-10T11:39:30Z

Operate Test Results

265 tests 264 ✅ 34s ⏱️
42 suites 1 💤
42 files 0 ❌

Results for commit 3fa08d5.

♻️ This comment has been updated with latest results.

maryarm

The structure looks good to me, I have a few suggestions that you can find in following comments.

maryarm · 2024-05-14T11:10:25Z

...th-starter/src/main/java/io/camunda/identity/authentication/oidc/OidcCurrentUserService.java

+@Profile("identity-oidc-auth")
+public class OidcCurrentUserService implements CurrentUserService {
+  @Override
+  public CamundaUser getCurrentUser() {


I'm thinking if we need a separate implementation for oidc, as the authorities can be retrieved from authentication object (it would be an instance of OAuth2AuthenticationToken). The only difference is using authentication.name which extracted from claims["name"] instead of principal.getPreferredUsername which filled from claims["preferred_username"].

maryarm · 2024-05-14T11:14:29Z

...h-starter/src/main/java/io/camunda/identity/authentication/basic/CamundaOidcUserService.java

+    final OidcUser oidcUser = super.loadUser(userRequest);
+    final UserDetails details = userDetailsManager.loadUserByUsername(oidcUser.getName());
+    if (details != null) {
+      return new CamundaOidcUser(


Intellij suggests to use map(GrantedAuthority.class::cast)

maryarm · 2024-05-14T12:07:07Z

...tarter/src/main/java/io/camunda/identity/authentication/basic/CamundaUserDetailsManager.java

+  }
+
+  @Override
+  protected List<GrantedAuthority> loadUserAuthorities(final String username) {


Do we need this override for the current issue?
if it's required I suggest to give a try to override JdbcDaoImpl#addCustomAuthorities.
but if not, it's better to remove it for now and make it working later with the tasks related to add role/permission to identity.

We don't need it (I just checked) so I'm removing it :)

github-actions · 2024-05-20T08:11:10Z

Operate Opensearch ITs Results

182 tests 182 ✅ 54s ⏱️
19 suites 0 💤
19 files 0 ❌

Results for commit bf50761.

♻️ This comment has been updated with latest results.

github-actions · 2024-05-20T08:16:56Z

Operate Integration Tests Results

523 tests 521 ✅ 8m 20s ⏱️
88 suites 2 💤
88 files 0 ❌

Results for commit bf50761.

♻️ This comment has been updated with latest results.

github-actions · 2024-05-20T08:17:02Z

Operate Unit Tests Results

275 tests 274 ✅ 6m 21s ⏱️
44 suites 1 💤
44 files 0 ❌

Results for commit bf50761.

♻️ This comment has been updated with latest results.

dlavrenuek

Thank you for the code. The current combination of basic auth (local user management) and OIDC is confusing since it is not in the scope of the issue, so I recommend removing OIDC here.

...tarter/src/main/java/io/camunda/identity/authentication/basic/CamundaCurrentUserService.java

...h-starter/src/main/java/io/camunda/identity/authentication/basic/CamundaOidcUserService.java

dlavrenuek · 2024-05-21T14:02:13Z

identity/auth-starter/src/main/java/io/camunda/identity/authentication/CurrentUserService.java

@@ -5,11 +5,10 @@
 * Licensed under the Camunda License 1.0. You may not use this file
 * except in compliance with the Camunda License 1.0.
 */
-package io.camunda.identity.usermanagement;
+package io.camunda.identity.authentication;


Do we need identity in the folder structure / class names? This way we have identity and authentication as (sub)domains. If we consider identity as the top level domain here, then it would make sense to split the internals in authentication and authorization. Otherwise we could just have a top level domain authentication and omit identity. What do you think?

Initially had included identity because its our realm and Identity is still a component in the stack in the same way we have io.camunda.operate... etc so I'd prefer to keep it, but I can see how we could remove this and have a flat level of io.camunda.authentication. Ultimately I don't feel strongly either way so I will change it if you have stronger feelings than me :D

and Identity is still a component

That is also a reason to not use identity here, so it is not confused with the application/component in the end. On a scale of 1-10 I am at about 6.5 that we should not use identity here, but just keep it simple as io.camunda.authentication

Ok, I've changed it to io.camunda.authentication

...ain/java/io/camunda/identity/authentication/handler/CamundaAuthenticationFailureHandler.java

dlavrenuek · 2024-05-21T14:24:13Z

identity/common/src/main/java/io/camunda/identity/record/CamundaOidcUser.java

+ * Licensed under the Camunda License 1.0. You may not use this file
+ * except in compliance with the Camunda License 1.0.
+ */
+package io.camunda.identity.record;


I suggest structuring the code by the domain instead of class type, so for example having a users directory with all user related records and services instead of service and records directories. This structure should be more robust as the code grows. In Identity it became challenging to name classes in a single directory - especially records as these could include entities but also request objects and DTOs. What do you think?

Sounds good, I've updated this structure now

The idea of naming the packages with subdomain sounds good but I have some concerns here regarding how to find a subdomain that contains all relevant stuffs. For example when we named this package as user, then when we want to add group dtos we need to add a new package with one class only that's not looks nice, also for user roles and group roles and group users how we can proceed with this style?

An example structure for roles: everything related to roles would live in a roles directory. This could be a subdirectory of users or on the same level and would contain all DTOs and services to retrieve/manage roles. Working with subdirectories can give more granularity and a structure similar to api endpoints while having these on the same level should be easier for reusable parts like.
An example why I think that structuring by domain is a good idea is multitenancy. We would have a directory tenants containing all controllers, services, records, DTOs etc... and all of these can be annotated to only be included if multitenancy is activated. Otherwise the multitenancy annotation would be present on different classes in different directories and would be harder to follow

dlavrenuek · 2024-05-21T14:28:17Z

identity/docker-compose-identity.yaml

@@ -0,0 +1,10 @@
+services:


It looks like this file is only for testing, but otherwise I can't see any instructions on how to use it. Should we remove it here and rather add it as part of automatic test coverage?

This file should be removed, right?

Ugh yea, sorry I removed it and then added it back in for local testing 🤦🏻

…rocedure

steff46 · 2024-05-22T18:21:55Z

.../auth-starter/src/main/java/io/camunda/identity/authentication/config/WebSecurityConfig.java

+
+  @Bean
+  @Primary
+  @Profile("auth-basic")


Do we need to have this here as well as on the class level above? 🤔

Good spot, that was left over from when we had multiple applicable beans in here, I'll remove

maryarm

I only have one concern regarding the package names but it's not necessary to be addressed in this PR. Otherwise it looks good to me.

…ntication

identity/auth-starter/src/main/java/io/camunda/authentication/config/WebSecurityConfig.java

+      return httpSecurity
+          .authorizeHttpRequests(
+              (authorizeHttpRequests) ->
+                  authorizeHttpRequests
+                      .requestMatchers(UNAUTHENTICATED_PATHS)
+                      .permitAll()
+                      .anyRequest()
+                      .authenticated())
+          .headers(
+              (headers) ->
+                  headers.httpStrictTransportSecurity(
+                      (httpStrictTransportSecurity) ->
+                          httpStrictTransportSecurity
+                              .includeSubDomains(true)
+                              .maxAgeInSeconds(63072000)
+                              .preload(true)))
+          .exceptionHandling(
+              (exceptionHandling) -> exceptionHandling.accessDeniedHandler(authFailureHandler))
+          .csrf(AbstractHttpConfigurer::disable)


dlavrenuek

LGTM 🚀

…d for now

github-actions bot added component/zeebe Related to the Zeebe component/team component/operate Related to the Operate component/team component/tasklist Related to the Tasklist component/team component/identity Related to the Identity component/team labels May 10, 2024

Ben-Sheppard requested review from maryarm, dlavrenuek and steff46 May 10, 2024 11:13

Ben-Sheppard force-pushed the implement-authentication-layer branch from 10c26b6 to bbe8387 Compare May 10, 2024 11:15

github-advanced-security bot found potential problems May 10, 2024

View reviewed changes

.../auth-starter/src/main/java/io/camunda/identity/authentication/config/WebSecurityConfig.java Fixed Show fixed Hide fixed

Ben-Sheppard requested a review from romansmirnov May 10, 2024 16:24

Ben-Sheppard force-pushed the implement-authentication-layer branch from 3519590 to 8121bb7 Compare May 10, 2024 16:28

maryarm mentioned this pull request May 13, 2024

feat: add native groups to identity #18465

Merged

maryarm reviewed May 14, 2024

View reviewed changes

Ben-Sheppard added 7 commits May 20, 2024 09:14

refactor: add a common module for Identity sections to share

7e8b1ac

feat: implement a centralised authentication layer

2124bc3

refactor: harmonize existing Identity backend module with new auth layer

1c42d8c

test: add Identity docker compose to support local dev with postgres

cfbfe2f

feat: support centralised auth in /dist

4c916fc

refactor: fix application yaml to use correct activate param

97fa898

refactor: fix config for database yaml

2219be1

Ben-Sheppard force-pushed the implement-authentication-layer branch from 4d4d042 to 2219be1 Compare May 20, 2024 08:17

Ben-Sheppard added 2 commits May 20, 2024 11:27

refactor: run spotless for pom files

0a49c13

refactor: PR comments

fc1046e

Ben-Sheppard requested a review from maryarm May 20, 2024 10:40

Ben-Sheppard added 4 commits May 20, 2024 12:17

build: fix dependency issues in poms

4358f6b

build: exclude oauth2-oidc-sdk from build

4b13459

build: add auth-spring-boot-starter to the list of unused dependencies

87829c0

Merge branch 'main' into implement-authentication-layer

e03bd93

dlavrenuek requested changes May 21, 2024

View reviewed changes

Ben-Sheppard added 2 commits May 21, 2024 15:44

refactor: include changes relating to groups

23e2f90

refactor: PR comments

13a683a

Ben-Sheppard requested a review from dlavrenuek May 22, 2024 12:00

Ben-Sheppard added 2 commits May 22, 2024 13:50

build: add missing dependency

25d6345

build: resolve issues with build causing applications to fail start p…

317605c

…rocedure

steff46 reviewed May 22, 2024

View reviewed changes

refactor: remove redundant profile annotation

0ebcfb7

steff46 approved these changes May 23, 2024

View reviewed changes

maryarm approved these changes May 23, 2024

View reviewed changes

Ben-Sheppard added 4 commits May 23, 2024 12:15

refactor: remove unused classes

d870ad1

refactor: move io.camunda.identity.authentication to io.camunda.authe…

cee09e1

…ntication

refactor: neaten dependencies for database connections

c7a0667

Merge branch 'main' into implement-authentication-layer

64ad319

github-advanced-security bot found potential problems May 23, 2024

View reviewed changes

refactor: resolve dependency issues

5410c65

dlavrenuek approved these changes May 23, 2024

View reviewed changes

refactor: only configure database connection in Identity when require…

bf50761

…d for now

Ben-Sheppard added this pull request to the merge queue May 23, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 23, 2024

Ben-Sheppard added this pull request to the merge queue May 23, 2024

Merged via the queue into main with commit d262c29 May 23, 2024
62 of 64 checks passed

Ben-Sheppard deleted the implement-authentication-layer branch May 23, 2024 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement authentication layer #18428

feat: implement authentication layer #18428

Ben-Sheppard commented May 10, 2024 •

edited

github-actions bot commented May 10, 2024 •

edited

github-actions bot commented May 10, 2024 •

edited

maryarm left a comment

maryarm May 14, 2024

maryarm May 14, 2024

maryarm May 14, 2024

Ben-Sheppard May 16, 2024

github-actions bot commented May 20, 2024 •

edited

github-actions bot commented May 20, 2024 •

edited

github-actions bot commented May 20, 2024 •

edited

dlavrenuek left a comment

dlavrenuek May 21, 2024

Ben-Sheppard May 21, 2024

dlavrenuek May 23, 2024 •

edited

Ben-Sheppard May 23, 2024

dlavrenuek May 21, 2024

Ben-Sheppard May 21, 2024

maryarm May 23, 2024

dlavrenuek May 23, 2024

dlavrenuek May 21, 2024

dlavrenuek May 23, 2024

Ben-Sheppard May 23, 2024

steff46 May 22, 2024

Ben-Sheppard May 22, 2024

maryarm left a comment

dlavrenuek left a comment

feat: implement authentication layer #18428

feat: implement authentication layer #18428

Conversation

Ben-Sheppard commented May 10, 2024 • edited

Description

github-actions bot commented May 10, 2024 • edited

Tasklist Test Results

github-actions bot commented May 10, 2024 • edited

Operate Test Results

maryarm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 20, 2024 • edited

Operate Opensearch ITs Results

github-actions bot commented May 20, 2024 • edited

Operate Integration Tests Results

github-actions bot commented May 20, 2024 • edited

Operate Unit Tests Results

dlavrenuek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlavrenuek May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryarm left a comment

Choose a reason for hiding this comment

dlavrenuek left a comment

Choose a reason for hiding this comment

Ben-Sheppard commented May 10, 2024 •

edited

github-actions bot commented May 10, 2024 •

edited

github-actions bot commented May 10, 2024 •

edited

github-actions bot commented May 20, 2024 •

edited

github-actions bot commented May 20, 2024 •

edited

github-actions bot commented May 20, 2024 •

edited

dlavrenuek May 23, 2024 •

edited