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

Introducing a sec-external-authentication flag http header to identify local vs remote users #101

Conversation

marwanehcine
Copy link
Contributor

@marwanehcine marwanehcine commented Feb 4, 2024

This PR aims to introduce a new HTTP header named sec-external-authentication, which would be set to true if the user is connected externally (either via OIDC/Oauth2 or preauthentication) or false is authenticated via a local geOrchestra LDAP.

Prior to merging this PR, the following one will be required, as an update of the GeorchestraUser object will be needed:
georchestra/georchestra#4183

datadir/default.properties Outdated Show resolved Hide resolved
datadir/nginx-preauth/nginx.conf Outdated Show resolved Hide resolved
@pmauduit
Copy link
Member

pmauduit commented Feb 6, 2024

as it is not required any more.

it is still, for users using the georchestra LDAP and the usual form.

@marwanehcine marwanehcine marked this pull request as draft February 12, 2024 19:38
@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch 2 times, most recently from aa62501 to 9290fae Compare February 14, 2024 07:21
@marwanehcine marwanehcine marked this pull request as ready for review February 15, 2024 08:57
@pmauduit
Copy link
Member

I'd also suggest to update the documentation here: https://github.com/georchestra/georchestra-gateway/blob/main/docs/authzn.adoc to add a paragraph about the sec-external-auth the pull-request provides.

@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch 4 times, most recently from 528bf7c to b575cf0 Compare February 19, 2024 21:42
Copy link
Member

@pmauduit pmauduit left a comment

Choose a reason for hiding this comment

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

I am wondering if we could not use a boolean instead of a string, I would expect the type to be a boolean, when the method / attribute is called "*isExternalAuth".

Also maybe the commit message would deserve something more meaningful, like "adding a flag in the HTTP headers to indicate whether the user is coming from an external identity provider" (as it is not only about preauth mode)

@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch 3 times, most recently from 8fc73a0 to 6443fce Compare February 20, 2024 11:21
@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch from 6443fce to b7ce457 Compare February 22, 2024 09:21
@pmauduit pmauduit changed the title handle password management for preauth mode Introducing a sec-external-authentication flag http header to identify local vs remote users Feb 23, 2024
docs/authzn.adoc Outdated Show resolved Hide resolved
@pmauduit
Copy link
Member

I allowed myself to rework a bit the title & description of this PR to make it clearer for other reviewers.

@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch from b7ce457 to e4081b1 Compare February 25, 2024 17:49
@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch from e4081b1 to d5ef0fa Compare February 25, 2024 18:01
Copy link
Member

@pmauduit pmauduit left a comment

Choose a reason for hiding this comment

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

Maybe it'd have been worth adding a IT based on the ones existing for the preauth already, but apart from that, LGTM.

@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch from 11764eb to 4305290 Compare March 7, 2024 20:57
@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch from 4305290 to 9d46a7f Compare March 7, 2024 21:02
@marwanehcine
Copy link
Contributor Author

marwanehcine commented Mar 7, 2024

Maybe it'd have been worth adding a IT based on the ones existing for the preauth already, but apart from that, LGTM.

It was very useful to implement unit/IT tests.

Thank you @pmauduit

@marwanehcine marwanehcine force-pushed the handle_password_management_for_preauth_mode branch from 9d46a7f to c085e25 Compare March 7, 2024 21:12
@pmauduit pmauduit self-requested a review March 8, 2024 09:34
@pmauduit
Copy link
Member

pmauduit commented Mar 8, 2024

It looks like the testsuite is broken still, after having merged the required PR georchestra/georchestra#4183

The GHA reports the following one to be broken:

[ERROR]   GeorchestraUserHeadersContributorTest.testNoUser:89 expected: <true> but was: <false>

I have got the two following ones locally, checking out your branch:

[ERROR] Failures: 
[ERROR]   GeorchestraGatewayApplicationTests.errorCustomizerReturnsServiceUnavailableInsteadOfServerError:117 Status expected:<503> but was:<500>
[ERROR]   GeorchestraUserHeadersContributorTest.testNoUser:89 expected: <true> but was: <false>

@marwanehcine
Copy link
Contributor Author

It looks like the testsuite is broken still, after having merged the required PR georchestra/georchestra#4183

The GHA reports the following one to be broken:

[ERROR]   GeorchestraUserHeadersContributorTest.testNoUser:89 expected: <true> but was: <false>

I have got the two following ones locally, checking out your branch:

[ERROR] Failures: 
[ERROR]   GeorchestraGatewayApplicationTests.errorCustomizerReturnsServiceUnavailableInsteadOfServerError:117 Status expected:<503> but was:<500>
[ERROR]   GeorchestraUserHeadersContributorTest.testNoUser:89 expected: <true> but was: <false>

@pmauduit , checks OK now. Merge if agreed. Thanks

@f-necas f-necas merged commit 28da9b1 into georchestra:main Jun 13, 2024
3 checks passed
@pmauduit pmauduit deleted the handle_password_management_for_preauth_mode branch June 13, 2024 08:27
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.

3 participants