OIDC Team Assignment - Feedback #2150

New Issue

waza-ari · 2024-03-02T14:09:55Z

waza-ari commented

2024-03-02 14:09:55 +00:00

Description

Dear team,

after PR #1393 has been merged I was able to test this based with Keycloak - first of all, in general it works wonderfully. There's a few details that came up while testing, which we may want to address.

Team Display

Teams created via OIDC currently have the oidcID behind them, like this:

In our case, Keycloak automatically assigned a UUID4 to client roles, which are unique and not changing across the lifecycle of the client, making them a natural choice for an ID. They do make the UI clunky though.

Suggestion 1: Instead of showing the actual UUID, would it be sufficient to add a label indicating it's created through OIDC instead of having the the entire ID printed? E.g. use Bereichsleiter (OIDC) instead of Bereichsleiter (sso: 0d3c3bc7-b806-4d59-9963-8b953e4180f4)?

Somehow going into the same direction, it might be useful to also indicate the fact that Team is created via OIDC in different spots, e.g. when sharing a project with Teams. In our case we had local teams with the same name as OIDC names (sure, this can easily be fixed by deleting the local Teams), but that results in something like this:

Suggestion 2: Add the (OIDC) indicator mentioned above also in the dropdown and potentially other places?

Doc improvements

It would be useful to include in the docs that the vikunja_groups claim is expected in the id token, not in the access token. While it does make sense, I believe it might be useful to explicitly highlight this.

Also we could extend the OpenID documentation a little bit by describing the scope parameter, which is currently only described in the example config.

If you agree to these statements I'm more than happy to contribute on the doc side of things, but I'm not too familiar with Go to support with the other two.

Tagging @viehlieb as original author of the PR. Thanks for your work!

Vikunja Version

unstable

Browser and version

No response

Can you reproduce the bug on the Vikunja demo site?

No

Screenshots

No response

### Description Dear team, after PR #1393 has been merged I was able to test this based with Keycloak - first of all, in general it works wonderfully. There's a few details that came up while testing, which we may want to address. ### Team Display Teams created via OIDC currently have the `oidcID` behind them, like this: ![image](/attachments/6cdbc409-06ec-4e99-a9a7-594eb4964d86) In our case, Keycloak automatically assigned a UUID4 to client roles, which are unique and not changing across the lifecycle of the client, making them a natural choice for an ID. They do make the UI clunky though. **Suggestion 1**: Instead of showing the actual UUID, would it be sufficient to add a label indicating it's created through OIDC instead of having the the entire ID printed? E.g. use `Bereichsleiter (OIDC)` instead of `Bereichsleiter (sso: 0d3c3bc7-b806-4d59-9963-8b953e4180f4)`? ### Team Display when sharing a project Somehow going into the same direction, it might be useful to also indicate the fact that Team is created via OIDC in different spots, e.g. when sharing a project with Teams. In our case we had local teams with the same name as OIDC names (sure, this can easily be fixed by deleting the local Teams), but that results in something like this: ![image](/attachments/6d21b46f-3a6a-4350-8987-ec04abb78b43) **Suggestion 2:** Add the `(OIDC)` indicator mentioned above also in the dropdown and potentially other places? ### Doc improvements It would be useful to include in the docs that the `vikunja_groups` claim is expected in the id token, not in the access token. While it does make sense, I believe it might be useful to explicitly highlight this. Also we could extend the OpenID documentation a little bit by describing the `scope` parameter, which is currently only described in the example config. If you agree to these statements I'm more than happy to contribute on the doc side of things, but I'm not too familiar with Go to support with the other two. Tagging @viehlieb as original author of the PR. Thanks for your work! ### Vikunja Version unstable ### Browser and version _No response_ ### Can you reproduce the bug on the Vikunja demo site? No ### Screenshots _No response_

image.png

114 KiB

image.png

32 KiB

waza-ari added the

kind/bug

label 2024-03-02 14:09:56 +00:00

konrad referenced this issue from a commit

2024-03-02 14:27:51 +00:00

chore(auth): add oidc suffix to openid team name in db

konrad commented

2024-03-02 14:29:12 +00:00

Wow, that was fast!

I've added the display changes in fc4303a778. Please check with the next unstable build (should be ready in ~45min, when the ci passed). You'll have to log out and log back in again to see the changes.

I'd love a PR for the docs.

Wow, that was fast! I've added the display changes in fc4303a7786fa2b78655b55763347eacb730d428. Please check with the next unstable build (should be ready in ~45min, [when the ci passed](https://drone.kolaente.de/vikunja/vikunja/6079)). You'll have to log out and log back in again to see the changes. I'd love a PR for the docs.

❤️ 1

konrad added

kind/feature

and removed

kind/bug

labels 2024-03-02 14:29:20 +00:00

waza-ari commented

2024-03-02 15:19:15 +00:00

Can only return this, that was fast! Tested it just now and that looks so much better. Thank you!

Working on a PR for the docs, should come through in the next hour or so.

Can only return this, that was fast! Tested it just now and that looks so much better. Thank you! Working on a PR for the docs, should come through in the next hour or so.

waza-ari referenced this issue

2024-03-02 16:20:16 +00:00

docs: improve OpenID documentation #2151

waza-ari commented

2024-03-02 16:21:39 +00:00

I've created #2151 to add more details about the OIDC implementation. One request for feedback:

The section in config.md currently only defines the entire auth.openid block, but doesn't describe the individual fields. I'm open to add details there as well, but would like to understand if there's environment variables for those settings and if so, how they're configured. Thanks!

I've created #2151 to add more details about the OIDC implementation. One request for feedback: The section in `config.md` currently only defines the entire `auth.openid` block, but doesn't describe the individual fields. I'm open to add details there as well, but would like to understand if there's environment variables for those settings and if so, how they're configured. Thanks!

waza-ari referenced this issue

2024-03-02 17:41:04 +00:00

fix: OIDC team comparison should not include the team name #2152

waza-ari commented

2024-03-02 17:42:54 +00:00

@konrad I believe your change above broke the automatic team creation. During login, the code did a check if a team already exists, if not, the team is created. To check for existence, the code so far did compare both the oidcID and the name, which now obviously doesn't match anymore as you're adding the (OIDC) suffix.

IMHO oidcID checking should be enough (that's why we have this unique after all), therefore I'm proposing to change this in #2152, which should fix the issue introduced here.

@konrad I believe your change above broke the automatic team creation. During login, the code did a check if a team already exists, if not, the team is created. To check for existence, the code so far did compare both the `oidcID` and the `name`, which now obviously doesn't match anymore as you're adding the `(OIDC)` suffix. IMHO `oidcID` checking should be enough (that's why we have this unique after all), therefore I'm proposing to change this in #2152, which should fix the issue introduced here.

👍 1

konrad commented

2024-03-03 21:44:13 +00:00

The section in config.md currently only defines the entire auth.openid block, but doesn't describe the individual fields. I'm open to add details there as well, but would like to understand if there's environment variables for those settings and if so, how they're configured. Thanks!

Currently, it's not possible to configure this with environment variables. The documentation of the individual variables is configured from the config.yml.sample file and the comments it contained. You shouldn't edit the markdown file manually.

> The section in config.md currently only defines the entire auth.openid block, but doesn't describe the individual fields. I'm open to add details there as well, but would like to understand if there's environment variables for those settings and if so, how they're configured. Thanks! Currently, it's not possible to configure this with environment variables. The documentation of the individual variables is configured from the `config.yml.sample` file and the comments it contained. You shouldn't edit the markdown file manually.

waza-ari commented

2024-03-03 21:47:53 +00:00

Currently, it's not possible to configure this with environment variables. The documentation of the individual variables is configured from the config.yml.sample file and the comments it contained. You shouldn't edit the markdown file manually.

Yep I realised that after my first commit. The final state of the PR is however generated from config.yml.sample using mage generate-docs, so should be good from that perspective.

> Currently, it's not possible to configure this with environment variables. The documentation of the individual variables is configured from the `config.yml.sample` file and the comments it contained. You shouldn't edit the markdown file manually. Yep I realised that after my first commit. The final state of the PR is however generated from `config.yml.sample` using ` mage generate-docs`, so should be good from that perspective.

👍 1

konrad referenced this issue from a commit

2024-03-03 22:19:28 +00:00

docs: improve OpenID documentation (#2151)

konrad referenced this issue from a commit

2024-03-04 20:26:57 +00:00

fix(auth): use (issuer, name) to check for uniqueness of oidc teams (#2152)

waza-ari commented

2024-03-04 22:48:45 +00:00

Just for completeness, deployed new unstable build to our environment, works wonderfully now. Thanks for the support!

❤️ 1

waza-ari closed this issue