Skip to content

Minimal implementation of logout and user avatar #41

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.

Sign up for GitHub

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

Closed
peholmst wants to merge 2 commits into from
Closed

Minimal implementation of logout and user avatar #41

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eae288b
Minimal implementation of logout and user avatar
peholmst May 9, 2025
28ef0e7
Merge branch 'v24.8' into real-user-menu
peholmst May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 37 additions & 7 deletions walking-skeleton/src/main/java/com/example/application/base/ui/view/MainLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,23 @@
import com.vaadin.flow.router.Layout;
import com.vaadin.flow.server.menu.MenuConfiguration;
import com.vaadin.flow.server.menu.MenuEntry;
import com.vaadin.flow.spring.security.AuthenticationContext;
import jakarta.annotation.security.PermitAll;
import org.jspecify.annotations.Nullable;
import org.springframework.beans.factory.ObjectProvider;

import java.util.Optional;

import static com.vaadin.flow.theme.lumo.LumoUtility.*;

@Layout
@PermitAll // When security is enabled, allow all authenticated users
public final class MainLayout extends AppLayout {

MainLayout() {
private final @Nullable AuthenticationContext authenticationContext;

MainLayout(ObjectProvider<AuthenticationContext> authenticationContext) {
this.authenticationContext = authenticationContext.getIfAvailable();
Copy link
Member Author

@peholmst peholmst May 9, 2025

Choose a reason for hiding this comment

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

If the application has security configured, then AuthenticationContext is always available. Most real applications should have this. The CC dependency also has this configured. However, if that dependency is removed without manually configuring security, the authenticationContext becomes null. The current implementation takes this into account, but it feels silly:

  • If the application is secured, AuthenticationContext is always available and all checks and exception cases are unecessary.
  • If the application is not secured, having a user menu makes no sense at all and should be removed.

What action should require code changes from the developer: adding/using CC identity management, or removing/not using CC identity management?

Copy link

@jojule jojule May 12, 2025

Choose a reason for hiding this comment

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

If the application is not secured, having a user menu makes no sense at all and should be removed.

Can we hide the menu in that case?

Copy link
Member Author

@peholmst peholmst May 12, 2025

Choose a reason for hiding this comment

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

We can add code for it, but then we're also introducing some extra complexity (although it is quite isolated and has negligible impact on performance). If the application will be secured, having a check for something that is always true is not needed. If the application won't be secured, the entire code block should be deleted.

setPrimarySection(Section.DRAWER);
addToDrawer(createHeader(), new Scroller(createSideNav()), createUserMenu());
}
Expand Down Expand Up @@ -58,8 +66,18 @@ private SideNavItem createSideNavItem(MenuEntry menuEntry) {
}

private Component createUserMenu() {
// TODO Replace with real user information and actions
var avatar = new Avatar("John Smith");
if (authenticationContext == null || !authenticationContext.isAuthenticated()) {
Copy link
Member Author

@peholmst peholmst May 9, 2025

Choose a reason for hiding this comment

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

This code is also silly:

  • If the application is supposed to be secured, this is a critical error that should stop the application from starting.
  • If the application is secured, this layout already has @PermitAll which means this code will never get called unless the user is authenticated.
  • If the application is secured but also available to anonymous users, then this code should return a Login button, not an error badge (also, in that case, the annotation of this layout should be @AnonymousAllowed).

Since the code of the MainLayout is supposed to be usable as-is in production, the code have production quality. We should pick one happy path that the layout follows out-of-the-box, and then give the user instructions for what changes are needed if they stray from that path.

// This happens if the security of your application is not configured correctly.
// See https://vaadin.com/docs/latest/building-apps/security for details.

var badge = new Span("Security not configured");
badge.getElement().getThemeList().add("badge error");
badge.addClassNames(Margin.MEDIUM);
return badge;
}

var fullName = getUserFullName().orElseThrow();
var avatar = new Avatar(fullName);
avatar.addThemeVariants(AvatarVariant.LUMO_XSMALL);
avatar.addClassNames(Margin.Right.SMALL);
avatar.setColorIndex(5);
Expand All @@ -69,12 +87,24 @@ private Component createUserMenu() {
userMenu.addClassNames(Margin.MEDIUM);

var userMenuItem = userMenu.addItem(avatar);
userMenuItem.add("John Smith");
userMenuItem.getSubMenu().addItem("View Profile").setEnabled(false);
userMenuItem.getSubMenu().addItem("Manage Settings").setEnabled(false);
userMenuItem.getSubMenu().addItem("Logout").setEnabled(false);
userMenuItem.add(fullName);
userMenuItem.getSubMenu().addItem("View Profile").setEnabled(false); // TODO Implement or remove
userMenuItem.getSubMenu().addItem("Manage Settings").setEnabled(false); // TODO Implement or remove
userMenuItem.getSubMenu().addItem("Logout", event -> authenticationContext.logout());

return userMenu;
}

private Optional<String> getUserFullName() {
Copy link

@knoobie knoobie May 16, 2025

Choose a reason for hiding this comment

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

Copied @peholmst from the flow PR:

We've been trying to get by with a single skeleton, to keep things simple. Since security can be implemented in multiple ways, I see the following options:

  • We just pick one and add it to the skeleton. If the users want something else, they have to change it manually.
  • We make it possible to customize the skeleton before you download it.
  • We keep the skeleton simple, as it is to day, and then you have to either follow a guide and add the security yourself, or use Vaadin Copilot to do it for you.

If I understand you correctly, you want the skeleton to have a wow effect, be production-grade code and easy to use for beginners and good starting points for novice users. Going with this bullet points as requirement would result Imho in a requirement to always have security included. New people are shielded from the hard part at the beginning and novice user can either use it, remove it or customize it.

Now to the question, which security should be integrated. From my experience it's often easier for people to get started with the good old login/password form (even tho there are people like Matti that hate them) - its a good base for people. But on the other hand Oauth2/OIDC is such a wide used concept and requirement that it should not be forgotten. Additionally novice users would love to see Vaadin's approach on it.

Speaking from history, I have multiple application developed over the last years were we have both included. We have a common MyAppNameUser class which implements all spring related classes to be interchangeable in both worlds - in a login form and oidc login. This class allows us to swap between both login modules without requiring any code change down the line.

To allow easier development we are using a spring profile, for example named "local", "dev" or "demo" which replaced our default security chain that is configured to use Keycloak with a security chain that is using plain old form based login. This allows for rapid local development without external runtime dependencies.

This comes with the downside that people have to learn about the usage of spring profiles of they wanna start the app locally with a plain login form - but I would argue that are well spent 5min learning.

This exact version can then be 1:1 deployed on CC without the profile enabled and would use OIDC. If you don't like demo code - thanks to different security chains and classes, those or the whole dev package could be removed when creating the production build to ensure nothing leaks

Copy link
Member Author

@peholmst peholmst May 16, 2025

Choose a reason for hiding this comment

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

Thanks, that was a constructive comment. I'm currently trying to summarize everything for myself, to get a better picture of where we are, where we want to go, what options we have, and their pros and cons. I'll get back to this later.

Copy link

@knoobie knoobie May 16, 2025

Choose a reason for hiding this comment

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

If you have additional questions.. just hit me up or something wasn't understandable.. just saw some typos (typing on mobile and Apple's autocorrect is hard)

Copy link
Member Author

@peholmst peholmst May 20, 2025

Choose a reason for hiding this comment

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

I started playing with a double-configuration prototype, with separate dev and oidc configurations.

https://github.com/peholmst-sandbox/skeleton-security-sample/tree/main/src/main/java/com/example/skeletonsecurity/security

What do you think about this? I've added JavaDocs that explain what each class does. This is not the final version, only a discussion starter.

Copy link

@knoobie knoobie May 20, 2025

Choose a reason for hiding this comment

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

That looks similar to the way we do it :)

Some personal preferences / ideas for you to consider:

  • the application should start secured without a developer applying a profile - so IMHO oidc could be default
  • I would suggest to include placeholder / environment variables + default values for username/password in the properties to guide people into proper management
  • If this only targets 24.8+, I would recommend to use the new VaadinSecurityConfigurer instead of VaadinWebSecurity
  • JSpecify Nullable/NonNull > Optional IMHO (highly subjective)
  • I'm personally not a fan ob non-get methods.. (but I understand where your problem comes from) - we do something like this, where we use a common MyUser class as base which can be stored within the MyOidcUser so that we don't clash with Spring's API.. you can even go one step further and not implement UserDetails within MyUser, but also delegate his attributes so that your "ApplicationUser" has no dependency to Spring and could theoretically even be used in a Quarkus or JavaEE example.
public interface AuthenticationService {

  MyUser getUser();
}

@Slf4j
@Component
public class AuthenticationServiceImpl implements AuthenticationService {

  @Override
  public MyUser getUser() {
    return getUserFromAuthentication(
      SecurityContextHolder.getContext().getAuthentication()
    );
  }

  @Nullable
  public static MyUser getUserFromAuthentication(@Nullable Authentication authentication) {
    if (null == authentication || null == authentication.getPrincipal()) {
      return null;
    }

    var principal = authentication.getPrincipal();

    if (principal instanceof MyOidcUser) {
      return ((MyOidcUser) principal).getUser();
    }

    if (principal instanceof MyUser) {
      return (MyUser) principal;
    }

    if (principal instanceof String && "anonymousUser".equals(principal.toString())) {
      return null;
    }

    log.warn("Could not authenticate User from SecurityContext. Principal was not of type 'MyUser'. Instead it was: '{}' with value: '{}'",
      principal.getClass().getName(), principal);

    return null;
  }
}


public class MyUser implements UserDetails {
 // ....
}

public class MyOidcUser extends DefaultOidcUser {

  @Getter
  private final MyUser user;

  public MyOidcUser(MyUser user, OidcUser oidcUser, String nameAttributeKey) {
    super(oidcUser.getAuthorities(), oidcUser.getIdToken(), oidcUser.getUserInfo(), nameAttributeKey);
    this.user = user;
  }

  @Override
  public Collection<? extends GrantedAuthority> getAuthorities() {
    // delegate to the correct user so that Spring has access to the correct authorities
    return this.user.getAuthorities();
  }
}

if (authenticationContext == null) {
return Optional.empty();
}
// If you are using OIDC (e.g., via Vaadin Control Center), this returns the user's full name:
//return authenticationContext
Copy link
Member Author

@peholmst peholmst May 9, 2025

Choose a reason for hiding this comment

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

If you use OIDC or Control Center and comment this code out, it returns the full name of the user. Without the CC dependency, it fails to compile because OidcUser and StandardClaimAccessor would be missing from the classpath.

At the moment, the CC dependency is in the classpath, which means we could have the code commented out by default. However, that would mean that if the user removes CC, they would have to fix this code. Would that be OK?

Also, if the user was to use some other authentication mechanism than OIDC, this code would compile but result in a ClassCastException during runtime. Do we want to protect against that as well in some way?

Copy link
Member Author

@peholmst peholmst May 12, 2025

Choose a reason for hiding this comment

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

One way to solve this is to add methods to AuthenticationContext for retrieving common user information like the full name and e-mail address. However, this would require a way of plugging in various implementations for different security configurations. What do @mshabarov and @Artur- say about this?

Copy link
Member

@heruan heruan May 16, 2025

Choose a reason for hiding this comment

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

Although I also suggested adding a wrapper user object to Flow, seeing it now feels like it doesn't really simplify anything—if not for the only purpose of the skeleton—with the result that the skeleton magically works but the users are more confused about yet another user class.

AFAIK only OidcUser provides accessor for common user information, as defined by OpenID Connect standard claims. Let's just use that in the skeleton, and advocate for configuring security with OpenID Connect: doesn't even need Kubernetes or Control Center, to authenticate with Google it only needs Spring Security OAuth2 starter and two configuration properties:

spring.security.oauth2.client.registration.google.client-id: <client-id>
spring.security.oauth2.client.registration.google.client-secret: <secret>

and you get your OidcUser with all the info from your Google account.

Then if you want to deploy with Control Center, you don't need to change your code since it also works with OidcUser. If, on the other hand, you don't want to use OpenID Connect, then you need to change some code.

// .getAuthenticatedUser(OidcUser.class)
// .map(OidcUser::getUserInfo)
// .map(StandardClaimAccessor::getFullName);

return authenticationContext.getPrincipalName(); // TODO This is typically a username or ID, not the full name.
}
}