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

Dev UI: OIDC Updates #34002

Conversation

phillip-kruger
Copy link
Member

This PR fix 3 things in the OIDC Screen

  • Allow the external link to the admin screen to be in the submenu
  • Make sure that the theme is working in OIDC
  • Fix the GraphQL Link in OIDC

oicd

@michalvavrik this might need some more work for other log in types. Is this something you can look at within a separate PR ?

@michalvavrik
Copy link
Member

michalvavrik commented Jun 13, 2023

hey @phillip-kruger, thanks for improvements. I'll test your changes today and get back to you here.

this might need some more work for other log in types. Is this something you can look at within a separate PR ?

Sure, if there is additional work to do, I am happy to have a look.

@michalvavrik
Copy link
Member

well will also need someone who can approve this when review is done, so cc @sberyozkin

@sberyozkin
Copy link
Member

Thanks very much @phillip-kruger @michalvavrik, let me give it a quick try, a little bit later, it looks good

@michalvavrik
Copy link
Member

@sberyozkin I actually had a look at changes and it's fine with me. Changes in appearance are made that depends on your opinion how it should look like, how container should be aligned... If we say there is one right behavior or this is how you want it to look, I'm fine with it. Personally, I don't give a damn about where things are aligned or what kind a green there is.

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

@michalvavrik Sounds good, I was building Phillip's branch, but yeah, I could've just had a look at the image provided :-), oh dear, I'm doing the review the difficult way :-).

@michalvavrik
Copy link
Member

michalvavrik commented Jun 13, 2023

this might need some more work for other log in types. Is this something you can look at within a separate PR ?

@phillip-kruger I checked other grant types, there is no palpable difference to your changes in appearance that are showed in image you provided. There are things like very wide login button/test svc button, but let's wait for Sergey's feedback on what he prefer. I'm off, thanks for your work.

@sberyozkin
Copy link
Member

sberyozkin commented Jun 13, 2023

Hey @phillip-kruger @michalvavrik

As far as Keycloak specific UI is concerned, moving Keycloak Admin into its own tab has given a lot of extra space, feels like all the information is much more accessible now. The focus issue Michal mentioned is noticeable but I guess it makes sense to open an Admin view in its own tab.

There is a minor problem though with non Keycloak providers. For example, here is what I get with Keycloak:

Screenshot from 2023-06-13 12-06-38.

And here what I get with Google (you can reproduce with quarkus.oidc.provider=google and set quarkus.oidc.application-type=hybrid and quarkus.oidc.client-id=any and ignore the secret for now - you won't be able to login but should be enough to reproduce):

Screenshot from 2023-06-13 12-21-20

Note, no Google Provider tab, I thought it was not even available, but then I forgot to restart the browser after checking Keycloak, and I got:

Screenshot from 2023-06-13 12-20-00

and here Google Provider tab exists, so this tab may not always be present.

I wonder, if we should keep a single Provider message, either as the text on the right or as a tab ? Having it as a tab looks nice, the Provider text on the left looks a bit detached, unless we can make it look more presentable somehow

@sberyozkin
Copy link
Member

This is how it looked like with the old DevUI:

Screenshot from 2023-06-13 12-38-47

Having OpenId Connect -> Keycloak on the left, instead of the Keycloak Provider text, would likely complement better the new Keycloak Provider tab:
Screenshot from 2023-06-13 12-06-38

@phillip-kruger phillip-kruger force-pushed the dev-ui-allow-external-link-in-submenu branch from be69577 to d6e56ae Compare June 13, 2023 12:01
@sberyozkin
Copy link
Member

Hey @phillip-kruger I've noticed you have pushed more updates, I haven't noticed what has changed, let me know your time tomorrow please if you'd like me and Michal to check something else. As I said, overall, moving the admin link to a submenu does give that feeling that much more space is available which is great. Only the initial login page might need to be tweaked a bit to have KC and non-KC providers have similar login pages (minus KC specific realm and client properties)

@quarkus-bot

This comment has been minimized.

@phillip-kruger phillip-kruger force-pushed the dev-ui-allow-external-link-in-submenu branch from 6175a97 to 4e5a52a Compare June 14, 2023 03:19
@phillip-kruger
Copy link
Member Author

@sberyozkin @michalvavrik I fixed the tab selection issue that Michal noted. W.r.t the rest, it's possibly something Michal can follow up with in more PRs ? I think if you are happy we should get this in and build on top of it ? Happy with that ?

@quarkus-bot

This comment has been minimized.

@michalvavrik
Copy link
Member

@phillip-kruger To be absolutely honest, I'm happy with your PR but there are HTML elements I'd adjust (I think when I tried either web app or client credentials service test button was huge as my screen as very very wide). I agree that I'll follow with PR and Sergey can provide his opinions on my design choices :-)

@sberyozkin
Copy link
Member

Sounds good, thanks @phillip-kruger @michalvavrik

@sberyozkin
Copy link
Member

@michalvavrik When you'll find time for following up with another PR, please also check some of my comments above, we can sync on them later on, cheers

@phillip-kruger
Copy link
Member Author

@michalvavrik yeah sure, agree

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 14, 2023

Failing Jobs - Building 4e5a52a

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Build Failures Logs Raw logs
✔️ Gradle Tests - JDK 11 Windows

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.QuarkusDevDependencyDevModeTest.main line 14 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@phillip-kruger phillip-kruger merged commit 212ef5a into quarkusio:main Jun 15, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants