Skip to content

Commit

Permalink
SSO login bug fix (#2492)
Browse files Browse the repository at this point in the history 
* SSO login bug fix

* 🤖 Auto-update API types for TypeScript usage

* Increment users list after adding users

Signed-off-by: Muralidhar Basani <[email protected]>

* From review

---------

Signed-off-by: Muralidhar Basani <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Aindriú Lavelle <[email protected]>
  • Loading branch information
@muralibasani @github-actions @aindriu-aiven
3 people authored Jun 26, 2024
1 parent 25090b6 commit d073e3e
Show file tree
Hide file tree
Showing 10 changed files with 291 additions and 91 deletions.
28 changes: 14 additions & 14 deletions coral/types/api.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ export type components = {
errCode?: string;
message: string;
debugMessage?: string;
data?: Record<string, never>;
data?: unknown;
/** Format: date-time */
timestamp?: string;
};
Expand Down Expand Up @@ -629,7 +629,7 @@ export type components = {
sourceEnv?: string;
selectedTeam?: string;
typeOfSync?: string;
topicDetails?: Record<string, never>[];
topicDetails?: unknown[];
topicSearchFilter?: string;
};
SyncConnectorUpdates: {
Expand Down Expand Up @@ -2230,25 +2230,25 @@ export type operations = {
/** @description OK */
200: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Multi Status */
207: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Bad Request */
405: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Internal Server Error */
500: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
};
Expand All @@ -2267,25 +2267,25 @@ export type operations = {
/** @description OK */
200: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Multi Status */
207: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Bad Request */
405: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Internal Server Error */
500: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
};
Expand All @@ -2304,25 +2304,25 @@ export type operations = {
/** @description OK */
200: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Multi Status */
207: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Bad Request */
405: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
/** @description Internal Server Error */
500: {
content: {
"application/json": components["schemas"]["ApiResponse"][];
"application/json": unknown;
};
};
};
Expand Down
25 changes: 17 additions & 8 deletions core/src/main/java/io/aiven/klaw/helpers/db/rdbms/InsertDataJdbc.java
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package io.aiven.klaw.helpers.db.rdbms;

import static org.springframework.beans.BeanUtils.copyProperties;

import io.aiven.klaw.dao.*;
import io.aiven.klaw.model.enums.ApiResultStatus;
import io.aiven.klaw.model.enums.EntityType;
import io.aiven.klaw.model.enums.NewUserStatus;
import io.aiven.klaw.model.enums.RequestStatus;
import io.aiven.klaw.repository.*;
import java.sql.Timestamp;
Expand Down Expand Up @@ -380,13 +381,21 @@ public String insertIntoRegisterUsers(RegisterUserInfo userInfo) {
Optional<UserInfo> userNameExists = userInfoRepo.findById(userInfo.getUsername());
if (userNameExists.isPresent()) return "Failure. User already exists";

// STAGING status comes from AD users
RegisterUserInfo userRegistration =
registerInfoRepo.findFirstByUsernameAndStatusIn(
userInfo.getUsername(),
List.of(NewUserStatus.PENDING.value, NewUserStatus.STAGING.value));
if (userRegistration != null) {
return "Failure. Registration already exists";
Optional<RegisterUserInfo> optionalUserRegistration =
registerInfoRepo.findByUsername(userInfo.getUsername());
if (optionalUserRegistration.isPresent()) {
if ("APPROVED".equals(optionalUserRegistration.get().getStatus())) {
// do nothing -- user is deleted
} else if (!"STAGING".equals(optionalUserRegistration.get().getStatus())
&& !"PENDING".equals(optionalUserRegistration.get().getStatus())) {
return "Failure. Registration already exists";
} else {
int id = optionalUserRegistration.get().getId();
copyProperties(userInfo, optionalUserRegistration.get());
optionalUserRegistration.get().setId(id);
registerInfoRepo.save(optionalUserRegistration.get());
return ApiResultStatus.SUCCESS.value;
}
}

registerInfoRepo.save(userInfo);
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/java/io/aiven/klaw/helpers/db/rdbms/UpdateDataJdbc.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,10 @@ public void updateNewUserRequest(String username, String approver, boolean isApp
if (isApprove) {
status = NewUserStatus.APPROVED.value;
} else {
status = NewUserStatus.DECLINED.value;
// In case if user registration is declined, delete the record from db, so user can try to
// register again with any new data.
registerInfoRepo.deleteById("" + registerUser.getId());
return;
}
if (registerUser != null) {
if (NewUserStatus.PENDING.value.equals(registerUser.getStatus())) {
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/io/aiven/klaw/service/UsersTeamsControllerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,8 @@ public ApiResponse addNewUser(UserInfoModel newUser, boolean isExternal) throws

if (isExternal) {

if ("".equals(newUser.getUserPassword())) {
if ("".equals(newUser.getUserPassword())
|| ACTIVE_DIRECTORY.value.equals(authenticationType)) {
mailService.sendMail(
newUser.getUsername(),
newUser.getUserPassword(),
Expand Down
13 changes: 13 additions & 0 deletions core/src/test/java/io/aiven/klaw/MockMethods.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import io.aiven.klaw.model.requests.EnvModel;
import io.aiven.klaw.model.requests.KwClustersModel;
import io.aiven.klaw.model.requests.KwRolesPermissionsModel;
import io.aiven.klaw.model.requests.RegisterUserInfoModel;
import io.aiven.klaw.model.requests.TeamModel;
import io.aiven.klaw.model.requests.UserInfoModel;
import io.aiven.klaw.model.response.EnvParams;
Expand Down Expand Up @@ -53,6 +54,18 @@ public UserInfoModel getUserInfoModel(String username, String role, String team)
return userInfoModel;
}

public RegisterUserInfoModel getRegisterUserInfoModel(String username, String role) {
RegisterUserInfoModel userInfoModel = new RegisterUserInfoModel();
userInfoModel.setUsername(username);
userInfoModel.setPwd("testpwd");
userInfoModel.setRole(role);
userInfoModel.setTeamId(1001);
userInfoModel.setFullname("New User");
userInfoModel.setMailid("[email protected]");

return userInfoModel;
}

public UserInfoModel getUserInfoModelSwitchTeams(
String username, String role, int teamId, int switchTeamSize) {
UserInfoModel userInfoModel = new UserInfoModel();
Expand Down
128 changes: 121 additions & 7 deletions core/src/test/java/io/aiven/klaw/SsoActiveDirectoryAuthenticationIT.java
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,28 @@
package io.aiven.klaw;

import static io.aiven.klaw.UsersTeamsControllerIT.OBJECT_MAPPER;
import static io.aiven.klaw.UsersTeamsControllerIT.superAdmin;
import static io.aiven.klaw.helpers.KwConstants.STAGINGTEAM;
import static io.aiven.klaw.model.enums.NewUserStatus.PENDING;
import static org.assertj.core.api.Assertions.assertThat;
import static org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames.ID_TOKEN;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.oidcLogin;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.aiven.klaw.model.ApiResponse;
import io.aiven.klaw.model.requests.RegisterUserInfoModel;
import io.aiven.klaw.model.response.RegisterUserInfoModelResponse;
import io.aiven.klaw.model.response.UserInfoModelResponse;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.MethodOrderer;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -42,6 +55,13 @@ public class SsoActiveDirectoryAuthenticationIT {

@Autowired private MockMvc mvc;

private static MockMethods mockMethods;

@BeforeAll
public static void setup() {
mockMethods = new MockMethods();
}

// Login with Oidc profile with success
@Test
@Order(1)
Expand All @@ -53,8 +73,9 @@ public void invokeRootPageWithOidcLoginSuccess() {
.with(
oidcLogin()
.oidcUser(
getOidcUser())) // oidc login with valid preferredName/claims and
// authorities
getOidcUser(
superAdmin))) // oidc login with valid preferredName/claims
// and authorities
.contentType(MediaType.APPLICATION_JSON)
.accept(MediaType.APPLICATION_JSON))
.andReturn()
Expand All @@ -77,9 +98,9 @@ public void invokeRootPageWithBasicLoginFailure() {
mvc.perform(
MockMvcRequestBuilders.get("/")
.with(
user("superadmin")
.password("superAdminPwd")) // Invalid login for AD context
// authentication
user(superAdmin)
.password(
"superAdminPwd")) // Invalid login for AD context authentication
.contentType(MediaType.APPLICATION_JSON)
.accept(MediaType.APPLICATION_JSON))
.andReturn()
Expand All @@ -95,11 +116,104 @@ public void invokeRootPageWithBasicLoginFailure() {
;
}

private OidcUser getOidcUser() {
/*
1. Login with valid AD user, but user doesn't exist in klaw
2. A record is created in kwregisterusers table in STAGING state and User is routed to register page to submit all details and signup.
3. After user registers, record in kwregisterusers table has an updated status of PENDING
4. superadmin now approves and the user should exist in kwusers
*/
@Test
@Order(3)
public void invokeRootPageWithOidcLoginFailure() {
String nonExistingUserInKlaw = "testuser";
try {
// From browser, this triggers a user to be created in staging users table (kwregisterusers),
// if successful login in SSO but user doesn't exist in klaw
mvc.perform(
MockMvcRequestBuilders.get("/")
.with(
oidcLogin()
.oidcUser(
getOidcUser(
nonExistingUserInKlaw))) // oidc login with non existing user
.contentType(MediaType.APPLICATION_JSON)
.accept(MediaType.APPLICATION_JSON))
.andReturn()
.getResponse();
RegisterUserInfoModel userInfoModel =
mockMethods.getRegisterUserInfoModel(nonExistingUserInKlaw, "USER");
String jsonReq = OBJECT_MAPPER.writer().writeValueAsString(userInfoModel);

// Allow the user to signup
MockHttpServletResponse response2 =
mvc.perform(
MockMvcRequestBuilders.post("/registerUser")
.with(oidcLogin().oidcUser(getOidcUser(nonExistingUserInKlaw)))
.content(jsonReq)
.contentType(MediaType.APPLICATION_JSON)
.accept(MediaType.APPLICATION_JSON))
.andReturn()
.getResponse();
ApiResponse objectResponse =
new ObjectMapper().readValue(response2.getContentAsString(), ApiResponse.class);
assertThat(objectResponse.isSuccess()).isTrue();

// Allow the superadmin to fetch requests to approve
MockHttpServletResponse response3 =
mvc.perform(
MockMvcRequestBuilders.get("/getNewUserRequests")
.with(oidcLogin().oidcUser(getOidcUser(superAdmin)))
.contentType(MediaType.APPLICATION_JSON)
.accept(MediaType.APPLICATION_JSON))
.andReturn()
.getResponse();

List<RegisterUserInfoModelResponse> userInfoModelActualList =
new ObjectMapper().readValue(response3.getContentAsString(), new TypeReference<>() {});

assertThat(userInfoModelActualList.get(0).getUsername()).isEqualTo(nonExistingUserInKlaw);
assertThat(userInfoModelActualList.get(0).getStatus()).isEqualTo(PENDING.value);

// Allow the superadmin to approve the user
MockHttpServletResponse response4 =
mvc.perform(
MockMvcRequestBuilders.post("/execNewUserRequestApprove")
.with(oidcLogin().oidcUser(getOidcUser(superAdmin)))
.param("username", nonExistingUserInKlaw)
.contentType(MediaType.APPLICATION_JSON)
.accept(MediaType.APPLICATION_JSON))
.andReturn()
.getResponse();
ApiResponse objectResponse1 =
new ObjectMapper().readValue(response4.getContentAsString(), ApiResponse.class);
assertThat(objectResponse1.isSuccess()).isTrue();

// Fetch and see if user is now created
String userDetailsResponse =
mvc.perform(
MockMvcRequestBuilders.get("/getUserDetails")
.with(oidcLogin().oidcUser(getOidcUser(superAdmin)))
.param("userId", nonExistingUserInKlaw)
.contentType(MediaType.APPLICATION_JSON)
.accept(MediaType.APPLICATION_JSON))
.andExpect(status().isOk())
.andReturn()
.getResponse()
.getContentAsString();
UserInfoModelResponse userInfoModelActual =
new ObjectMapper().readValue(userDetailsResponse, new TypeReference<>() {});
assertThat(userInfoModelActual.getTeam()).isEqualTo(STAGINGTEAM);

} catch (Exception e) {
throw new RuntimeException(e);
}
}

private OidcUser getOidcUser(String username) {
Map<String, Object> claims = new HashMap<>();
claims.put("groups", "ROLE_USER");
claims.put("sub", 123);
claims.put("preferred_username", "superadmin"); // existing user with default installation
claims.put("preferred_username", username); // existing user with default installation
OidcIdToken idToken =
new OidcIdToken(ID_TOKEN, Instant.now(), Instant.now().plusSeconds(60), claims);
Collection<GrantedAuthority> authorities = getAuthorities();
Expand Down
Loading

0 comments on commit d073e3e

Please sign in to comment.