Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show error message to OAuth2 user when a matching local account already exists #86

Conversation

emmdurin
Copy link
Contributor

@emmdurin emmdurin commented Dec 4, 2023

If a user tries to log with OAuth2 and a local account with the same e-mail address already exists, it should be presented a clear error messages about this conflict, and the user should not considered logged in.

@pierrejego
Copy link
Member

Hello,
I am just starting testing gateway. Thansk for this work.

In my need, I want the email address to be the user id for connection. And let the user connect from where ever he wanted if he has the same email.
For exemple, compagny could have internal specific SSO (when in VPN ) and use the georchestra SSO when only on internet. The idea is to have the same account if he is in or out the compagny.
This could be the same for FranceConnect. A already georchestra user could also have a France connect user with the same mail. It could be a good idea to have only one georchestra account.

I know PR is not the place to discuss about this point, but I didn't find issue link to the PR.

Regards

@fvanderbiest
Copy link
Member

fvanderbiest commented Dec 6, 2023

I know PR is not the place to discuss about this point, but I didn't find issue link to the PR.

Hi Pierre,

Thanks for testing the software and joining the conversation, but it's a bit out of topic here.
Suggested places to interact with the community regarding the gateway and its implementation:

  • mailing list (probably more dev than general topics)
  • new issue in this repository
  • github discussion in this repository

Can you relocate your comment to one of the suggested places ?
Thanks !

@fvanderbiest
Copy link
Member

fvanderbiest commented Dec 6, 2023

it's a bit out of topic here

My bad.
Not out of topic here.

To my understanding, France Connect does not recommend to use the email as pivot data.
See eg point 16 from https://partenaires.franceconnect.gouv.fr/monprojet/recetter/

@emmdurin emmdurin force-pushed the show_error_message_to_user_when_duplicate_email_or_username_oauth2 branch from 9048c29 to b1ec30c Compare December 21, 2023 10:02
@emmdurin emmdurin force-pushed the show_error_message_to_user_when_duplicate_email_or_username_oauth2 branch from 1db4c11 to dbc4a60 Compare January 15, 2024 21:40
@@ -115,7 +115,7 @@ void testFilter_UseResolved() {

filter.filter(exchange, mockChain).block();

verify(mockChain, times(1)).filter(same(exchange));
verify(mockChain, times(2)).filter(same(exchange));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much satisfied having to change this, as this test can no longer guarantee that exchange.filter won't be called twice, even though it is not possible in tested method.

Copy link
Contributor Author

@emmdurin emmdurin Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I messed up some things between my local version and this one, here chain.filter(exchange) is really called twice, which is wrong. So I need to find a way to replace .defaultIfEmpty(chain.filter(exchange)) in gateway/src/main/java/org/georchestra/gateway/security/ResolveGeorchestraUserGlobalFilter.java by something which calls chain.filter(exchange) only if it receives an empty stream.

I tried to replace it with .switchIfEmpty(Mono.fromRunnable(() -> chain.filter(exchange))) which satisfies the test but then I can no longer access any service behind gateway (like /datahub/) when I'm not logged in, just get an empty 200 response.

@fvanderbiest fvanderbiest requested a review from groldan January 18, 2024 10:45
groldan added a commit to groldan/georchestra-gateway that referenced this pull request Mar 13, 2024
groldan added a commit to groldan/georchestra-gateway that referenced this pull request Mar 13, 2024
emmdurin pushed a commit that referenced this pull request Apr 10, 2024
@emmdurin
Copy link
Contributor Author

Closed and followed by PR#116 after a change of source branch.

@emmdurin emmdurin closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants