feat(auth): add keycloak and auth Logic #44

[MasterEvarior]

Authentication

It looks way worse than it is, most of the files are just renaming, tests and moving of some dependencies.

To make the reviewing a bit easier, here is an overview of what changed and why.

Backend

Configuration

Configurations are defined as records and immutable, making them relatively safe to pass around.

The following can be configured:

# Authentication & JWT
pcts.security.authorization.admin-authorities=org_hr,org_gl
pcts.security.authentication.username-claim=name
pcts.security.authentication.email-claim=email
pcts.security.authorization.authorities-spel-expression=[pitc][roles] #path to extract the roles from

Some of the configuration can be accessed through the /configuration endpoint, this is basically the things which are necessary for the frontend to function.

Annotations

There are 5 new annotations used for authorization:

• IsAdmin: only accessible by people with the roles defined in the configuration
• IsOwner: only accessible if the user that access the resource is the owner of the resource
• IsAdminOrOwner: either or of the two above
• IsAuthenticated: the user must only be authenticated

The owner is identified by their email address. If no email is defined in either the JWT or the database, access is denied.

Email

An email property was added to the member so an owner can be identified. If there is a better way to do this, I am very open for ideas :D

Frontend

keycloak-angular

The integration with keycloak was done with the keycloak-angular library, which is configured in the app.config.ts.

Guard

The guard checks wether a user:

• has an admin role => can do anything
• has a user role => can only access his own site

The roles are loaded from the backend, aswell as which id is connected to the users email address for the redirect.

Testing

Manual

Simply start the application and login as either:

• gl:gl
• member:member

The member should only have access to his own details page, the admin should be able to do everything.

E2E

E2E test have been changed to include a login, aswell as new ones that check wether the permission and redirects work.

[peggimann]

As for the backend part, i reckon the approach definitely is the right one. The architecture and design is clean. There are some questions raised however. In my opinion it is not a promising pattern to have the user identifier (email) as optional. Currently our LDAP system uses preffered_username as unique identifier for an user so the token must contain it otherwise you should not authorize the user. Further the email must exist too if not you should not authorize this user too. I propose to use preffered_username as the key to identify a user as this is guaranteed to be unique and matches the ldap, if you dont like that take the email as you did, however do not make it optional as this forces you to handle cases you can't handle properly in the app. The idea is neat to have a backup authorization but in general a user either fulfills the requirements to be authorized and authenticated (e.g. have an email) or he wont be authorized (401) and for sure not authenticated (403).

[kcinay055679]

You have 3 libraries to handle the auth flow, why?
This seems to be the standard library for oAuth2 is there a specific reason why you decided yourself against it?

[github-actions]

Frontend Test Results

  1 files  ± 0   45 suites  +3   36s ⏱️ +11s
235 tests +18  235 ✅ +18  0 💤 ±0  0 ❌ ±0 
239 runs  +20  239 ✅ +20  0 💤 ±0  0 ❌ ±0 

Results for commit 6931a98. ± Comparison against base commit 8fa3018.

This pull request removes 1 and adds 19 tests. Note that renamed tests count towards both.

Auth should be created ‑ Auth should be created

AppComponent should call logout service when handleLogout() is called ‑ AppComponent should call logout service when handleLogout() is called
AppComponent should navigate to /member when visitRoot() is called ‑ AppComponent should navigate to /member when visitRoot() is called
AuthService name signal should combine given_name and family_name if "name" is missing ‑ AuthService name signal should combine given_name and family_name if "name" is missing
AuthService name signal should return "name" if it exists ‑ AuthService name signal should return "name" if it exists
AuthService name signal should return null if no name fields are present ‑ AuthService name signal should return null if no name fields are present
AuthService name signal should return null if the token is undefined ‑ AuthService name signal should return null if the token is undefined
AuthService should be created ‑ AuthService should be created
AuthService should call keycloak logout ‑ AuthService should call keycloak logout
MemberService toDto should set email address to null if string is blank or empty ‑ MemberService toDto should set email address to null if string is blank or empty
ShowIfAdminDirective (Jest) should NOT render content if user is not admin ‑ ShowIfAdminDirective (Jest) should NOT render content if user is not admin
…

♻️ This comment has been updated with latest results.

[MasterEvarior]

You have 3 libraries to handle the auth flow, why? This seems to be the standard library for oAuth2 is there a specific reason why you decided yourself against it?

@kcinay055679 keycloak-angular depends on keycloak-js (as does angular-auth-oidc-client as far as I can tell) so those two are intended to be there. I deleted the other one.

I've gone with keycloak-angular as currently we only have to support keycloak and it provides some out-of-the-box components which made the implementation somewhat more easy.

[MasterEvarior]

@peggimann

There are some questions raised however. In my opinion it is not a promising pattern to have the user identifier (email) as optional. Currently our LDAP system uses preffered_username as unique identifier for an user so the token must contain it otherwise you should not authorize the user. Further the email must exist too if not you should not authorize this user too. I propose to use preffered_username as the key to identify a user as this is guaranteed to be unique and matches the ldap, if you dont like that take the email as you did, however do not make it optional as this forces you to handle cases you can't handle properly in the app.

The whole thing is sadly a bit more complicated. I initially wanted to us something else rather than the email but I've decided on the email for the reason that this is an information that can be synced over from PTime via API. Otherwise the preferred_username property would be need to be manually updated by somebody.

The reason why the email is optional, is that members can exist in this system before they ever do in our LDAP. Hence they have (not yet) an email assigned to them. The flow looks roughly like this:

1. Potential member gets entered into the PCTS-Tool
   2a. Member does not join -> gets deleted
   2b. Member joins -> receives email
   3b. Members email is continuously synced from PTime

I am aware that all of this is not the ideal solution from a technical standpoint but the best I could think up for both technical and business reasons. Nonetheless, I do not want to make this a hill for me to die on, so maybe we brainstorm together over a coffee? ☕

The idea is neat to have a backup authorization but in general a user either fulfills the requirements to be authorized and authenticated (e.g. have an email) or he wont be authorized (401) and for sure not authenticated (403).
That I can do! Ff somebody does not have an email, he wont be authorized.

[MasterEvarior]

TODO

Use preferred_username as auth instead of email.

• On first login check email against users in DB, set preferred_username
• On auth -> check preferred_username against DB
  • if found -> login
  • if not found -> check if email exists, update preferred_username
  • if no email and no pref_username -> create new user in DB

[github-actions]

Backend Test Results

721 tests  +31   721 ✅ +31   1m 2s ⏱️ +9s
 63 suites + 5     0 💤 ± 0 
 63 files   + 5     0 ❌ ± 0 

Results for commit 44d832b. ± Comparison against base commit f8c4373.

This pull request removes 7 and adds 38 tests. Note that renamed tests count towards both.

ch.puzzle.pcts.util.PCTSStringValidationTest ‑ shouldNotThrowErrorWhenStringIsValid
ch.puzzle.pcts.util.PCTSStringValidationTest ‑ shouldThrowErrorWhenStringIsBlank(String)[1]
ch.puzzle.pcts.util.PCTSStringValidationTest ‑ shouldThrowErrorWhenStringIsBlank(String)[2]
ch.puzzle.pcts.util.PCTSStringValidationTest ‑ shouldThrowErrorWhenStringIsNull
ch.puzzle.pcts.util.PCTSStringValidationTest ‑ shouldThrowErrorWhenStringIsTooLong
ch.puzzle.pcts.util.PCTSStringValidationTest ‑ shouldThrowErrorWhenStringIsTooShort(String)[1]
ch.puzzle.pcts.util.PCTSStringValidationTest ‑ shouldThrowErrorWhenStringIsTooShort(String)[2]

ch.puzzle.pcts.architecture.ArchitectureTest ‑ configurationsMustBeRecordsAndCorrectlyAnnotated
ch.puzzle.pcts.architecture.ArchitectureTest ‑ configurationsShouldBePrefixedCorrectly
ch.puzzle.pcts.architecture.ArchitectureTest ‑ controllersShouldBeAnnotatedWithIsAdmin
ch.puzzle.pcts.configuration.AuthenticationConfigurationTest ‑ shouldStorePropertiesCorrectly
ch.puzzle.pcts.configuration.AuthorizationConfigurationTest ‑ adminAuthoritiesShouldBeImmutableCopy
ch.puzzle.pcts.configuration.AuthorizationConfigurationTest ‑ shouldPrefixAdminAuthoritiesWithScope
ch.puzzle.pcts.service.JwtServiceTest ‑ shouldFallbackToSubject
ch.puzzle.pcts.service.JwtServiceTest ‑ shouldReturnEmailFromJwtClaim
ch.puzzle.pcts.service.JwtServiceTest ‑ shouldReturnEmptyWhenNoAuth
ch.puzzle.pcts.service.JwtServiceTest ‑ shouldReturnUsernameClaim
…

♻️ This comment has been updated with latest results.