[17.0][IMP] auth_oidc: add groups' handling

[OdyX]

This allows groups' handling via a token's attributes as passed by either a Keycloak or a Gitlab instance serving as IdP.

• This is the 17.0-version of [IMP][12.0] auth_oidc: allow assign groups from token claims #350, then 14.0 auth OIDC groups: allow assign groups from token claims #372 , by @26houseRobot and @hbrunn , with just minor fixes

@sbidoul : I'd be happy to make any necessary changes!

[OCA-git-bot]

Hi @sbidoul,
some modules you are maintaining are being modified, check this out!

[hbrunn]

the v14 PR is based on my v12 PR which was merged - why didn't you just forward port this?

[OdyX]

@hbrunn thanks for asking! As I'm quite fresh in the Odoo ecosystem, I did not see the v12 PR. Care to share a link?

As you can see from the code, my patch works a bit differently; as it appeared that what I needed for group mapping was directly in the access token, there's no usage of the data_endpoint. But I'm also likely not fluent enough in OAuth2 to know if that is really a correct way too.

Well; in any case, I'm happy to work towards merging either this or your v12 PR (or a mix of both) for v17. We need @sbidoul 's input, right?

[hbrunn]

you find the v12 PR here

[OdyX]

@hbrunn Great. Thanks for the pointer to the v12 PR. I've now understood the code much better, and did a mostly-straightforward port, with just two minor additions as separate commits. Could you perhaps review?

[OdyX]

As the codecov warnings seem critical, I've now added some more tests around the safe_eval call of the expressions.

Edit: and now also added some groups' assignment/deassignment checks, pushing the codecov bar above the needed limits.

[hbrunn]

thanks, just minor stylistic things

[hbrunn]

@OdyX please rebase your branch. If you allow edits by maintainers, I could say

/ocabot rebase

and it would do this automatically

[OCA-git-bot]

@hbrunn The rebase process failed, because command git rebase origin/17.0 failed with output:

Rebasing (1/10)
Auto-merging auth_oidc/__manifest__.py
Auto-merging auth_oidc/demo/local_keycloak.xml
CONFLICT (content): Merge conflict in auth_oidc/demo/local_keycloak.xml
Auto-merging auth_oidc/models/auth_oauth_provider.py
CONFLICT (content): Merge conflict in auth_oidc/models/auth_oauth_provider.py
Auto-merging auth_oidc/tests/test_auth_oidc_auth_code.py
Auto-merging auth_oidc/views/auth_oauth_provider.xml
CONFLICT (content): Merge conflict in auth_oidc/views/auth_oauth_provider.xml
error: could not apply 3e0bcb53... [IMP] auth_oidc: allow assign groups from token claims
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 3e0bcb53... [IMP] auth_oidc: allow assign groups from token claims

[OdyX]

@hbrunn integrated your suggestions in afd10ea; and I have rebased this on top of 17.0.

[OdyX]

Rebased and pushed. It would be nice to have @sbidoul 's input and get this merged (or commented upon).

[sbidoul]

Sorry folks for the delay.

I've not had time to do a deep review yet, but I have a couple a questions from a first read.

[OdyX]

@sbidoul Could you perhaps take another look?

[OdyX]

@sbidoul Force-pushed a rebase with the comment suggestion included in the first commit. (twice because I forgot about pre-commit).

[sbidoul]

I'm not comfortable with replacing upstream method completely. Is there no other way? It is not clear to me why it is necessary for this PR but I have unfortunately no time to dig into this until October.

[OdyX]

@sbidoul I managed to dive back into this code again.

I'm not comfortable with replacing upstream method completely. Is there no other way?

It turns out, there is. See 77c5c2d . In there, we just delete the duplicated code from auth_oauth, and merge the id_token and userinfo data into validation.

I'd be thrilled if we could merge this in 17.0, and I'd then be happy with forward porting these to 18.0 and 19.0 as needed.

(edited: sorry for the multiple force-pushes; tests turned out to be… relevant :-) )

[OdyX]

I'm not comfortable with replacing upstream method completely. Is there no other way?

It turns out, after more tests and a (yet) better understanding of the code, doing so is actually worse than using the upstream code: the "Authorization: Bearer" is behind a system-level toggle, which makes things unnecessarily confusing and hard to debug.

I finally went with just a copy of the response checker in 89a2a5f (and I could live without that last commit).

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[OdyX]

I would really love this being merged. As far as I can tell, this is ready and just missing validation from @sbidoul .

So please remove this stale label.

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.