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.

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

Jump to bottom

extensions: auth-sso-saml: Add option to get username from attribute #989

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

extensions: auth-sso-saml: Add option to get username from attribute #989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 26 additions & 0 deletions ...auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,18 @@ public class ConfigurationService {

};

/**
* The property that defines what attribute the SAML provider will return
* that contains login name for the authenticated user.
*/
private static final StringGuacamoleProperty SAML_USER_ATTRIBUTE =
new StringGuacamoleProperty() {

@Override
public String getName() { return "saml-user-attribute"; }

};

/**
* The maximum amount of time to allow for an in-progress SAML
* authentication attempt to be completed, in minutes. A user that takes
Expand Down Expand Up @@ -340,6 +352,20 @@ public String getGroupAttribute() throws GuacamoleException {
return environment.getProperty(SAML_GROUP_ATTRIBUTE, "groups");
}

/**
* Return the name of the attribute that will be supplied by the identity
* provider that contains the username.
*
* @return
* The name of the attribute that contains the username.
*
* @throws GuacamoleException
* If guacamole.properties cannot be parsed.
*/
public String getUserAttribute() throws GuacamoleException {
return environment.getProperty(SAML_USER_ATTRIBUTE, null);
}

/**
* Returns the maximum amount of time to allow for an in-progress SAML
* authentication attempt to be completed, in minutes. A user that takes
Expand Down
19 changes: 18 additions & 1 deletion ...uth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/user/SAMLAuthenticatedUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,23 @@ private Set<String> getGroups(AssertedIdentity identity)

}

private String getUser(AssertedIdentity identity)
throws GuacamoleException {

String samlUserAttribute = confService.getUserAttribute();
List<String> samlUser = null;

if (samlUserAttribute == null || samlUserAttribute.isEmpty())
return identity.getUsername();

samlUser = identity.getAttributes().get(samlUserAttribute);
if (samlUser == null || samlUser.isEmpty())
return identity.getUsername();

return samlUser.get(0);

}

Comment on lines +107 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, here, with a getUser() method, what if the AssertedIdentity object gets initialized with what we expect to be the username from the outset? The AssertedIdentity class pulls the NameId value out of the SAML response and assigns it to the username variable - I'm thinking a better approach would be to somehow let AssertedIdentity know what attribute should be used for the username. This may require @Injecting the configuration service into that class, but would make sure that the username actually contains what we expect it to contain - and then there wouldn't be a need to adjust calls to anything else.

I don't know of the NameId value needs to be kept for any other reason, but, if so, a separate variable could be added to AssertedIdentity for that.

This would also allow you, in the configuration service, to specify NameId as the default and not have to return null, check for null, etc.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, we could place the username assignment here:
https://github.com/scpcom/guacamole-client/blob/guacamole-auth-sso-saml-user-attribute/extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/acs/AssertedIdentity.java#L93

But since "NameId" is not an attribute it may not be a good idea to set this as default in the configuration (response.getNameId() is not the same as response.getAttributes().get("NameId")).
Instead of checking for null with still would ned to check for "NameId".
May be we could use "" as default, so we only need to check for isEmpty()

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay, I see - that makes sense. I would think that 1) we'd stick with null rather than empty ("") for that, but that we'd still want to update AssertedIdentity to capture the username from the specified attribute, falling back to NameId if the username is not specified.

Copy link
Author

Choose a reason for hiding this comment

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

I did not get the configuration service to work in AssertedIdentity.
This is what I added:

import com.google.inject.Inject;
import org.apache.guacamole.auth.saml.conf.ConfigurationService;

public class AssertedIdentity {
    /**
     * Service for retrieving SAML configuration information.
     */
    @Inject
    private ConfigurationService confService;

    // .....
}

In the line that calls "confService.getUserAttribute();" I get "Unexpected error in REST endpoint." followed by "java.lang.NullPointerException: null"

/**
* Initializes this AuthenticatedUser using the given
* {@link AssertedIdentity} and credentials.
Expand All @@ -121,7 +138,7 @@ private Set<String> getGroups(AssertedIdentity identity)
*/
public void init(AssertedIdentity identity, Credentials credentials)
throws GuacamoleException {
super.init(identity.getUsername(), credentials, getGroups(identity), getTokens(identity));
super.init(getUser(identity), credentials, getGroups(identity), getTokens(identity));
}

}