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

ACLs - Making use of resolved GeorchestraUser's roles along with the resolved authorities #89

Closed
wants to merge 7 commits into from

Conversation

pmauduit
Copy link
Member

When defining accesses rules to the targets, e.g.:

georchestra:
  gateway:
    services:
      ServiceName:
        access-rules:
          - intercept-url: /echo/administrator
            allowed-roles: ADMINISTRATOR
          - intercept-url: /echo/connected
            anonymous: false
          - intercept-url: /echo/anonymous
            anonymous: true

Initially, only the Authentication.Authorities were used to determine if the user was member of the different roles. But we also resolve the geOrchestra roles onto the GeorchestraUser.Roles object created, no matter if the user is coming from an external OIDC provider, is preauthenticated via another upstream proxy server via http headers, or via the classical geOrchestra LDAP. These roles were not used for access evaluation, though.

The intent of this pull-request is to also check the accesses to the proxified targets against the GeorchestraUser's roles being resolved.

Note: this work was already done onto Deutsche-Telekom georchestra-gateway fork and has been backported here.

@pmauduit pmauduit requested a review from groldan December 11, 2023 09:29
@pmauduit pmauduit changed the title ACLs - Making use of resolved GeorchestraRoles along with the resolved authorities ACLs - Making use of resolved GeorchestraUSer's roles along with the resolved authorities Dec 11, 2023
@pmauduit pmauduit changed the title ACLs - Making use of resolved GeorchestraUSer's roles along with the resolved authorities ACLs - Making use of resolved GeorchestraUser's roles along with the resolved authorities Dec 11, 2023
@emmdurin
Copy link
Contributor

emmdurin commented Dec 12, 2023

I can confirm the problem is solved, but getting theses changes from DT fork brought back many changes that cancels changes that were made to this main branch.

First commit is sufficient to resolve the problem encountered in PR#84. However other changes are interesting in general case, we may use this current PR to add them to main branch, after fixing some regressions they cause.

@pmauduit
Copy link
Member Author

However other changes are interesting in general case, we may use this current PR to add them to main branch, after fixing some regressions they cause.

So, is that ok if we merge this one, and see afterwards for regressions ? Are there any way of having possible regressions being tested ?

@groldan
Copy link
Member

groldan commented Dec 13, 2023

@emmdurin it'd be great if you had a list of regressions so we can fix them. Especially if we can add tests to ensure they don't happen again.
When working on adding new functionality or refactoring, the approval criteria must include that no current tests are broken.

@emmdurin
Copy link
Contributor

emmdurin commented Dec 13, 2023

So, is that ok if we merge this one, and see afterwards for regressions ? Are there any way of having possible regressions being tested ?

No, we have to fix regressions first. But we can merge PR#84 which contains the fix for the problem encountered, as soon as there is an approved review, do not hesitate to review it if you want.

Then in this PR#89 will remains some interesting changes that should be useful in the future but are not needed for now. We can merge them when regressions are fixed.

@emmdurin it'd be great if you had a list of regressions so we can fix them. Especially if we can add tests to ensure they don't happen again. When working on adding new functionality or refactoring, the approval criteria must include that no current tests are broken.

I will check this when I have time, as I stated above there is no hurry. Just now I can tell that OAuth2 configurable logout endpoint code has been removed, and custom JWT decoding needed for some cases has also been removed (trying to use FranceConnect crashes because of a missing JWT URI endpoint), and there may be other problems. About testing, for example writing a test for the first one should be quite easy, but for the second one it's quite harder. So you're right pointing that some tests are missing, but I'm not sure how much we can test all of the changes we did on OAuth2 authentication process.

Theses regressions are introduced because of the changes me and Marwane made to this repository but that were not replicated to DT repository. So when some code is retrieved back from DT repository, it cancels some of theses changes. This problem has already occurred.

@pmauduit
Copy link
Member Author

pmauduit commented Jan 26, 2024

I can't remember if this PR is still needed somewhere, TBH ...

groldan and others added 7 commits January 31, 2024 16:29
Clarify treatment of Authentication authority names and resolved
`GeorchestraUser` role names.

The `Authentication` authority names are not changed at all, whereas
previously they where being appended the `ROLE_` prefix only when the
authentication came from LDAP.

Instead, leave the `Authentication` object always unchanged and use a
`GeorchestraUserCustomizerExtension`
(`RolePrefixGeorchestraUserCustomizerExtension`), to ensure all
`GeorchestraUser` roles are prefixed, since those are the ones to be
sent to the downstream services in the `sec-roles` header.
Allows to normalize role names coming from any Authentication provider
authority names. Previously it only worked for Ldap.

Configuration change:

`georchestra.gateway.security.oidc.*` properties moved as child of `oauth2`:
`georchestra.gateway.security.oauth2.oidc.*`
* Testsuite broken because sending a request with no Accept header (or
  "Accept: */*") will make spring sending a 401 instead of a 302
  redirect to /login
* restoring previous work on csrf/cors not present in DT's fork (#73, #59)
* restoring some other works related to login/logout controllers/templates
@pmauduit
Copy link
Member Author

rebased onto main today, but I'm getting confused, maybe I missed some recent modifications introduced from other PRs ...

@pmauduit
Copy link
Member Author

pmauduit commented Feb 7, 2024

closing, as work has been done in #84

@pmauduit pmauduit closed this Feb 7, 2024
@f-necas f-necas deleted the gitlab_merge branch June 26, 2024 10:03
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