Skip to content

Commit

Permalink
Merge pull request #1214 from SpiNNakerManchester/fix_password_reset
Browse files Browse the repository at this point in the history
Fix password reset
  • Loading branch information
rowleya authored Jan 28, 2025
2 parents ecc96b3 + 5d206ab commit f6a47f9
Show file tree
Hide file tree
Showing 10 changed files with 320 additions and 32 deletions.
10 changes: 10 additions & 0 deletions SpiNNaker-allocserv/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,11 @@ limitations under the License.
<artifactId>spring-boot-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-test-autoconfigure</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>uk.ac.manchester.spinnaker</groupId>
<artifactId>SpiNNaker-comms</artifactId>
Expand Down Expand Up @@ -365,6 +370,11 @@ limitations under the License.
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-core</artifactId>
</dependency>
<dependency>
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-security</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;

import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.Response.Status;

Expand Down Expand Up @@ -292,13 +294,22 @@ private ModelAndView errors(DataAccessException exception) {
}

@ExceptionHandler(BindException.class)
ModelAndView validationError(BindException result) {
ModelAndView validationError(BindException result, HttpServletRequest req) {
log.debug("Binding problem on request {}", req.getRequestURI());
if (req.getMethod() == "POST") {
try {
log.debug("Body: {}", req.getReader().lines().collect(
Collectors.joining()));
} catch (IOException e) {
log.debug("Failed to read request body", e);
}
}
if (result.hasGlobalErrors()) {
// I don't believe this is really reachable code
log.debug("binding problem", result);
log.debug("global binding problem", result);
return errors(errorMessage(result.getGlobalError()));
} else if (result.hasFieldErrors()) {
log.debug("binding problem", result);
log.debug("field binding problem", result);
return errors(errorMessage(result.getFieldError()));
} else {
// This should definitely be unreachable
Expand Down Expand Up @@ -416,7 +427,7 @@ public ModelAndView getUserCreationForm(ModelMap ignored) {
public ModelAndView createUser(UserRecord user, ModelMap model,
RedirectAttributes attrs) {
user.initCreationDefaults();
var realUser = userManager.createUser(user)
var realUser = userManager.createUser(user, this::showGroupInfoUrl)
.orElseThrow(() -> new AdminException(
"user creation failed (duplicate username?)"));
int id = realUser.getUserId();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,12 @@ public Map<String, URI> listUsers(UriInfo ui) {
@Override
public Response createUser(UserRecord providedUser, UriInfo ui) {
log.warn("CALLED createUser({})", providedUser.getUserName());
var ub = ui.getBaseUriBuilder().path(DESCRIBE_USER);
providedUser.initCreationDefaults();
var realUser = userManager.createUser(providedUser)
var realUser = userManager.createUser(providedUser,
m -> ub.build(m.getUserId()))
.orElseThrow(() -> new RequestFailedException(NOT_MODIFIED,
"user already exists"));
var ub = ui.getBaseUriBuilder().path(DESCRIBE_USER);
int id = realUser.getUserId();
return created(ub.build(id)).type(APPLICATION_JSON)
.entity(realUser.sanitise()).build();
Expand All @@ -210,10 +211,10 @@ public UserRecord updateUser(int id, UserRecord providedUser, UriInfo ui,
log.warn("CALLED updateUser({})", providedUser.getUserName());
var adminUser = security.getUserPrincipal().getName();
providedUser.setUserId(null);
var ub = ui.getBaseUriBuilder().path(DESCRIBE_GROUP);
var ub = ui.getBaseUriBuilder().path(DESCRIBE_USER);
return userManager
.updateUser(id, providedUser, adminUser,
m -> ub.build(m.getGroupId()))
m -> ub.build(m.getUserId()))
.orElseThrow(AdminImpl::noUser).sanitise();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,18 +108,6 @@ public void close() {
super.close();
}

/**
* Get a user.
*
* @param id
* User ID
* @return Database row, if user exists.
* @see SQLQueries#GET_USER_DETAILS
*/
Optional<UserRecord> getUser(int id) {
return getUserDetails.call1(UserControl::sketchUser, id);
}

Function<UserRecord, UserRecord>
populateMemberships(Function<MemberRecord, URI> urlGen) {
if (isNull(urlGen)) {
Expand Down Expand Up @@ -365,23 +353,31 @@ private static UserRecord sketchUser(Row row) {
*
* @param user
* The description of the user to create.
* @param urlGen
* How to construct the URL for a group membership in the
* response. If {@code null}, the memberships will be omitted.
* @return A description of the created user, or {@link Optional#empty()} if
* the user exists already.
*/
public Optional<UserRecord> createUser(UserRecord user) {
public Optional<UserRecord> createUser(UserRecord user,
Function<MemberRecord, URI> urlGen) {
// This is a slow operation; don't hold a database transaction
var encPass = passServices.encodePassword(user.getPassword());
try (var sql = new CreateSQL()) {
return sql.transaction(() -> createUser(user, encPass, sql));
return sql.transaction(
() -> createUser(user, encPass, urlGen, sql));
}
}

private Optional<UserRecord> createUser(UserRecord user, String encPass,
CreateSQL sql) {
return sql
.createUser(user.getUserName(), encPass, user.getTrustLevel(),
!user.getEnabled(), user.getOpenIdSubject())
.flatMap(sql::getUser);
Function<MemberRecord, URI> urlGen, CreateSQL sql) {
var result = sql.createUser(user.getUserName(), encPass,
user.getTrustLevel(), !user.getEnabled(),
user.getOpenIdSubject());
if (result.isEmpty()) {
return Optional.empty();
}
return getUser(result.get(), urlGen);
}

/**
Expand Down Expand Up @@ -517,7 +513,7 @@ private Optional<UserRecord> updateUser(int id, UserRecord user,
}
}

return sql.getUser(id).map(sql.populateMemberships(urlGen));
return getUser(id, urlGen);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,6 @@ public void setOpenIdSubject(String subject) {
}

/** @return Whether this is an internal user. */
@JsonIgnore
public boolean isInternal() {
return isInternal;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
https://www.apache.org/licenses/LICENSE-2.0
https://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
Expand All @@ -34,6 +34,7 @@ limitations under the License.
</c:choose>

<form:form method="POST" modelAttribute="user">
<form:hidden path="internal"/>
<form:label path="userName">User Name: </form:label>
<form:input path="userName" type="text"/>
<form:select path="trustLevel">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ private static void makeMachine(Connection c, int width, int height,
}
}

private static void makeUser(Connection c) {
protected static void makeUser(Connection c) {
try (var u = c.update(INSERT_USER)) {
u.call(USER, USER_NAME, BASIC, true);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/*
* Copyright (c) 2025 The University of Manchester
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package uk.ac.manchester.spinnaker.alloc.admin;

import static uk.ac.manchester.spinnaker.alloc.admin.AdminController.BASE_PATH;
import static uk.ac.manchester.spinnaker.alloc.admin.AdminController.CREATE_USER_PATH;
import static uk.ac.manchester.spinnaker.alloc.admin.AdminController.USERS_PATH;
import static org.junit.Assert.assertTrue;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.redirectedUrlPattern;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.forwardedUrl;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;

import java.net.URI;
import java.util.Base64;

import javax.ws.rs.core.MediaType;

import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.junit.jupiter.web.SpringJUnitWebConfig;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;

import uk.ac.manchester.spinnaker.alloc.TestSupport;
import uk.ac.manchester.spinnaker.alloc.model.UserRecord;
import uk.ac.manchester.spinnaker.alloc.security.TrustLevel;

@AutoConfigureMockMvc
@SpringBootTest
@SpringJUnitWebConfig(TestSupport.Config.class)
@ActiveProfiles("unittest")
public class AdminControllerTest extends TestSupport {

@Autowired
private MockMvc mvc;

@Autowired
private UserControl userControl;

@Test
public void testCreateUser() throws Exception {
doTransactionalTest(() -> {
// Create an admin
UserRecord admin = new UserRecord();
admin.setUserName("admin");
admin.setEnabled(true);
admin.setHasPassword(true);
admin.setTrustLevel(TrustLevel.ADMIN);
admin.setPassword("admin");

var adminRecord = userControl.createUser(admin,
m -> URI.create(""));
assertTrue(adminRecord.isPresent());

String userPass = Base64.getEncoder().encodeToString(
"admin:admin".getBytes());
try {
var result = mvc.perform(
MockMvcRequestBuilders
.post(BASE_PATH + CREATE_USER_PATH)
.with(csrf())
.header("Authorization", "Basic " + userPass)
.param("internal", "true")
.param("userName", "test")
.param("trustLevel", "USER")
.param("password", "test")
.param("enabled", "true")
.param("hasPassword", "true")
.accept(MediaType.APPLICATION_JSON));
result.andExpect(
redirectedUrlPattern(BASE_PATH + USERS_PATH + "/*"));
} catch (Exception e) {
throw new RuntimeException(e);
}
});
}

@Test
public void updateUserPassword() throws Exception {
doTransactionalTest(() -> {
// Create an admin
UserRecord admin = new UserRecord();
admin.setUserName("admin");
admin.setEnabled(true);
admin.setHasPassword(true);
admin.setTrustLevel(TrustLevel.ADMIN);
admin.setPassword("admin");

var adminRecord = userControl.createUser(admin,
m -> URI.create(""));
assertTrue(adminRecord.isPresent());
log.info("Admin created: {}", adminRecord.get());

String userPass = Base64.getEncoder().encodeToString(
"admin:admin".getBytes());

// Create a user
UserRecord test = new UserRecord();
test.setUserName("test");
test.setEnabled(true);
test.setHasPassword(true);
test.setTrustLevel(TrustLevel.USER);
test.setPassword("test");

var testRecord = userControl.createUser(test,
m -> URI.create(""));
assertTrue(testRecord.isPresent());
UserRecord testUser = testRecord.get();

try {
var result = mvc.perform(
MockMvcRequestBuilders
.post(BASE_PATH + USERS_PATH + "/" + testUser.getUserId())
.with(csrf())
.header("Authorization", "Basic " + userPass)
.param("internal", "true")
.param("userName", "test")
.param("trustLevel", "USER")
.param("password", "testchange")
.accept(MediaType.APPLICATION_JSON));

// Note: not sure how to get this dynamically!
result.andExpect(
forwardedUrl("/WEB-INF/views/admin/userdetails.jsp"));
} catch (Exception e) {
throw new RuntimeException(e);
}
});
}


}
Loading

0 comments on commit f6a47f9

Please sign in to comment.