8000 UI should prevent admins from defining a dynamic client scope with type Default by mabartos · Pull Request #48973 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

UI should prevent admins from defining a dynamic client scope with type Default - #48973

Merged
rmartinc merged 3 commits into
keycloak:mainfrom
mabartos:KC-22229
May 18, 2026
Merged

UI should prevent admins from defining a dynamic client scope with type Default#48973
rmartinc merged 3 commits into
keycloak:mainfrom
mabartos:KC-22229

Conversation

@mabartos
@mabartos mabartos commented May 13, 2026 •
edited
Loading
Copy link
Copy Markdown
Member
  • Closes UI should prevent admins from defining a dynamic client scope with type Default #22229
  • Improves the help messages
  • Add checks on the server side to not contain the Default type for dynamic client scopes
  • If there's already some existing dynamic scope with the default type, the UI set the type to Optional, and user need to save it explicitly
  • (optional) Add badge for the dynamic scopes in the UI (not sure if we need to localize it? I'd say not as we don't have anything existing...) - I can create a separate issue
Screenshot From 2026-05-13 17-50-27 Screenshot From 2026-05-13 17-50-06 Screenshot From 2026-05-13 17-49-52

@mabartos mabartos mentioned this pull request May 13, 2026
Closed
@mabartos
mabartos marked this pull request as ready for review May 13, 2026 19:08
@mabartos
mabartos requested review from a team as code owners May 13, 2026 19:08
@mabartos mabartos self-assigned this May 13, 2026
@mabartos
mabartos requested review from edewit and rmartinc May 13, 2026 19:09
@mabartos
mabartos commented May 13, 2026
Copy link
Copy Markdown
Member Author

@edewit Could you please check it? Thanks!

rmartinc
rmartinc reviewed May 14, 2026
View reviewed changes
@rmartinc rmartinc left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @mabartos! In general I see this OK. But probably with these changes we need to change the client scope to non-dynamic on an update in the admin console when the feature is disabled. If not there can be errors for sure when updating (because the None is allowed in the console but disallowed in the endpoint).

Maybe we can add something like this in the ScopeForm.tsx:

  useEffect(() => {
    if (!isDynamicScopesEnabled && dynamicScope === "true") {
      setValue(
        convertAttributeNameToForm<ClientScopeDefaultOptionalType>(
          "attributes.is.dynamic.scope",
        ),
        "false",
      );
    }
  }, [setValue, dynamicScope, isDynamicScopesEnabled]);

The idea would be that, if you update a previously dynamic scope, is transformed to non-dynamic in the admin console. At endpoint is OK to return the error.

WDYT?

@mabartos
UI should prevent admins from defining a dynamic client scope with ty…
ad91155 
…pe Default

Closes keycloak#22229

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
@mabartos
mabartos marked this pull request as draft May 14, 2026 10:05
@mabartos
mabartos commented May 14, 2026 •
edited
Loading
Copy link
Copy Markdown
Member Author

@rmartinc Works to me. However, it's quite fragile to just change the dynamic scope to the normal one without notice. Maybe the intention was to use it only as the dynamic scope? In order to be sure, I just added a small warning, with the information.

Demo:

  • Created dynamic scope with the feature enabled
  • Disable the feature
  • Go to the dynamic scope
  • The message is "This scope was previously configured as dynamic, but the Dynamic Scopes feature is currently disabled. Saving will convert it to a regular scope, or consider removing it." (not covered in the screencast)
Screencast.From.2026-05-14.12-11-14.mp4

@rmartinc WDYT?

Would be nice if @edewit could review it, but if not, I think it's not a big deal as there should not be any risk.

@mabartos
mabartos marked this pull request as ready for review May 14, 2026 10:24
@mabartos
mabartos marked this pull request as draft May 14, 2026 12:58
rmartinc
rmartinc approved these changes May 14, 2026
View reviewed changes
@rmartinc rmartinc left a comment •
edited
Loading
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @mabartos! But the test seems to be related.
Do you want @edewit to also check it?

rmartinc
rmartinc requested changes May 14, 2026
View reviewed changes
@rmartinc rmartinc left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mabartos The admin test seems to be related. Please check it.

@mabartos
Add Alert, and change to non-dynamic scope
564ac90 
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
@mabartos
Unify the dynamic label usage
3e922c9 
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
@mabartos
mabartos marked this pull request as ready for review May 14, 2026 15:24
@mabartos
mabartos requested a review from rmartinc May 15, 2026 07:47
rmartinc
rmartinc approved these changes May 18, 2026
View reviewed changes
@rmartinc rmartinc left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the changes @mabartos!

@rmartinc
rmartinc merged commit 5621e7f into keycloak:main May 18, 2026
87 checks passed
@stianst stianst mentioned this pull request Jun 1, 2026
Closed
sashyo pushed a commit to tide-foundation/keycloak-IGA that referenced this pull request Jul 20, 2026
@mabartos @sashyo
UI should prevent admins from defining a dynamic client scope with ty…
34e9d99 
…pe Default (keycloak#48973)

* UI should prevent admins from defining a dynamic client scope with type Default

Closes keycloak#22229

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

* Add Alert, and change to non-dynamic scope

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

* Unify the dynamic label usage

Signed-off-by: Martin Bartoš <mabartos@redhat.com>

---------

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rmartinc rmartinc rmartinc approved these changes
@edewit edewit Awaiting requested review from edewit

Assignees

@mabartos mabartos

Labels

team/core-authn

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UI should prevent admins from defining a dynamic client scope with type Default

2 participants

@mabartos @rmartinc
0