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

[pmauduit]

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.

[emmdurin]

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]

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]

@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]

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]

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

[pmauduit]

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

[pmauduit]

closing, as work has been done in #84