search

georchestra / georchestra-gateway

GNU General Public License v3.0
0 stars 5 forks source link

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

Closed marwanehcine closed 2 months ago

marwanehcine commented 6 months ago

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: https://github.com/georchestra/georchestra/pull/4183

pmauduit commented 6 months ago

as it is not required any more.

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

pmauduit commented 6 months ago

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.

pmauduit commented 6 months ago

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

marwanehcine commented 5 months ago

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

pmauduit commented 5 months ago

It looks like the testsuite is broken still, after having merged the required PR https://github.com/georchestra/georchestra/pull/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 commented 5 months ago

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