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

Allow creation of DefaultIdentityService without realmName. #6554

Closed
lachlan-roberts opened this issue Jul 28, 2021 · 2 comments
Closed

Allow creation of DefaultIdentityService without realmName. #6554

lachlan-roberts opened this issue Jul 28, 2021 · 2 comments
Assignees

Comments

@lachlan-roberts
Copy link
Contributor

SecurityHandler.doStart() has the following logic:

if (_identityService == null)
{
    if (_loginService != null)
        setIdentityService(_loginService.getIdentityService());

    if (_identityService == null)
        setIdentityService(findIdentityService());

    if (_identityService == null)
    {
        if (_realmName != null)
        {
            setIdentityService(new DefaultIdentityService());
            manage(_identityService);
        }
    }
    else
        unmanage(_identityService);
}

I am not sure we need the check for if (_realmName != null) to create the DefaultIdentityService.

This has caused previous issues with some people trying to configure jetty-openid from xml before. It would also make it easier to configure and use JASPI without this check.

@lachlan-roberts lachlan-roberts self-assigned this Jul 28, 2021
lachlan-roberts added a commit that referenced this issue Jul 29, 2021
…is provided

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@janbartel
Copy link
Contributor

Not checking the _realmName would make sense to me: we've already tried to find the LoginService, and if there was one, we would have asked it for it's IdentityService. As we didn't find one, it seems to be irrelevant to check the _realmName. In fact, it maybe should be an error for either of the 2 following conditions:

  1. we get this far with a realmName configured, but we didn't find any LoginServices
  2. we have a realmName but it didn't match any of of the LoginServices

@gregw thoughts?

lachlan-roberts added a commit that referenced this issue Aug 2, 2021
…is provided

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 2, 2021
…enticator for null AuthMethod

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

@janbartel This change will not currently work because the DefaultAuthenticatorFactory will return a BasicAuthenticator even if the AuthMethod is null. So if you create a DefaultIdentiyService it will always create a BasicAuthenticator.

I am not sure why we are doing this in DefaultAuthenticationFactory, doesn't seem like we should need to. There is a test towards the end of SecurityHandler.doStart which will throw if there is a realmName but no Authenticator.

I will open a PR with these changes as well to see what it looks like.

lachlan-roberts added a commit that referenced this issue Aug 25, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 27, 2021
Issue #6554 - create the DefaultIdentityService even if no realmName is provided
lachlan-roberts added a commit that referenced this issue Sep 2, 2021
…is provided

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Sep 3, 2021
…yService

Issue #6554 - create the DefaultIdentityService even if no realmName is provided
@sbordet sbordet added this to To do in Jetty 9.4.44 FROZEN via automation Sep 20, 2021
@sbordet sbordet moved this from To do to Done in Jetty 9.4.44 FROZEN Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants