-
Notifications
You must be signed in to change notification settings - Fork 712
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
extensions: auth-sso-saml: Add option to get username from attribute #989
base: main
Are you sure you want to change the base?
Conversation
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); | ||
|
||
} | ||
|
There was a problem hiding this comment.
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 @Inject
ing 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.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
The auth-sso-saml plugin always uses NameID as username.
Some SAML providers like simplesamlphp use NameID format "urn:oasis:names:tc:SAML:2.0:nameid-format:transient", this is a temporary ID associated with the user.
I added the option "saml-user-attribute" which allows to get the username from one of the attributes.
For example if simplesamlphp uses Active Directory LDAP as backend you can add one of this lines to guacamole.properties:
saml-user-attribute: mail
saml-user-attribute: sAMAccountName
saml-user-attribute: userPrincipalName
If saml-user-attribute is not set or empty the NameID wil be used.