Feature: External Login through OIDC #169

tuj · 2023-11-09T10:41:16Z

Link to ticket

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

Description

Adds "external" openid-connect provider.
Renamed "oidc" openid-connect provider to "internal".
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

…/external-login

src/Entity/Tenant/UserActivationCode.php

turegjorup · 2024-01-15T13:49:23Z

src/Dto/EmptyDTO.php

+
+namespace App\Dto;
+
+class EmptyDTO


"EmptyDTO" ??? A short comment to explain use would be nice

Added comment.

turegjorup · 2024-01-15T13:51:37Z

.env

-OIDC_CLIENT_SECRET=ADMIN_APP_CLIENT_SECRET
-OIDC_REDIRECT_URI=ADMIN_APP_REDIRECT_URI
-OIDC_LEEWAY=30
+# ad provider


ad is specific to our planned use. We should stick to only using internal and external as terms

turegjorup · 2024-01-15T13:52:06Z

CHANGELOG.md

@@ -4,6 +4,17 @@ All notable changes to this project will be documented in this file.

 ## [Unreleased]

+- [#162](https://github.com/os2display/display-api-service/pull/162)
+  - Adds "external" openid-connect provider.
+  - Renamed "oidc" openid-connect provider to "ad".


See above about ad

README.md

turegjorup · 2024-01-15T13:57:29Z

src/Entity/Tenant/UserActivationCode.php

+    private ?\DateTimeImmutable $codeExpire;
+
+    #[ORM\Column(type: Types::STRING, nullable: false)]
+    private ?string $username;


Unique constraint? Would it ever make sense to have two codes for the same user?

If you have invitations to two different Tenants

turegjorup · 2024-01-16T09:31:59Z

src/Security/AzureOidcAuthenticator.php


            // Check if user exists already - if not create a user
-            $user = $this->entityManager->getRepository(User::class)->findOneBy(['email' => $email]);
+            $user = $this->entityManager->getRepository(User::class)->findOneBy(['providerId' => $providerId]);

            if (null === $user) {
                // Create the new user and persist it
                $user = new User();
                $user->setCreatedBy('OIDC');


Should 'OIDC' be changed to one of the UserTypeEnum values, or should we have different enum?

turegjorup · 2024-01-16T09:33:23Z

src/Command/User/AddUserCommand.php

@@ -216,9 +216,11 @@ protected function execute(InputInterface $input, OutputInterface $output): int
        // create the user and hash its password
        $user = new User();
        $user->setEmail($email);
+        $user->setProviderId($email);
        $user->setFullName($fullName);
        $user->setProvider(self::class);
        $user->setCreatedBy('CLI');


Should we have an enum for 'CLI'?

turegjorup · 2024-01-16T09:46:06Z

src/Utils/Roles.php

+
+namespace App\Utils;
+
+class Roles


What we want as output is a string so this is easier to use I think

turegjorup · 2024-01-16T09:47:57Z

tests/AbstractBaseApiTestCase.php

@@ -70,6 +74,7 @@ protected function getAuthenticatedClient(string $role = 'ROLE_EDITOR'): Client

            $user->addUserRoleTenant($userRoleTenant);

+            $manager->persist($userRoleTenant);


Should not be necessary because User has cascade: ['persist', ... on the relation

#[ORM\OneToMany(targetEntity: UserRoleTenant::class, mappedBy: 'user', cascade: ['persist', 'remove'], orphanRemoval: true)] private Collection $userRoleTenants;

tuj added 30 commits August 26, 2023 08:08

DISPLAY-993: Upgraded openid connect bundle and added external provider

88c9069

Merge branch 'develop' into feature/external-login

6755236

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-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-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

DISPLAY-993: Cleaned up code

1b74d90

DISPLAY-993: Cleanup

e406aec

DISPLAY-1010: Added tests

33f4767

DISPLAY-993: Renamed from external user to user

b8c623a

DISPLAY-993: Renamed ad oidc provider to internal

d7d04f2

DISPLAY-993: Removed SetUserType command

25eb4d0

tuj added 8 commits December 7, 2023 16:24

Merge branch 'feature/external-login-align-with-develop' into feature…

6061fe0

…/external-login

DISPLAY-1093: Fixed issues from automatic review

eff41c6

DISPLAY-1093: Changed github action for markdown

91e5506

DISPLAY-1093: Fixed markdown lint command naming

89283b6

DISPLAY-1093: Updated api path names

ef36258

DISPLAY-1093: Fixed provider repository copy-paste error

ab49103

DISPLAY-1093: Applied coding standards

df6294f

DISPLAY-1093: Updated api spec

b0f7f2b

tuj mentioned this pull request Dec 11, 2023

Feature: External Login through OIDC os2display/display-admin-client#216

Merged

4 tasks

tuj self-assigned this Dec 11, 2023

tuj added the enhancement New feature or request label Jan 7, 2024

tuj changed the title ~~Feature/external login~~ Feature: External Login through OIDC Jan 12, 2024

tuj requested a review from turegjorup January 15, 2024 09:09

turegjorup requested changes Jan 16, 2024

View reviewed changes

tuj added 4 commits January 16, 2024 17:13

#394: Changed 'ad' text to 'internal'

ebd4c6e

#394: Added unique constaint to UserActivationCode->code

53fbd74

Merge branch 'develop' into feature/external-login

8ef746e

#394: Fixed issues raised in code review

0dfc833

tuj requested a review from turegjorup January 16, 2024 17:03

tuj added 4 commits January 19, 2024 11:40

#394: Fixed issues with open api representation

ab4ba70

#394: Updated api spec

53dd1d9

#394: Added comment

414792d

#394: Removed EmptyDTO

cff03f1

turegjorup approved these changes Jan 20, 2024

View reviewed changes

tuj and others added 4 commits January 22, 2024 12:21

#394: Removed responses from open api section

171c127

#398: Fixed merge

80534ee

#398: Fixed merge

5f32fcb

Merge branch 'develop' into feature/external-login

20a8016

turegjorup merged commit 674df81 into develop Feb 26, 2024

turegjorup deleted the feature/external-login branch February 26, 2024 08:45

		@@ -70,6 +74,7 @@ protected function getAuthenticatedClient(string $role = 'ROLE_EDITOR'): Client

		$user->addUserRoleTenant($userRoleTenant);

		$manager->persist($userRoleTenant);


		namespace App\Dto;

		class EmptyDTO


		namespace App\Utils;

		class Roles

Feature: External Login through OIDC #169

Feature: External Login through OIDC #169

Uh oh!

Conversation

tuj commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to ticket

Description

Screenshots

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tuj commented Nov 9, 2023 •

edited

Loading