-
Notifications
You must be signed in to change notification settings - Fork 8
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
Introducing a sec-external-authentication
flag http header to identify local vs remote users
#101
Conversation
it is still, for users using the georchestra LDAP and the usual form. |
aa62501
to
9290fae
Compare
gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java
Outdated
Show resolved
Hide resolved
...way/src/main/java/org/georchestra/gateway/security/preauth/PreauthAuthenticationManager.java
Outdated
Show resolved
Hide resolved
...java/org/georchestra/gateway/filter/headers/providers/GeorchestraUserHeadersContributor.java
Outdated
Show resolved
Hide resolved
gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java
Outdated
Show resolved
Hide resolved
gateway/src/main/java/org/georchestra/gateway/model/HeaderMappings.java
Outdated
Show resolved
Hide resolved
...way/src/main/java/org/georchestra/gateway/security/preauth/PreauthAuthenticationManager.java
Outdated
Show resolved
Hide resolved
...way/src/main/java/org/georchestra/gateway/security/preauth/PreauthAuthenticationManager.java
Outdated
Show resolved
Hide resolved
...way/src/main/java/org/georchestra/gateway/security/preauth/PreauthAuthenticationManager.java
Outdated
Show resolved
Hide resolved
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 |
528bf7c
to
b575cf0
Compare
There was a problem hiding this 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)
gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java
Outdated
Show resolved
Hide resolved
8fc73a0
to
6443fce
Compare
6443fce
to
b7ce457
Compare
sec-external-authentication
flag http header to identify local vs remote users
I allowed myself to rework a bit the title & description of this PR to make it clearer for other reviewers. |
b7ce457
to
e4081b1
Compare
…ing from an external IDP
e4081b1
to
d5ef0fa
Compare
...java/org/georchestra/gateway/filter/headers/providers/GeorchestraUserHeadersContributor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
11764eb
to
4305290
Compare
4305290
to
9d46a7f
Compare
It was very useful to implement unit/IT tests. Thank you @pmauduit |
9d46a7f
to
c085e25
Compare
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:
I have got the two following ones locally, checking out your branch:
|
@pmauduit , checks OK now. Merge if agreed. Thanks |
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