local account creation for user connected with external identity provider #45

marwanehcine · 2023-06-26T00:33:29Z

On this PR, we are creating a new LDAP account for user connected using external IDP (Google, Mel, FranceConnect).
When user connect for first time, a new LDAP account is created. After that, user information (firstname, lastname...) will be read from user LDAP account.
External IDP users can modify password and connect using login/password page.
For FranceConnect, an EndSessionURL is added to perform correct logout process.
Create e new LDAP account is optional depending on property "georchestra.gateway.security.createNonExistingUsersInLDAP"

pmauduit

Thanks @marwanehcine, early review from my side: One conception concern on the PR is that it tightly couples oauth2 to the presence of a geOrchestra LDAP in the infrastructure. While it would probably be the case, it won't be necessary to create accounts in some cases (e.g.: DT only uses the geOrchestra LDAP for internal / m2m accounts purposes, in their case the ability to extract roles from the JSON user payload being returned by the identity provider is sufficient).

Would it be possible to add an option to the oauth2 client definitions like createNonExistingUsersInTheLDAP: true or something ? Also asking @groldan about this.

pmauduit · 2023-06-27T10:43:34Z

gateway/src/main/java/org/georchestra/gateway/security/oauth2/OAuth2UserMapper.java

+import org.georchestra.ds.users.DuplicatedEmailException;
+import org.georchestra.ds.users.DuplicatedUidException;


needed imports ? no code change in the file afterwards

ok will check an remove

emmdurin · 2023-06-28T15:27:29Z

With FranceConnect, e-mail address should not be used as a unique identifier, as same user may have a different e-mail address depending on the FranceConnect identity provider he chooses to login using FranceConnect. So we should use another unique identifier containing identity provider name and OAuth2 subject value, which I propose in this commit, so that such a user will not have 2 different Georchestra account.

The problem with that is if the user login using OAuth2 with 2 different providers with same e-mail address associated, second account creation will fail because even if UID does not exists, there is another account that has the same e-mail address. We may address this problem by searching for an existing account by UID or by e-mail, taking the one which answers positively, or create a new account if all two answers negatively.

I also think that we should treat theses automatically created accounts differently on some cases. The user should not be able to see his login UID, to change password, or to login using login/password form. Header should also display something specific to this case, but I do not know what at the moment. We may address this by adding a new sec-XXXXX header to forwarded requests so that services know that this is an auto-generated account.

pmauduit · 2023-07-05T08:03:46Z

...org/georchestra/gateway/security/oauth2/CustomOAuth2ClientPropertiesRegistrationAdapter.java

+        if (registrationId.equals("franceconnect")) {
+            Map<String, Object> configurationMetadata = Collections.singletonMap("end_session_endpoint",
+                    "https://fcp.integ01.dev-franceconnect.fr");
+            map.from(configurationMetadata).to(builder::providerConfigurationMetadata);
+        }


I think we have no choice than having hardcoded things for franceconnect unfortunately, but won't this be an issue when switching to "franceconnect-production" ? integ01.dev sounds quite dedicated to development purposes

Discussed during the daily, another approach needs to be tested by @marwanehcine

groldan · 2023-07-14T02:34:26Z

hey sorry I just stumbled upon this pr right now.
My first comment is I find it difficult to reason about it. Please add a meaningful commit message and PR description.
Aside, I don't think creating accounts should be the responsibility of the gateway.
Now, without a deeper understanding of the pr's intent and the fact that's 23:30 already here I don't feel capable of providing a better feedback, but at least let's discuss it and see if we can find an alternative.

marwanehcine · 2023-07-14T10:07:24Z

Hello @groldan, sorry if you find PR difficult to undrestand. The goal of PR is to create local LDAP account for users who connect using Google, Mel, FranceConnect. From second user login, users information will be loaded from LDAP account.
Will be waiting for your questions/feedbacks.
Thanks

marwanehcine · 2023-07-14T11:42:45Z

Hello @pmauduit , I have add option "createNonExistingUsersInTheLDAP" to enable local account creation, however when createNonExistingUsersInTheLDAP=false, what about "userDetails" page, this page cannot be shown unless a local account is created and also items was not shown to user since the is no local account.
We need to handle these cases if we will go forward making local account creation optional.
Thanks

pmauduit · 2023-07-17T07:21:29Z

gateway/src/main/java/org/georchestra/gateway/security/oauth2/OAuth2Configuration.java

-@PropertySource("file:${georchestra.datadir}/gateway/security.yaml")
+@PropertySource("file:${georchestra.datadir}/default.properties")


don't we still need the security.yaml file ?

pmauduit · 2023-07-17T07:43:52Z

Aside, I don't think creating accounts should be the responsibility of the gateway.

@groldan why not ? The gateway is the "entrypoint" for accessing the application, it sounds as the only place being aware of who is connecting how (when it deals with user authentication / authorization & external identity providers). If we want the administrators to be able of managing external users to e.g. ban an account, promote the user to more than ROLE_USER, ... we have to keep track of them, and one solution which sounded natural to us was to create the account into the openLDAP (which is the actual reference database for our users). I think that most online services which provide the possibility to use external 3rd party identity providers do function the same way (thinking of strava, but probably applies to others, they do keep my users info attached to my google account).

We are opened to discussion if you have a more relevant approach in mind

pmauduit · 2023-07-21T09:13:16Z

gateway/src/main/java/org/georchestra/gateway/security/oauth2/OpenIdConnectUserMapper.java

+                } catch (DuplicatedUidException e) {
+                } catch (DuplicatedEmailException e) {
+                } catch (DataServiceException e) {
+                }


to be revisited later on ?

Yes, but do you know what to do in case of Exception ? What should happen to the end user ?

pmauduit · 2023-07-21T09:14:29Z

gateway/src/test/resources/test-datadir/gateway/security.yaml

+            clientId: 730144348959-uudptrtn5a910g8scktj7s35um9fians.apps.googleusercontent.com
+            clientSecret: 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


sensitive data to be removed ?

marwanehcine force-pushed the account_creation_with_external_identity_provider branch 2 times, most recently from 26920b6 to fedd393 Compare June 27, 2023 08:01

pmauduit reviewed Jun 27, 2023

View reviewed changes

emmdurin marked this pull request as draft June 28, 2023 15:28

pmauduit reviewed Jul 5, 2023

View reviewed changes

marwanehcine force-pushed the account_creation_with_external_identity_provider branch from 2fa3a88 to d358906 Compare July 14, 2023 01:00

pmauduit reviewed Jul 17, 2023

View reviewed changes

marwanehcine force-pushed the account_creation_with_external_identity_provider branch 2 times, most recently from 19b6eab to 6063186 Compare July 20, 2023 22:20

marwanehcine requested review from emmdurin and pmauduit July 21, 2023 08:11

marwanehcine marked this pull request as ready for review July 21, 2023 08:14

pmauduit reviewed Jul 21, 2023

View reviewed changes

marwanehcine requested a review from pmauduit July 21, 2023 09:35

emmdurin force-pushed the account_creation_with_external_identity_provider branch 3 times, most recently from 3ea122f to fa62071 Compare July 31, 2023 16:43

fvanderbiest self-requested a review August 7, 2023 09:50

Update to allow local account creation for IDP logins

3a5d74c

emmdurin force-pushed the account_creation_with_external_identity_provider branch from 83fe191 to 3a5d74c Compare August 8, 2023 20:23

emmdurin approved these changes Aug 8, 2023

View reviewed changes

emmdurin merged commit e112870 into georchestra:main Aug 8, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

local account creation for user connected with external identity provider #45

local account creation for user connected with external identity provider #45

marwanehcine commented Jun 26, 2023 •

edited

Loading

pmauduit left a comment •

edited

Loading

pmauduit Jun 27, 2023

marwanehcine Jun 27, 2023

emmdurin commented Jun 28, 2023 •

edited

Loading

pmauduit Jul 5, 2023

pmauduit Jul 5, 2023

groldan commented Jul 14, 2023

marwanehcine commented Jul 14, 2023

marwanehcine commented Jul 14, 2023

pmauduit Jul 17, 2023

pmauduit commented Jul 17, 2023 •

edited

Loading

pmauduit Jul 21, 2023

emmdurin Jul 21, 2023

pmauduit Jul 21, 2023

emmdurin Jul 21, 2023

		import org.georchestra.ds.users.DuplicatedEmailException;
		import org.georchestra.ds.users.DuplicatedUidException;

		@PropertySource("file:${georchestra.datadir}/gateway/security.yaml")
		@PropertySource("file:${georchestra.datadir}/default.properties")

		clientId: 730144348959-uudptrtn5a910g8scktj7s35um9fians.apps.googleusercontent.com
		clientSecret: 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

local account creation for user connected with external identity provider #45

local account creation for user connected with external identity provider #45

Conversation

marwanehcine commented Jun 26, 2023 • edited Loading

pmauduit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmdurin commented Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groldan commented Jul 14, 2023

marwanehcine commented Jul 14, 2023

marwanehcine commented Jul 14, 2023

Choose a reason for hiding this comment

pmauduit commented Jul 17, 2023 • edited Loading

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

marwanehcine commented Jun 26, 2023 •

edited

Loading

pmauduit left a comment •

edited

Loading

emmdurin commented Jun 28, 2023 •

edited

Loading

pmauduit commented Jul 17, 2023 •

edited

Loading