Skip to content

Don't allow to create new users when using external authentication systems like KeyCloak or OpenID #9281

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.

Sign up for GitHub

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

Open
josegar74 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Don't allow to create new users when using external authentication systems like KeyCloak or OpenID #9281

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f37b1be
Don't allow to create new users when using external authentication sy…
josegar74 May 18, 2026
b71b387
Don't allow to create new users when using external authentication sy…
josegar74 Jun 10, 2026
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
15 changes: 15 additions & 0 deletions core/src/main/java/org/fao/geonet/util/XslUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,21 @@ public static boolean isDisableLoginForm() {
return false;
}

/**
* Check if user profile create is enabled.
*/
public static boolean isUserProfileCreateEnabled() {
SecurityProviderConfiguration securityProviderConfiguration = SecurityProviderConfiguration.get();

if (securityProviderConfiguration != null) {
// No user creation allowed if providing a link or autologin
return !securityProviderConfiguration.getLoginType().equals(SecurityProviderConfiguration.LoginType.AUTOLOGIN.toString().toLowerCase())
&& !securityProviderConfiguration.getLoginType().equals(SecurityProviderConfiguration.LoginType.LINK.toString().toLowerCase());
}
// If we cannot find SecurityProviderConfiguration then default to true.
return true;
}

/**
* Check if security provider require login link
*/
Expand Down
4 changes: 4 additions & 0 deletions schemas/iso19115-3.2018/src/test/java/org/fao/geonet/util/XslUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ public static boolean isShowLoginAsLink() {
return false;
}

public static boolean isUserProfileCreateEnabled() {
return true;
}

public static boolean isUserProfileUpdateEnabled() {
return true;
}
Expand Down
84 changes: 39 additions & 45 deletions services/src/main/java/org/fao/geonet/api/users/UsersApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2001-2025 Food and Agriculture Organization of the
* Copyright (C) 2001-2026 Food and Agriculture Organization of the
* United Nations (FAO-UN), United Nations World Food Programme (WFP)
* and United Nations Environment Programme (UNEP)
*
Expand Down Expand Up @@ -45,16 +45,17 @@
import org.fao.geonet.domain.*;
import org.fao.geonet.domain.auditable.UserAuditable;
import org.fao.geonet.exceptions.UserNotFoundEx;
import org.fao.geonet.kernel.DataManager;
import org.fao.geonet.kernel.datamanager.IMetadataOperations;
import org.fao.geonet.kernel.datamanager.IMetadataStatus;
import org.fao.geonet.kernel.datamanager.IMetadataUtils;
import org.fao.geonet.kernel.datamanager.base.BaseMetadataStatus;
import org.fao.geonet.kernel.security.SecurityProviderConfiguration;
import org.fao.geonet.kernel.setting.SettingManager;
import org.fao.geonet.repository.*;
import org.fao.geonet.repository.specification.MetadataSpecs;
import org.fao.geonet.repository.specification.UserGroupSpecs;
import org.fao.geonet.repository.specification.UserSpecs;
import org.fao.geonet.util.PasswordUtil;
import org.fao.geonet.util.XslUtil;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.context.support.ResourceBundleMessageSource;
Expand Down Expand Up @@ -114,15 +115,9 @@ public class UsersApi {
@Autowired
UserGroupRepository userGroupRepository;

@Autowired
BaseMetadataStatus baseMetadataStatus;

@Autowired
UserSavedSelectionRepository userSavedSelectionRepository;

@Autowired
DataManager dataManager;

@Autowired
LanguageUtils languageUtils;

Expand All @@ -139,7 +134,13 @@ public class UsersApi {
@Autowired
UserAuditableService userAuditableService;

private BufferedImage pixel;
@Autowired
private IMetadataOperations metadataOperations;

@Autowired
private IMetadataStatus metadataStatus;

private final BufferedImage pixel;

public UsersApi() {
pixel = new BufferedImage(1, 1, BufferedImage.TYPE_INT_ARGB);
Expand All @@ -148,8 +149,7 @@ public UsersApi() {


@io.swagger.v3.oas.annotations.Operation(
summary = "Get users",
description = "")
summary = "Get users")
@RequestMapping(
produces = MediaType.APPLICATION_JSON_VALUE,
method = RequestMethod.GET)
Expand All @@ -159,16 +159,13 @@ public UsersApi() {
public List<User> getUsers(
@Parameter(hidden = true)
HttpSession httpSession
) throws Exception {
) {
UserSession session = ApiUtils.getUserSession(httpSession);
Profile profile = session.getProfile();

if (Profile.Administrator.equals(profile)) {
// Get all users
return userRepository.findAll(SortUtils.createSort(User_.name));
} else if (!Profile.UserAdmin.equals(profile)) {
// Return only the current user
return userRepository.findAll(UserSpecs.hasUserId(session.getUserIdAsInt()));
} else if (Profile.UserAdmin.equals(profile)) {
// Return all the users belonging to a group where the current user is UserAdmin
int userId = session.getUserIdAsInt();
Expand All @@ -186,15 +183,15 @@ public List<User> getUsers(
// alToRemove.add(elRec);

return allUsers;
} else {
// Return only the current user
return userRepository.findAll(UserSpecs.hasUserId(session.getUserIdAsInt()));
}

return null;
}


@io.swagger.v3.oas.annotations.Operation(
summary = "Get user",
description = "")
summary = "Get user")
@RequestMapping(
value = "/{userIdentifier}",
produces = MediaType.APPLICATION_JSON_VALUE,
Expand All @@ -211,7 +208,7 @@ public User getUser(
@Parameter(hidden = true)
HttpSession httpSession

) throws Exception {
) {
UserSession session = ApiUtils.getUserSession(httpSession);
Profile myProfile = session.getProfile();
String myUserId = session.getUserId();
Expand Down Expand Up @@ -243,8 +240,7 @@ public User getUser(
}

@io.swagger.v3.oas.annotations.Operation(
summary = "Get user identicon",
description = "")
summary = "Get user identicon")
@RequestMapping(
value = "/{userIdentifier}.png",
produces = MediaType.IMAGE_PNG_VALUE,
Expand Down Expand Up @@ -321,15 +317,14 @@ public ResponseEntity<String> deleteUser(
Profile myProfile = session.getProfile();
String myUserId = session.getUserId();


if (myUserId == null || myUserId.equals(Integer.toString(userIdentifier))) {
throw new IllegalArgumentException(
"You cannot delete yourself from the user database");
}


if (myProfile == Profile.UserAdmin) {
final Integer iMyUserId = Integer.parseInt(myUserId);
final int iMyUserId = Integer.parseInt(myUserId);
final List<Integer> groupIdsSessionUser = userGroupRepository
.findGroupIds(where(hasUserId(iMyUserId)));

Expand All @@ -344,7 +339,7 @@ public ResponseEntity<String> deleteUser(
// Before processing DELETE check that the user is not referenced
// elsewhere in the GeoNetwork database - an exception is thrown if
// this is the case
if (dataManager.isUserMetadataOwner(userIdentifier)) {
if (metadataOperations.isUserMetadataOwner(userIdentifier)) {
IMetadataUtils metadataRepository = ApplicationContextHolder.get().getBean(IMetadataUtils.class);
final long numUserRecords = metadataRepository.count(MetadataSpecs.isOwnedByUser(userIdentifier));
throw new IllegalArgumentException(
Expand All @@ -353,10 +348,10 @@ public ResponseEntity<String> deleteUser(
numUserRecords));
}

if (dataManager.isUserMetadataStatus(userIdentifier)) {
if (metadataStatus.isUserMetadataStatus(userIdentifier)) {
Optional<User> nobody = userRepository.findById(0);
if (nobody.isPresent()) {
baseMetadataStatus.transferMetadataStatusOwnership(userIdentifier,
metadataStatus.transferMetadataStatusOwnership(userIdentifier,
nobody.get().getId());
} else {
throw new IllegalArgumentException(
Expand Down Expand Up @@ -386,15 +381,11 @@ public ResponseEntity<String> deleteUser(
}


return new ResponseEntity(HttpStatus.NO_CONTENT);
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
}

@io.swagger.v3.oas.annotations.Operation(
summary = "Check if a user property already exist",
description = ""
// authorizations = {
// @Authorization(value = "basicAuth")
// })
summary = "Check if a user property already exist"
)
@RequestMapping(
value = "/properties/{property}",
Expand Down Expand Up @@ -454,13 +445,17 @@ public ResponseEntity<String> createUser(
ServletRequest request,
@Parameter(hidden = true)
HttpSession httpSession
) throws Exception {
) {
Locale locale = languageUtils.parseAcceptLanguage(request.getLocales());
ResourceBundle messages = ResourceBundle.getBundle("org.fao.geonet.api.Messages", locale);

UserSession session = ApiUtils.getUserSession(httpSession);
Profile myProfile = session.getProfile();

if (!XslUtil.isUserProfileCreateEnabled()) {

@ianwallen ianwallen May 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the next condition there is check to skip if the user is an administrator. This allows administrators to pre-create the users before the user logs in. I suspect that there should be a check here as well to skip the check for administrators?

We used this logic during a migration exercise to be able to populate the metadata and the ownership via api. In this case the users never logged in so it was nice to be able to pre-create the users and then use the account for metadata ownership before the user ever logged in. In this case, only the email was specified in the user account as the email/username. The future OIDC login updated the user account with the rest of the information. Otherwise we would have had to do all this logic via SQL statements and it could have been more prone to errors.

@josegar74 josegar74 Jun 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ianwallen I have updated the code, but the original code you point:

core-geonetwork/services/src/main/java/org/fao/geonet/api/users/UsersApi.java

Lines 464 to 469 in c7be4e4

// Allow administrator to modify the user profile as they may need to manually pre-create users via api in certain cases (i.e. migration)
if (securityProviderConfiguration != null &&
!securityProviderConfiguration.isUserProfileUpdateEnabled() &&
!Profile.Administrator.equals(myProfile)) {
return new ResponseEntity<>(messages.getString("security_provider_unsupported_functionality"), HttpStatus.PRECONDITION_FAILED);
}

The comment is about updating a user profile, but the method is to create a new user, not to update a user, that seems confusing. Maybe the previous code should not have that check? Or do I miss something?

@ianwallen ianwallen Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isUserProfileUpdateEnabled means are we allowed to make changes to the profile. This implies creation and updates since creations are modifying the profile and so is updating. I'm not understanding why we would want to allow someone to create profiles but not update them? Or vice versa.

The goal for this flag was to disallow the modification of the user profile since the profile is managed by the OIDC.
This is why if the external authentication is updating the profile then we use the opposite for this flag.

core-geonetwork/core/src/main/java/org/fao/geonet/kernel/security/openidconnect/OIDCConfiguration.java

Line 226 in c7be4e4

return !updateProfile;

I guess if the external authentication system would only copy the user profile during account creation and then going forward it was only used to authentication then I guess disallowing create and allowing updates would be acceptable? But I don't think this is what you are trying to fix.

To be honest, I'm not understanding why we need a variable for creation and another one for updates? Since they are for the same purpose. And if the system is only allowing create but not update and a user is created with a minor bug (i.e. incorrect email) then update would not be allowed to fix the issue via the api? - Based you the description of this PR there is no information on what issue we are trying to address? In our environment, we use keycloak via openidconnect configuration and we cannot create/update users via the ui. We have used the api as an administrator (which we used during a migration to pre-create users.)

I have no objection with refactoring the variable if you think it would clarify the meaning.

return new ResponseEntity<>(messages.getString("security_provider_unsupported_functionality"), HttpStatus.PRECONDITION_FAILED);
}

// Allow administrator to modify the user profile as they may need to manually pre-create users via api in certain cases (i.e. migration)
if (securityProviderConfiguration != null &&
!securityProviderConfiguration.isUserProfileUpdateEnabled() &&
Expand Down Expand Up @@ -535,7 +530,7 @@ public ResponseEntity<String> createUser(
UserAuditable userAuditable = UserAuditable.build(user, userGroups);
userAuditableService.auditSave(userAuditable);

return new ResponseEntity(HttpStatus.NO_CONTENT);
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
}

@io.swagger.v3.oas.annotations.Operation(
Expand All @@ -562,7 +557,7 @@ public ResponseEntity<String> updateUser(
ServletRequest request,
@Parameter(hidden = true)
HttpSession httpSession
) throws Exception {
) {
Locale locale = languageUtils.parseAcceptLanguage(request.getLocales());

Profile profile = Profile.findProfileIgnoreCase(userDto.getProfile());
Expand Down Expand Up @@ -677,7 +672,7 @@ public ResponseEntity<String> updateUser(
UserAuditable userAuditable = UserAuditable.build(user, userGroups);
userAuditableService.auditSave(userAuditable);

return new ResponseEntity(HttpStatus.NO_CONTENT);
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
}

private boolean isUserAllowedToResetWithoutOldPassword(Profile myProfile) {
Expand Down Expand Up @@ -708,7 +703,7 @@ public ResponseEntity<String> resetUserPassword(
ServletRequest request,
@Parameter(hidden = true)
HttpSession httpSession
) throws Exception {
) {

Locale locale = languageUtils.parseAcceptLanguage(request.getLocales());
ResourceBundle messages = ResourceBundle.getBundle("org.fao.geonet.api.Messages", locale);
Expand Down Expand Up @@ -742,7 +737,7 @@ public ResponseEntity<String> resetUserPassword(

PasswordEncoder encoder = PasswordUtil.encoder(ApplicationContextHolder.get());

if (isUserAllowedToResetWithoutOldPassword(myProfile) == false
if (!isUserAllowedToResetWithoutOldPassword(myProfile)
&& (passwordResetDto.getPasswordOld() == null
|| !encoder.matches(
passwordResetDto.getPasswordOld(),
Expand All @@ -756,7 +751,7 @@ public ResponseEntity<String> resetUserPassword(
user.get().getSecurity().getSecurityNotifications().remove(UserSecurityNotification.UPDATE_HASH_REQUIRED);
userRepository.save(user.get());

return new ResponseEntity(HttpStatus.NO_CONTENT);
return new ResponseEntity<>(HttpStatus.NO_CONTENT);
}

@io.swagger.v3.oas.annotations.Operation(
Expand All @@ -779,7 +774,7 @@ public List<UserGroup> retrieveUserGroups(
ServletRequest request,
@Parameter(hidden = true)
HttpSession httpSession
) throws Exception {
) {
UserSession session = ApiUtils.getUserSession(httpSession);
Profile myProfile = session.getProfile();
String myUserId = session.getUserId();
Expand All @@ -801,7 +796,7 @@ public List<UserGroup> retrieveUserGroups(
// Return all groups for administrator.
// TODO: Check if a better option returning instead of UserGroup a customised GroupDTO
// containing all group properties and user profile
userGroups = new ArrayList<UserGroup>();
userGroups = new ArrayList<>();

List<Group> groups = groupRepository.findAll();

Expand Down Expand Up @@ -850,8 +845,7 @@ private List<Integer> getGroupIdsWhereUserIsUserAdmin(int userId) {
}


private void setUserGroups(final User user, List<GroupElem> userGroups, Locale locale)
throws Exception {
private void setUserGroups(final User user, List<GroupElem> userGroups, Locale locale) {

Collection<UserGroup> all = userGroupRepository.findAll(UserGroupSpecs
.hasUserId(user.getId()));
Expand Down
2 changes: 2 additions & 0 deletions web-ui/src/main/resources/catalog/js/CatController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,6 +1394,7 @@
current: null,
isDisableLoginForm: false,
isShowLoginAsLink: false,
isUserProfileCreateEnabled: true,
isUserProfileUpdateEnabled: true,
isUserGroupUpdateEnabled: true,
init: function (
Expand Down Expand Up @@ -1804,6 +1805,7 @@
$scope.isDebug = window.location.search.indexOf("debug") !== -1;
$scope.isDisableLoginForm = gnGlobalSettings.isDisableLoginForm;
$scope.isShowLoginAsLink = gnGlobalSettings.isShowLoginAsLink;
$scope.isUserProfileCreateEnabled = gnGlobalSettings.isUserProfileCreateEnabled;
$scope.isUserProfileUpdateEnabled = gnGlobalSettings.isUserProfileUpdateEnabled;
$scope.isUserGroupUpdateEnabled = gnGlobalSettings.isUserGroupUpdateEnabled;
$scope.isExternalViewerEnabled = gnExternalViewer.isEnabled();
Expand Down
3 changes: 1 addition & 2 deletions web-ui/src/main/resources/catalog/templates/admin/usergroup/users.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,12 @@
data-cache="users"
></span>
</div>

<button
type="button"
class="btn btn-primary btn-block"
id="gn-btn-user-add"
data-ng-click="addUser()"
data-ng-show="user.isUserAdminOrMore() && isUserProfileUpdateEnabled"
data-ng-show="user.isUserAdminOrMore() && isUserProfileCreateEnabled"
>
<i class="fa fa-plus"></i>
<span data-translate="">newUser</span>
Expand Down
1 change: 1 addition & 0 deletions web/src/main/webapp/xslt/base-layout-cssjs-loader.xsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@
function(gnGlobalSettings) {
gnGlobalSettings.isDisableLoginForm = <xsl:value-of select="$isDisableLoginForm"/>;
gnGlobalSettings.isShowLoginAsLink = <xsl:value-of select="$isShowLoginAsLink"/>;
gnGlobalSettings.isUserProfileCreateEnabled = <xsl:value-of select="$isUserProfileCreateEnabled"/>;
gnGlobalSettings.isUserProfileUpdateEnabled = <xsl:value-of select="$isUserProfileUpdateEnabled"/>;
gnGlobalSettings.isUserGroupUpdateEnabled = <xsl:value-of select="$isUserGroupUpdateEnabled"/>;
}]);
Expand Down
3 changes: 3 additions & 0 deletions web/src/main/webapp/xslt/common/base-variables.xsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@
<xsl:variable name="isShowLoginAsLink"
select="util:isShowLoginAsLink()"/>

<xsl:variable name="isUserProfileCreateEnabled"
select="util:isUserProfileCreateEnabled()"/>

<xsl:variable name="isUserProfileUpdateEnabled"
select="util:isUserProfileUpdateEnabled()"/>

Expand Down
Loading