Support external users #162

tuj · 2023-09-01T10:00:48Z

Link to ticket

https://jira.itkdev.dk/browse/DISPLAY-993

Description

Adds "external" openid-connect provider.
Renamed "oidc" openid-connect provider to "ad".
Modifies User to support external user type.
Adds command to set user type.
Expands api with external user endpoints.
Upgrades openid-connect bundle to 3.1 to support multiple providers.
Changes php requirement in composer.json to >= 8.1.
Changed user identifier to providerId.

NB! Uses the changes from #163 since support for multiple providers is necessary.

Screenshots

Checklist

My code is covered by test cases.
My code passes our test (all our tests).
My code passes our static analysis suite.
My code passes our continuous integration process.

… api

…ers for /external-users endpoints

…n of openid-connect bundle

DISPLAY-1039: Release 1.4.0

no message

…ndpoint

Code authorization flow

… in the database as a fullname

README.md

turegjorup · 2023-10-01T08:54:33Z

config/api_platform/user-activation-code.yaml

+  App\Entity\Tenant\UserActivationCode:
+    itemOperations:
+      get:
+        security: 'is_granted("ROLE_ADMIN") or is_granted("ROLE_EXTERNAL_USER_ADMIN")'


Would it be better to only have is_granted("ROLE_EXTERNAL_USER_ADMIN") here and the use the role hierarchy to let ROLE_ADMIN have the ROLE_EXTERNAL_USER_ADMIN. I think that would make it easier to change this later if we decide that **_USER_ADMIN should be a dedicated role.

turegjorup · 2023-10-01T08:54:46Z

config/api_platform/user-activation-code.yaml

+      get:
+        security: 'is_granted("ROLE_ADMIN") or is_granted("ROLE_EXTERNAL_USER_ADMIN")'
+      delete:
+        security: 'is_granted("ROLE_ADMIN") or is_granted("ROLE_EXTERNAL_USER_ADMIN")'


Consider using role hierarchy as above

turegjorup · 2023-10-01T08:54:52Z

config/api_platform/user-activation-code.yaml

+      delete:
+        security: 'is_granted("ROLE_ADMIN") or is_granted("ROLE_EXTERNAL_USER_ADMIN")'
+      refresh_code:
+        security: 'is_granted("ROLE_ADMIN") or is_granted("ROLE_EXTERNAL_USER_ADMIN")'


Consider using role hierarchy as above

turegjorup · 2023-10-01T09:00:45Z

config/api_platform/user-activation-code.yaml

+      refresh_code:
+        security: 'is_granted("ROLE_ADMIN") or is_granted("ROLE_EXTERNAL_USER_ADMIN")'
+        method: 'POST'
+        path: '/user-activation-codes/{id}/refresh-code'


We have other auth related endpoint under /v1/authentication should this be there as well?

I think I will leave it as a separate endpoint since it relates to an entity

turegjorup · 2023-10-02T11:34:33Z

src/Security/ExternalUserAuthenticator.php

+        $user = $passport->getUser();
+
+        if (!$user instanceof User) {
+            throw new AuthenticationException('UserInterface not of User type');


I have no idea what that exception message means

turegjorup · 2023-10-02T11:41:46Z

src/Service/ExternalUserService.php

+    {
+        $slugged = $this->slugifyDisplayName($displayName);
+
+        return "$slugged@ext";


I prefer $slugged.'@ext' or a sprintf() for readbility

turegjorup · 2023-10-02T11:44:41Z

src/Security/AzureOidcAuthenticator.php

+                    $type = UserTypeEnum::OIDC_EXTERNAL;
+                    $name = ExternalUserService::EXTERNAL_USER_DEFAULT_NAME;
+                    $providerId = $this->externalUserService->generatePersonalIdentifierHash($signInName);
+                    $email = null;


Not sure what the best solution is if the email is not nullable. Either do $signInName.'@ext' as a temporary value or make email nullable, but unique. (MySQL allows multiple NULLs in a column with a unique constraint.)

Will create a temporary email from providerId when the user has not activated. The mail will be updated after activation.

turegjorup · 2023-10-02T11:47:38Z

src/Service/ExternalUserService.php

+    public function refreshCode(UserActivationCode $code): UserActivationCode
+    {
+        $code->setCode($this->generateExternalUserCode());
+        $code->setCodeExpire((new \DateTime())->add(new \DateInterval('P2D')));


We should not have this hardcoded two different places (Also found in ExternalUserActivationCodeInputDataTransformer). Promote it to a public class constant here, and then reference it.

(Or better, make it and .env value)

turegjorup · 2023-10-02T11:49:51Z

src/Service/ExternalUserService.php

+    private function generateRandomCode(): string
+    {
+        $length = 12;
+        $chars = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ';


Promote to class constant

Co-authored-by: Ture Gjørup <[email protected]>

…2display/display-api-service into feature/external-login-entity-and-api

…system at migration time

…k_jwt_authentication

tuj and others added 30 commits September 1, 2023 11:57

DISPLAY-1001: Started work on supporting external users in entity and…

3d4c278

… api

DISPLAY-993: Added data providers to limit access to only external us…

8e602a9

…ers for /external-users endpoints

DISPLAY-993: Fixed mutation endpoints

295709a

DISPLAY-1030: Code authorization flow

d097a67

DISPLAY-1030: Prepared upgrade to php 8.2

6a66a67

DISPLAY-1030: Updated OpenAPI spec

5710905

DISPLAY-993: Work on activation. Modified flow to match newest versio…

fea67ef

…n of openid-connect bundle

DISPLAY-993: Changed to seperate entity for activation code

fbffd67

DISPLAY-993: Gathered migrations into one

21d4df4

DISPLAY-993: Fixed external user authentication and activation

cf70159

DISPLAY-993: Applied coding standards

81940e3

DISPLAY-993: Updated changelog

58507fa

DISPLAY-993: Removed PHP Upgrade coding standards github actions check

3b04b88

DISPLAY-1039: Update Changelog

1b33efe

DISPLAY-1039: Remove code style check from PHP upgrade check

de9ecdb

Merge pull request #164 from os2display/release/1.4.0

878f3eb

DISPLAY-1039: Release 1.4.0

Merge branch 'release/1.4.0'

640ed02

Merge tag '1.4.0' into develop

9e76bff

no message

DISPLAY-993: Added check for existing tenant user connection

3de6957

DISPLAY-993: Removed activation code after usage

94f0e32

DISPLAY-993: Applied coding standards

77515da

DISPLAY-993: Fixed access to user

1bbe0e1

DISPLAY-993: Added fields to serialization

69e6c1f

DISPLAY-993: Changed refresh code to POST method

2be105e

DISPLAY-993: Changed activation code to be tenant scoped

2d0fadc

DISPLAY-993: Added controller to handle user delete

c2115eb

DISPLAY-993: Applied coding standards

b224309

DISPLAY-993: Cleaned up relation

a3903f3

DISPLAY-993: Fixed infrastructure files

088d17c

DISPLAY-993: Updated api spec

ec7e706

tuj added 6 commits September 29, 2023 10:42

DISPLAY-993: Changed to use providerId as user identifier

5ea1665

DISPLAY-993: Fixed email as slugified version of display name

4c46815

DISPLAY-993: Changed from email to providerId in token/authenticate e…

1fc2d9b

…ndpoint

DISPLAY-993: Updated markdown setup and fixed linting issues

39e119e

DISPLAY-993: Added -

38bd220

DISPLAY-993: Changed command

259ae1e

tuj mentioned this pull request Sep 29, 2023

Feature/external login crud os2display/display-admin-client#220

Merged

4 tasks

turegjorup and others added 3 commits September 29, 2023 13:12

Merge pull request #163 from os2display/feature/openid-connect-code-flow

d9b39ba

Code authorization flow

DISPLAY-399: Added check that the display name does not already exist…

1648341

… in the database as a fullname

DISPLAY-993: Fixed merge

0b6076a

tuj changed the base branch from feature/external-login to develop September 29, 2023 13:39

tuj changed the base branch from develop to feature/external-login September 29, 2023 13:44

DISPLAY-993: Fixed issues raised in review

f32f858

turegjorup requested changes Oct 2, 2023

View reviewed changes

tuj and others added 11 commits October 3, 2023 05:57

Update README.md

de8d9ed

Co-authored-by: Ture Gjørup <[email protected]>

DISPLAY-993: Fixed issues raised in review

56ded9b

Merge branch 'feature/external-login-entity-and-api' of github.com:os…

1e912ef

…2display/display-api-service into feature/external-login-entity-and-api

DISPLAY-993: Fixed issues raised in review

a9e9c86

DISPLAY-993: Fixed issues raised in review

bf1e1fe

DISPLAY-993: Added user extension to user item provider

95813ae

DISPLAY-993: Added updates to set user_type for users already in the …

8f39172

…system at migration time

DISPLAY-993: Changed UserTypeEnum values

4a425a5

DISPLAY-993: Changed how string is concatenated

47700c9

DISPLAY-993: Fixed roles check in security voter

4edc952

DISPLAY-993: Changed role strings to class constants

fe744e3

tuj requested a review from turegjorup October 4, 2023 11:48

DISPLAY-993: Changed getUsername to providerId to fix issue with lexi…

2f34c11

…k_jwt_authentication

turegjorup approved these changes Oct 10, 2023

View reviewed changes

tuj merged commit 2f5a3a0 into feature/external-login Nov 9, 2023
10 checks passed

tuj deleted the feature/external-login-entity-and-api branch November 9, 2023 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support external users #162

Support external users #162

tuj commented Sep 1, 2023 •

edited

Loading

turegjorup Oct 1, 2023

tuj Oct 4, 2023

turegjorup Oct 1, 2023

tuj Oct 4, 2023

turegjorup Oct 1, 2023

tuj Oct 4, 2023

turegjorup Oct 1, 2023

tuj Oct 4, 2023

turegjorup Oct 2, 2023

tuj Oct 4, 2023

turegjorup Oct 2, 2023

tuj Oct 3, 2023

turegjorup Oct 2, 2023

tuj Oct 3, 2023

turegjorup Oct 2, 2023

tuj Oct 4, 2023

turegjorup Oct 2, 2023

tuj Oct 3, 2023

Support external users #162

Support external users #162

Conversation

tuj commented Sep 1, 2023 • edited Loading

Link to ticket

Description

Screenshots

Checklist

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

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

tuj commented Sep 1, 2023 •

edited

Loading