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

Returns ExtendedGeorchestraUser object when createUserInLdap set to true #114

Merged

Conversation

pmauduit
Copy link
Member

@pmauduit pmauduit commented Mar 29, 2024

Considering the following configuration scenario:

  • external authentication (oidc/oauth2 or pre-auth) being configured
  • createUsersInGeorchestraLdap set to true

Then the resolved GeorchestraUser should be an ExtendedGeorchestraUser, in order to have a behaviour coherent with the geOrchestra LDAP authentication (via the classic login form provided by the gateway).

Without doing so, users externally authenticated will resolve as a classic GeorchestraUser, leading to missing http headers and breaking some geOrchestra applications (e.g. datafeeder, which requires the sec-orgname provided only when resolving to an ExtendedGeorchestraUser).

This also refactors the LdapConfigProperties to GeorchestraGatewaySecurityConfigProperties, as the object is not only about LDAP, but also nests some other configureable features (OIDC, ...).

Documentation has been updated to describe / explain the behaviour.

Tests:

  • testsuite adapted
  • added specific tests case scenario
  • Runtime tested onto a setup making use of pre-authentication: as a member of the IMPORT role, being externally authenticated, I was able to import a dataset following the datafeeder wizard.

@pmauduit pmauduit requested a review from groldan March 29, 2024 10:37
@pmauduit pmauduit force-pushed the extended-geor-users-when-create-account-in-ldap-true branch from 0bebc62 to 3e41367 Compare April 12, 2024 17:33
@pmauduit pmauduit force-pushed the extended-geor-users-when-create-account-in-ldap-true branch from 3e41367 to c0f1a68 Compare April 29, 2024 13:57
Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

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

I've added a couple small commits. Looks good to me.
@pmauduit if you prefer so and it looks good to you, squash-merge them into your original commit.
Feel free to merge

Considering the following configuration scenario:

* external authentication (oidc/oauth2 or pre-auth) being configured
* `createUsersInGeorchestraLdap` set to true

Then the resolved GeorchestraUser should be an
`ExtendedGeorchestraUser`, in order to have a behaviour coherent with
the extended geOrchestra LDAP authentication.

Without doing so, users externally authenticated will resolve as a
classic GeorchestraUser, leading to missing http headers and breaking
some geOrchestra applications (e.g. datafeeder, which requires the
`sec-orgname` provided only when resolving to an
`ExtendedGeorchestraUser`).

This also refactors the LdapConfigProperties to
GeorchestraGatewaySecurityConfigProperties, as the object is not only
about LDAP, but also nests some other configureable features (OIDC,
...).

Documentation has been updated to describe / explain the behaviour.

Tests:
* testsuite adapted
* added specific tests case scenario
@pmauduit pmauduit force-pushed the extended-geor-users-when-create-account-in-ldap-true branch from a4f5563 to 204f91a Compare May 7, 2024 07:59
@pmauduit
Copy link
Member Author

pmauduit commented May 7, 2024

Thanks @groldan for the review

squash-merge them into your original commit.

👍

@pmauduit pmauduit merged commit adea34c into main May 7, 2024
3 checks passed
@pmauduit pmauduit deleted the extended-geor-users-when-create-account-in-ldap-true branch May 7, 2024 08:03
@pmauduit
Copy link
Member Author

pmauduit commented May 7, 2024

thymeleaf templates for e.g. login are not resolved anymore after this merge, I cannot explain why for now, but hitting /login will return the "login" string as response body.

@pmauduit
Copy link
Member Author

pmauduit commented May 7, 2024

pmauduit added a commit that referenced this pull request May 7, 2024
See #114 (comment)

Using RestController will disable thymeleaf template integration,
reverting back to Controller annotation instead.

Tests: runtime in the docker composition provided at the root of the
reppository.
pmauduit added a commit that referenced this pull request May 7, 2024
…er-merge-114

login - fix thymeleaf integration (reverts a modification from #114)
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.

2 participants