Skip to content

Commit f6a47f9

Browse files
rowleyarowleya
authored
Merge pull request #1214 from SpiNNakerManchester/fix_password_reset
Fix password reset
2 parents ecc96b3 + 5d206ab commit f6a47f9

File tree

10 files changed

+320
-32
lines changed

10 files changed

+320
-32
lines changed

‎SpiNNaker-allocserv/pom.xml

+10
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,11 @@ limitations under the License.
335335
<artifactId>spring-boot-test</artifactId>
336336
<scope>test</scope>
337337
</dependency>
338+
<dependency>
339+
<groupId>org.springframework.boot</groupId>
340+
<artifactId>spring-boot-test-autoconfigure</artifactId>
341+
<scope>test</scope>
342+
</dependency>
338343
<dependency>
339344
<groupId>uk.ac.manchester.spinnaker</groupId>
340345
<artifactId>SpiNNaker-comms</artifactId>
@@ -365,6 +370,11 @@ limitations under the License.
365370
<groupId>org.springframework.security</groupId>
366371
<artifactId>spring-security-core</artifactId>
367372
</dependency>
373+
<dependency>
374+
<groupId>org.springframework.security</groupId>
375+
<artifactId>spring-security-test</artifactId>
376+
<scope>test</scope>
377+
</dependency>
368378
<dependency>
369379
<groupId>org.springframework.boot</groupId>
370380
<artifactId>spring-boot-starter-security</artifactId>

‎SpiNNaker-allocserv/src/main/java/uk/ac/manchester/spinnaker/alloc/admin/AdminControllerImpl.java

+15-4
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,9 @@
8686
import java.util.List;
8787
import java.util.Map;
8888
import java.util.Optional;
89+
import java.util.stream.Collectors;
8990

91+
import javax.servlet.http.HttpServletRequest;
9092
import javax.ws.rs.WebApplicationException;
9193
import javax.ws.rs.core.Response.Status;
9294

@@ -292,13 +294,22 @@ private ModelAndView errors(DataAccessException exception) {
292294
}
293295

294296
@ExceptionHandler(BindException.class)
295-
ModelAndView validationError(BindException result) {
297+
ModelAndView validationError(BindException result, HttpServletRequest req) {
298+
log.debug("Binding problem on request {}", req.getRequestURI());
299+
if (req.getMethod() == "POST") {
300+
try {
301+
log.debug("Body: {}", req.getReader().lines().collect(
302+
Collectors.joining()));
303+
} catch (IOException e) {
304+
log.debug("Failed to read request body", e);
305+
}
306+
}
296307
if (result.hasGlobalErrors()) {
297308
// I don't believe this is really reachable code
298-
log.debug("binding problem", result);
309+
log.debug("global binding problem", result);
299310
return errors(errorMessage(result.getGlobalError()));
300311
} else if (result.hasFieldErrors()) {
301-
log.debug("binding problem", result);
312+
log.debug("field binding problem", result);
302313
return errors(errorMessage(result.getFieldError()));
303314
} else {
304315
// This should definitely be unreachable
@@ -416,7 +427,7 @@ public ModelAndView getUserCreationForm(ModelMap ignored) {
416427
public ModelAndView createUser(UserRecord user, ModelMap model,
417428
RedirectAttributes attrs) {
418429
user.initCreationDefaults();
419-
var realUser = userManager.createUser(user)
430+
var realUser = userManager.createUser(user, this::showGroupInfoUrl)
420431
.orElseThrow(() -> new AdminException(
421432
"user creation failed (duplicate username?)"));
422433
int id = realUser.getUserId();

‎SpiNNaker-allocserv/src/main/java/uk/ac/manchester/spinnaker/alloc/admin/AdminImpl.java

+5-4
Original file line numberDiff line numberDiff line change
@@ -186,11 +186,12 @@ public Map<String, URI> listUsers(UriInfo ui) {
186186
@Override
187187
public Response createUser(UserRecord providedUser, UriInfo ui) {
188188
log.warn("CALLED createUser({})", providedUser.getUserName());
189+
var ub = ui.getBaseUriBuilder().path(DESCRIBE_USER);
189190
providedUser.initCreationDefaults();
190-
var realUser = userManager.createUser(providedUser)
191+
var realUser = userManager.createUser(providedUser,
192+
m -> ub.build(m.getUserId()))
191193
.orElseThrow(() -> new RequestFailedException(NOT_MODIFIED,
192194
"user already exists"));
193-
var ub = ui.getBaseUriBuilder().path(DESCRIBE_USER);
194195
int id = realUser.getUserId();
195196
return created(ub.build(id)).type(APPLICATION_JSON)
196197
.entity(realUser.sanitise()).build();
@@ -210,10 +211,10 @@ public UserRecord updateUser(int id, UserRecord providedUser, UriInfo ui,
210211
log.warn("CALLED updateUser({})", providedUser.getUserName());
211212
var adminUser = security.getUserPrincipal().getName();
212213
providedUser.setUserId(null);
213-
var ub = ui.getBaseUriBuilder().path(DESCRIBE_GROUP);
214+
var ub = ui.getBaseUriBuilder().path(DESCRIBE_USER);
214215
return userManager
215216
.updateUser(id, providedUser, adminUser,
216-
m -> ub.build(m.getGroupId()))
217+
m -> ub.build(m.getUserId()))
217218
.orElseThrow(AdminImpl::noUser).sanitise();
218219
}
219220

‎SpiNNaker-allocserv/src/main/java/uk/ac/manchester/spinnaker/alloc/admin/UserControl.java

+16-20
Original file line numberDiff line numberDiff line change
@@ -108,18 +108,6 @@ public void close() {
108108
super.close();
109109
}
110110

111-
/**
112-
* Get a user.
113-
*
114-
* @param id
115-
* User ID
116-
* @return Database row, if user exists.
117-
* @see SQLQueries#GET_USER_DETAILS
118-
*/
119-
Optional<UserRecord> getUser(int id) {
120-
return getUserDetails.call1(UserControl::sketchUser, id);
121-
}
122-
123111
Function<UserRecord, UserRecord>
124112
populateMemberships(Function<MemberRecord, URI> urlGen) {
125113
if (isNull(urlGen)) {
@@ -365,23 +353,31 @@ private static UserRecord sketchUser(Row row) {
365353
*
366354
* @param user
367355
* The description of the user to create.
356+
* @param urlGen
357+
* How to construct the URL for a group membership in the
358+
* response. If {@code null}, the memberships will be omitted.
368359
* @return A description of the created user, or {@link Optional#empty()} if
369360
* the user exists already.
370361
*/
371-
public Optional<UserRecord> createUser(UserRecord user) {
362+
public Optional<UserRecord> createUser(UserRecord user,
363+
Function<MemberRecord, URI> urlGen) {
372364
// This is a slow operation; don't hold a database transaction
373365
var encPass = passServices.encodePassword(user.getPassword());
374366
try (var sql = new CreateSQL()) {
375-
return sql.transaction(() -> createUser(user, encPass, sql));
367+
return sql.transaction(
368+
() -> createUser(user, encPass, urlGen, sql));
376369
}
377370
}
378371

379372
private Optional<UserRecord> createUser(UserRecord user, String encPass,
380-
CreateSQL sql) {
381-
return sql
382-
.createUser(user.getUserName(), encPass, user.getTrustLevel(),
383-
!user.getEnabled(), user.getOpenIdSubject())
384-
.flatMap(sql::getUser);
373+
Function<MemberRecord, URI> urlGen, CreateSQL sql) {
374+
var result = sql.createUser(user.getUserName(), encPass,
375+
user.getTrustLevel(), !user.getEnabled(),
376+
user.getOpenIdSubject());
377+
if (result.isEmpty()) {
378+
return Optional.empty();
379+
}
380+
return getUser(result.get(), urlGen);
385381
}
386382

387383
/**
@@ -517,7 +513,7 @@ private Optional<UserRecord> updateUser(int id, UserRecord user,
517513
}
518514
}
519515

520-
return sql.getUser(id).map(sql.populateMemberships(urlGen));
516+
return getUser(id, urlGen);
521517
}
522518

523519
/**

‎SpiNNaker-allocserv/src/main/java/uk/ac/manchester/spinnaker/alloc/model/UserRecord.java

-1
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,6 @@ public void setOpenIdSubject(String subject) {
288288
}
289289

290290
/** @return Whether this is an internal user. */
291-
@JsonIgnore
292291
public boolean isInternal() {
293292
return isInternal;
294293
}

‎SpiNNaker-allocserv/src/main/webapp/WEB-INF/views/admin/userdetails.jsp

+2-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ Licensed under the Apache License, Version 2.0 (the "License");
1010
you may not use this file except in compliance with the License.
1111
You may obtain a copy of the License at
1212
13-
https://www.apache.org/licenses/LICENSE-2.0
13+
https://www.apache.org/licenses/LICENSE-2.0
1414
1515
Unless required by applicable law or agreed to in writing, software
1616
distributed under the License is distributed on an "AS IS" BASIS,
@@ -34,6 +34,7 @@ limitations under the License.
3434
</c:choose>
3535

3636
<form:form method="POST" modelAttribute="user">
37+
<form:hidden path="internal"/>
3738
<form:label path="userName">User Name: </form:label>
3839
<form:input path="userName" type="text"/>
3940
<form:select path="trustLevel">

‎SpiNNaker-allocserv/src/test/java/uk/ac/manchester/spinnaker/alloc/TestSupport.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ private static void makeMachine(Connection c, int width, int height,
178178
}
179179
}
180180

181-
private static void makeUser(Connection c) {
181+
protected static void makeUser(Connection c) {
182182
try (var u = c.update(INSERT_USER)) {
183183
u.call(USER, USER_NAME, BASIC, true);
184184
}

‎SpiNNaker-allocserv/src/test/java/uk/ac/manchester/spinnaker/alloc/admin/AdminControllerTest.java

+148
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
/*
2+
* Copyright (c) 2025 The University of Manchester
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package uk.ac.manchester.spinnaker.alloc.admin;
17+
18+
import static uk.ac.manchester.spinnaker.alloc.admin.AdminController.BASE_PATH;
19+
import static uk.ac.manchester.spinnaker.alloc.admin.AdminController.CREATE_USER_PATH;
20+
import static uk.ac.manchester.spinnaker.alloc.admin.AdminController.USERS_PATH;
21+
import static org.junit.Assert.assertTrue;
22+
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.redirectedUrlPattern;
23+
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.forwardedUrl;
24+
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
25+
26+
import java.net.URI;
27+
import java.util.Base64;
28+
29+
import javax.ws.rs.core.MediaType;
30+
31+
import org.junit.jupiter.api.Test;
32+
import org.springframework.beans.factory.annotation.Autowired;
33+
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
34+
import org.springframework.boot.test.context.SpringBootTest;
35+
import org.springframework.test.context.ActiveProfiles;
36+
import org.springframework.test.context.junit.jupiter.web.SpringJUnitWebConfig;
37+
import org.springframework.test.web.servlet.MockMvc;
38+
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;
39+
40+
import uk.ac.manchester.spinnaker.alloc.TestSupport;
41+
import uk.ac.manchester.spinnaker.alloc.model.UserRecord;
42+
import uk.ac.manchester.spinnaker.alloc.security.TrustLevel;
43+
44+
@AutoConfigureMockMvc
45+
@SpringBootTest
46+
@SpringJUnitWebConfig(TestSupport.Config.class)
47+
@ActiveProfiles("unittest")
48+
public class AdminControllerTest extends TestSupport {
49+
50+
@Autowired
51+
private MockMvc mvc;
52+
53+
@Autowired
54+
private UserControl userControl;
55+
56+
@Test
57+
public void testCreateUser() throws Exception {
58+
doTransactionalTest(() -> {
59+
// Create an admin
60+
UserRecord admin = new UserRecord();
61+
admin.setUserName("admin");
62+
admin.setEnabled(true);
63+
admin.setHasPassword(true);
64+
admin.setTrustLevel(TrustLevel.ADMIN);
65+
admin.setPassword("admin");
66+
67+
var adminRecord = userControl.createUser(admin,
68+
m -> URI.create(""));
69+
assertTrue(adminRecord.isPresent());
70+
71+
String userPass = Base64.getEncoder().encodeToString(
72+
"admin:admin".getBytes());
73+
try {
74+
var result = mvc.perform(
75+
MockMvcRequestBuilders
76+
.post(BASE_PATH + CREATE_USER_PATH)
77+
.with(csrf())
78+
.header("Authorization", "Basic " + userPass)
79+
.param("internal", "true")
80+
.param("userName", "test")
81+
.param("trustLevel", "USER")
82+
.param("password", "test")
83+
.param("enabled", "true")
84+
.param("hasPassword", "true")
85+
.accept(MediaType.APPLICATION_JSON));
86+
result.andExpect(
87+
redirectedUrlPattern(BASE_PATH + USERS_PATH + "/*"));
88+
} catch (Exception e) {
89+
throw new RuntimeException(e);
90+
}
91+
});
92+
}
93+
94+
@Test
95+
public void updateUserPassword() throws Exception {
96+
doTransactionalTest(() -> {
97+
// Create an admin
98+
UserRecord admin = new UserRecord();
99+
admin.setUserName("admin");
100+
admin.setEnabled(true);
101+
admin.setHasPassword(true);
102+
admin.setTrustLevel(TrustLevel.ADMIN);
103+
admin.setPassword("admin");
104+
105+
var adminRecord = userControl.createUser(admin,
106+
m -> URI.create(""));
107+
assertTrue(adminRecord.isPresent());
108+
log.info("Admin created: {}", adminRecord.get());
109+
110+
String userPass = Base64.getEncoder().encodeToString(
111+
"admin:admin".getBytes());
112+
113+
// Create a user
114+
UserRecord test = new UserRecord();
115+
test.setUserName("test");
116+
test.setEnabled(true);
117+
test.setHasPassword(true);
118+
test.setTrustLevel(TrustLevel.USER);
119+
test.setPassword("test");
120+
121+
var testRecord = userControl.createUser(test,
122+
m -> URI.create(""));
123+
assertTrue(testRecord.isPresent());
124+
UserRecord testUser = testRecord.get();
125+
126+
try {
127+
var result = mvc.perform(
128+
MockMvcRequestBuilders
129+
.post(BASE_PATH + USERS_PATH + "/" + testUser.getUserId())
130+
.with(csrf())
131+
.header("Authorization", "Basic " + userPass)
132+
.param("internal", "true")
133+
.param("userName", "test")
134+
.param("trustLevel", "USER")
135+
.param("password", "testchange")
136+
.accept(MediaType.APPLICATION_JSON));
137+
138+
// Note: not sure how to get this dynamically!
139+
result.andExpect(
140+
forwardedUrl("/WEB-INF/views/admin/userdetails.jsp"));
141+
} catch (Exception e) {
142+
throw new RuntimeException(e);
143+
}
144+
});
145+
}
146+
147+
148+
}

0 commit comments

Comments
 (0)