Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix handling of SAML optional attributes #92

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Fix handling of SAML optional attributes #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b36c23
Do not require SAML attributes marked as optional
markpatton Aug 26, 2024
a5c0c9a
Test parsing of SAML attributes
markpatton Aug 26, 2024
0c32338
Add more testing of required SAML attribute handling
markpatton Aug 26, 2024
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
14 changes: 5 additions & 9 deletions pass-core-main/src/main/java/org/eclipse/pass/main/security/PassAuthenticationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ private Authentication authenticate(Saml2AuthenticatedPrincipal principal)
});
}

User user = parse_user(principal.getAttributes());
User user = parseUser(principal.getAttributes());
create_or_update_pass_user(user);

return new PassAuthentication(user);
Expand Down Expand Up @@ -229,10 +229,10 @@ private User find_pass_user(PassClient pass_client, User user) throws IOExceptio
}

/**
* @param attributes
* @return User representing the information in the request.
* @param attributes of an authenticating user
* @return User representing the attributes
*/
private User parse_user(Map<String, List<Object>> attributes) {
User parseUser(Map<String, List<Object>> attributes) {
User user = new User();

String display_name = get(attributes, Attribute.DISPLAY_NAME, true);
Expand Down Expand Up @@ -293,13 +293,9 @@ private String get(Map<String, List<Object>> attributes, Attribute attr, boolean

List<Object> values = attributes.get(key);

if (values == null || values.size() == 0 && required) {
throw new BadCredentialsException("Missing attribute: " + attr + "[" + key + "]");
}

String value = null;

if (values.get(0) != null) {
if (values != null && values.size() > 0 && values.get(0) != null) {
value = values.get(0).toString().trim();
}

Expand Down
114 changes: 114 additions & 0 deletions ...-core-main/src/test/java/org/eclipse/pass/main/security/PassAuthenticationFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,28 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;

import org.eclipse.pass.main.SamlIntegrationTest;
import org.eclipse.pass.object.PassClient;
import org.eclipse.pass.object.model.User;
import org.eclipse.pass.object.model.UserRole;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.authentication.BadCredentialsException;

public class PassAuthenticationFilterTest extends SamlIntegrationTest {
@Autowired
private PassAuthenticationFilter passAuthFilter;

@Test
public void testLoggedInUser() throws IOException {
User submitter = doSamlLogin();
Expand Down Expand Up @@ -65,4 +74,109 @@ public void testLoginUpdatesUser() throws IOException {
assertNotEquals(updated, result);
assertEquals(submitter, result);
}

@Test
public void testParseUser() {
Map<String, List<Object>> attributes = new HashMap<>();

attributes.put("urn:oid:2.16.840.1.113730.3.1.241", List.of("Thomas L. Submitter"));
attributes.put("urn:oid:1.3.6.1.4.1.5923.1.1.1.9", List.of("FACULTY@johnshopkins.edu"));
attributes.put("urn:oid:0.9.2342.19200300.100.1.3", List.of("tom987654321@jhu.edu"));
attributes.put("urn:oid:1.3.6.1.4.1.5923.1.1.1.6", List.of("thomassubmitter987654321@johnshopkins.edu"));
attributes.put("urn:oid:2.5.4.42", List.of("Tom"));
attributes.put("urn:oid:2.5.4.4", List.of("Submitter"));
attributes.put("urn:oid:2.16.840.1.113730.3.1.3", List.of("987654321"));
attributes.put("urn:oid:1.3.6.1.4.1.5923.1.1.1.13", List.of("tls987654321@johnshopkins.edu"));

User expected = new User();
expected.setDisplayName("Thomas L. Submitter");
expected.setEmail("tom987654321@jhu.edu");
expected.setAffiliation(new HashSet<>(List.of("FACULTY@johnshopkins.edu", "johnshopkins.edu")));
expected.setFirstName("Tom");
expected.setLastName("Submitter");
expected.setLocatorIds(List.of("johnshopkins.edu:unique-id:tls987654321",
"johnshopkins.edu:eppn:thomassubmitter987654321",
"johnshopkins.edu:employeeid:987654321"));
expected.setUsername("thomassubmitter987654321@johnshopkins.edu");
expected.setRoles(List.of(UserRole.SUBMITTER));

User user = passAuthFilter.parseUser(attributes);

assertEquals(expected, user);
}

@Test
public void testParseUserMissingEmployeeIdAndAffiliation() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there are test for missing required attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. That is a good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test for that case.

Map<String, List<Object>> attributes = new HashMap<>();

attributes.put("urn:oid:2.16.840.1.113730.3.1.241", List.of("Thomas L. Submitter"));
attributes.put("urn:oid:0.9.2342.19200300.100.1.3", List.of("tom987654321@jhu.edu"));
attributes.put("urn:oid:1.3.6.1.4.1.5923.1.1.1.6", List.of("thomassubmitter987654321@johnshopkins.edu"));
attributes.put("urn:oid:2.5.4.42", List.of("Tom"));
attributes.put("urn:oid:2.5.4.4", List.of("Submitter"));
attributes.put("urn:oid:1.3.6.1.4.1.5923.1.1.1.13", List.of("tls987654321@johnshopkins.edu"));

User expected = new User();
expected.setDisplayName("Thomas L. Submitter");
expected.setEmail("tom987654321@jhu.edu");
expected.setAffiliation(new HashSet<>(List.of("johnshopkins.edu")));
expected.setFirstName("Tom");
expected.setLastName("Submitter");
expected.setLocatorIds(List.of("johnshopkins.edu:unique-id:tls987654321",
"johnshopkins.edu:eppn:thomassubmitter987654321"));
expected.setUsername("thomassubmitter987654321@johnshopkins.edu");
expected.setRoles(List.of(UserRole.SUBMITTER));

User user = passAuthFilter.parseUser(attributes);

assertEquals(expected, user);
}

@Test
public void testParseUserMissingRequiredAttributes() {
Map<String, List<Object>> attributes = new HashMap<>();

attributes.put("urn:oid:2.16.840.1.113730.3.1.241", List.of("Thomas L. Submitter"));
attributes.put("urn:oid:0.9.2342.19200300.100.1.3", List.of("tom987654321@jhu.edu"));
attributes.put("urn:oid:1.3.6.1.4.1.5923.1.1.1.6", List.of("thomassubmitter987654321@johnshopkins.edu"));
attributes.put("urn:oid:2.5.4.42", List.of("Tom"));
attributes.put("urn:oid:2.5.4.4", List.of("Submitter"));
attributes.put("urn:oid:1.3.6.1.4.1.5923.1.1.1.13", List.of("tls987654321@johnshopkins.edu"));

// Show that removing each of the required keys causes a failure
// and that various values are treated as the key not existing
for (String key: attributes.keySet()) {
HashMap<String, List<Object>> test = new HashMap<>(attributes);

test.remove(key);

assertThrows(BadCredentialsException.class, () -> {
passAuthFilter.parseUser(test);
});

test.put(key, null);

assertThrows(BadCredentialsException.class, () -> {
passAuthFilter.parseUser(test);
});

test.put(key, List.of());

assertThrows(BadCredentialsException.class, () -> {
passAuthFilter.parseUser(test);
});

test.put(key, List.of(""));

assertThrows(BadCredentialsException.class, () -> {
passAuthFilter.parseUser(test);
});

test.put(key, new ArrayList<>(1));

assertThrows(BadCredentialsException.class, () -> {
passAuthFilter.parseUser(test);
});
}
}
}
Loading