Copilot commented on code in PR #155:
URL: https://github.com/apache/roller/pull/155#discussion_r3772295798
##########
app/src/main/webapp/WEB-INF/security.xml:
##########
@@ -49,6 +49,12 @@
<remember-me services-ref="rollerRememberMeServices"
key="715F2448-3176-11DD-ABC6-9CD955D89593"/>
+ <!-- OAuth2/OIDC login (active only when oidc.*.client-id properties
are configured) -->
+ <oauth2-login
client-registration-repository-ref="clientRegistrationRepository"
+ oidc-user-service-ref="rollerOidcUserService"
+ authentication-success-handler-ref="oidcSuccessHandler"
+ login-page="/roller-ui/login.rol"/>
Review Comment:
The security filter chain does not enforce `authentication.method`:
configuring any provider enables `/oauth2/authorization/{id}` even for `db`,
`ldap`, or `cma`, while the existing `<form-login>` remains usable by direct
POST in `oidc` mode. Hiding links/forms in the JSP is not an authentication
control, so these modes currently allow authentication methods they claim to
disable. Build or guard both filters according to `AuthMethod`.
##########
app/src/main/java/org/apache/roller/weblogger/config/AuthMethod.java:
##########
@@ -20,8 +20,8 @@
public enum AuthMethod {
ROLLERDB("db"),
LDAP("ldap"),
- OPENID("openid"),
- DB_OPENID("db-openid"),
+ OIDC("oidc"),
+ DB_OIDC("db-oidc"),
Review Comment:
The renamed enum values were not propagated to the other authentication
views. `Register.jsp:76-84,103-104,159-174`, `Profile.jsp:22,52,67`, and
`admin/UserEdit.jsp:39,71,77` still compare `OPENID`/`DB_OPENID`; under OIDC
modes their password/identity controls never render, and registration
JavaScript never enables submission. Update all three JSPs to use
`OIDC`/`DB_OIDC`.
##########
.dockerignore:
##########
@@ -0,0 +1,4 @@
+.git
+docker/postgresql-data
Review Comment:
This ignore path does not match the PostgreSQL bind directory used by
`docker-compose.yml` (`docker/postgresql-16-data`). After the demo has run,
`COPY . /project` sends the live database directory into every Docker build
context, which can be very large and may fail on database-owned files. Ignore
the actual directory name.
##########
app/src/main/java/org/apache/roller/weblogger/ui/core/security/RollerClientRegistrationRepository.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * 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
+ *
+ * http://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. For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+package org.apache.roller.weblogger.ui.core.security;
+
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.roller.weblogger.config.WebloggerConfig;
+import
org.springframework.security.oauth2.client.registration.ClientRegistration;
+import
org.springframework.security.oauth2.client.registration.ClientRegistrationRepository;
+import
org.springframework.security.oauth2.client.registration.ClientRegistrations;
+import org.springframework.security.oauth2.core.AuthorizationGrantType;
+
+/**
+ * Builds OAuth2/OIDC client registrations from Roller properties.
+ *
+ * <p>OIDC discovery is deferred until first access so the identity provider
+ * does not need to be reachable during application startup.
+ *
+ * <p>Properties follow the pattern:
+ * <pre>
+ * oidc.{registrationId}.client-id=...
+ * oidc.{registrationId}.client-secret=...
+ * oidc.{registrationId}.issuer-uri=...
+ * oidc.{registrationId}.client-name=... (optional, defaults to
registrationId)
+ * oidc.{registrationId}.scope=openid,profile,email (optional)
+ * </pre>
+ */
+public class RollerClientRegistrationRepository implements
ClientRegistrationRepository, Iterable<ClientRegistration> {
+
+ private static final Log log =
LogFactory.getLog(RollerClientRegistrationRepository.class);
+ private static final String PREFIX = "oidc.";
+
+ private volatile Map<String, ClientRegistration> registrations;
+
+ @Override
+ public ClientRegistration findByRegistrationId(String registrationId) {
+ return getRegistrations().get(registrationId);
+ }
+
+ @Override
+ public Iterator<ClientRegistration> iterator() {
+ return getRegistrations().values().iterator();
+ }
+
+ /**
+ * Discovery runs on first use and the result is cached, but only once
every
+ * configured provider resolved. A provider that was unreachable is retried
+ * on the next call rather than being cached as permanently broken.
+ */
+ public Map<String, ClientRegistration> getRegistrations() {
+ Map<String, ClientRegistration> cached = registrations;
+ if (cached != null) {
+ return cached;
+ }
+ synchronized (this) {
+ if (registrations != null) {
+ return registrations;
+ }
+ Map<String, ClientRegistration> built = buildRegistrations();
+ if (built.size() < configuredProviderIds().size()) {
+ return Collections.unmodifiableMap(built);
+ }
+ registrations = Collections.unmodifiableMap(built);
Review Comment:
A single unreachable or malformed provider prevents the map from ever being
cached. Because the public login page calls this method to render buttons,
every request serializes on this monitor and repeats network discovery for all
providers, including healthy ones, until the failed endpoint times out. Cache
provider results independently and retry failures with bounded backoff so an
IdP outage cannot stall login-page traffic or hammer discovery endpoints.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Login.java:
##########
@@ -61,28 +68,44 @@ public String getAuthMethod() {
return authMethod.name();
}
+ /**
+ * Providers to offer sign-in buttons for. Only registrations the
repository
+ * could actually resolve are listed, so the page never advertises a
provider
+ * whose discovery endpoint was unreachable.
+ */
+ public List<Map<String, String>> getOidcProviders() {
+ List<Map<String, String>> providers = new ArrayList<>();
+ RollerClientRegistrationRepository repository =
RollerContext.getClientRegistrationRepository();
+ if (repository == null) {
+ return providers;
+ }
+ for (ClientRegistration registration :
repository.getRegistrations().values()) {
+ Map<String, String> provider = new LinkedHashMap<>();
+ provider.put("id", registration.getRegistrationId());
+ provider.put("name", registration.getClientName());
+ providers.add(provider);
+ }
+ return providers;
+ }
+
@Override
public String execute() {
-
+
// set action error message if there was login error
if(getError() != null) {
- if (authMethod == AuthMethod.OPENID) {
- addError("error.unmatched.openid");
- } else {
- addError("error.password.mismatch");
- }
+ addError("error.password.mismatch");
Review Comment:
OIDC failures are redirected to this action with an error parameter, but
every failure is reported as a wrong username/password and the newly added
`error.oidc.login` message is never used. This hides provider, linking,
disabled-user, and provisioning failures behind an incorrect diagnosis. Give
OAuth/OIDC a distinct failure URL/parameter and select the OIDC message here;
`DB_OIDC` needs that distinction because either flow can fail.
##########
app/src/main/java/org/apache/roller/weblogger/ui/core/security/RollerClientRegistrationRepository.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * 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
+ *
+ * http://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. For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+package org.apache.roller.weblogger.ui.core.security;
+
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.roller.weblogger.config.WebloggerConfig;
+import
org.springframework.security.oauth2.client.registration.ClientRegistration;
+import
org.springframework.security.oauth2.client.registration.ClientRegistrationRepository;
+import
org.springframework.security.oauth2.client.registration.ClientRegistrations;
+import org.springframework.security.oauth2.core.AuthorizationGrantType;
+
+/**
+ * Builds OAuth2/OIDC client registrations from Roller properties.
+ *
+ * <p>OIDC discovery is deferred until first access so the identity provider
+ * does not need to be reachable during application startup.
+ *
+ * <p>Properties follow the pattern:
+ * <pre>
+ * oidc.{registrationId}.client-id=...
+ * oidc.{registrationId}.client-secret=...
+ * oidc.{registrationId}.issuer-uri=...
+ * oidc.{registrationId}.client-name=... (optional, defaults to
registrationId)
+ * oidc.{registrationId}.scope=openid,profile,email (optional)
+ * </pre>
+ */
+public class RollerClientRegistrationRepository implements
ClientRegistrationRepository, Iterable<ClientRegistration> {
+
+ private static final Log log =
LogFactory.getLog(RollerClientRegistrationRepository.class);
+ private static final String PREFIX = "oidc.";
+
+ private volatile Map<String, ClientRegistration> registrations;
+
+ @Override
+ public ClientRegistration findByRegistrationId(String registrationId) {
+ return getRegistrations().get(registrationId);
+ }
+
+ @Override
+ public Iterator<ClientRegistration> iterator() {
+ return getRegistrations().values().iterator();
+ }
+
+ /**
+ * Discovery runs on first use and the result is cached, but only once
every
+ * configured provider resolved. A provider that was unreachable is retried
+ * on the next call rather than being cached as permanently broken.
+ */
+ public Map<String, ClientRegistration> getRegistrations() {
+ Map<String, ClientRegistration> cached = registrations;
+ if (cached != null) {
+ return cached;
+ }
+ synchronized (this) {
+ if (registrations != null) {
+ return registrations;
+ }
+ Map<String, ClientRegistration> built = buildRegistrations();
+ if (built.size() < configuredProviderIds().size()) {
+ return Collections.unmodifiableMap(built);
+ }
+ registrations = Collections.unmodifiableMap(built);
+ if (!registrations.isEmpty()) {
+ log.info("Configured OIDC providers: " +
registrations.keySet());
+ }
+ return registrations;
+ }
+ }
+
+ /** Registration ids that have an {@code oidc.<id>.client-id} property
set. */
+ static Map<String, String> configuredProviderIds() {
+ Map<String, String> registrationIds = new LinkedHashMap<>();
+ Enumeration<Object> keys = WebloggerConfig.keys();
+ while (keys.hasMoreElements()) {
+ String key = (String) keys.nextElement();
+ if (key.startsWith(PREFIX) && key.endsWith(".client-id")) {
+ String id = key.substring(PREFIX.length(), key.length() -
".client-id".length());
+ String clientId = WebloggerConfig.getProperty(key);
+ if (clientId != null && !clientId.isBlank()) {
+ registrationIds.put(id, clientId);
+ }
+ }
+ }
+ return registrationIds;
+ }
+
+ private Map<String, ClientRegistration> buildRegistrations() {
+ Map<String, String> registrationIds = configuredProviderIds();
+
+ Map<String, ClientRegistration> result = new LinkedHashMap<>();
+ for (Map.Entry<String, String> entry : registrationIds.entrySet()) {
+ String id = entry.getKey();
+ String clientId = entry.getValue();
+ String clientSecret = WebloggerConfig.getProperty(PREFIX + id +
".client-secret");
+ String issuerUri = WebloggerConfig.getProperty(PREFIX + id +
".issuer-uri");
+ String clientName = WebloggerConfig.getProperty(PREFIX + id +
".client-name", id);
+ String scopeStr = WebloggerConfig.getProperty(PREFIX + id +
".scope", "openid,profile,email");
+
+ if (clientId == null || clientId.isBlank() || issuerUri == null ||
issuerUri.isBlank()) {
+ log.warn("Skipping OIDC registration '" + id + "': client-id
and issuer-uri are required");
+ continue;
+ }
Review Comment:
The documented required `client-secret` is not validated, so a
confidential-client registration can be built and advertised even though its
token exchange will fail without credentials. Either require the secret as
documented or add an explicit client-authentication-method setting for
supported public clients.
##########
app/src/main/java/org/apache/roller/weblogger/ui/core/security/RollerOidcUserService.java:
##########
@@ -0,0 +1,257 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * 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
+ *
+ * http://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. For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+package org.apache.roller.weblogger.ui.core.security;
+
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.TimeZone;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.roller.util.UUIDGenerator;
+import org.apache.roller.weblogger.business.UserManager;
+import org.apache.roller.weblogger.business.WebloggerFactory;
+import org.apache.roller.weblogger.config.WebloggerConfig;
+import org.apache.roller.weblogger.pojos.User;
+import org.springframework.security.core.GrantedAuthority;
+import org.springframework.security.core.authority.SimpleGrantedAuthority;
+import
org.springframework.security.oauth2.client.oidc.userinfo.OidcUserRequest;
+import
org.springframework.security.oauth2.client.oidc.userinfo.OidcUserService;
+import org.springframework.security.oauth2.client.userinfo.OAuth2UserService;
+import org.springframework.security.oauth2.core.OAuth2AuthenticationException;
+import org.springframework.security.oauth2.core.OAuth2Error;
+import org.springframework.security.oauth2.core.oidc.OidcIdToken;
+import org.springframework.security.oauth2.core.oidc.OidcUserInfo;
+import org.springframework.security.oauth2.core.oidc.user.DefaultOidcUser;
+import org.springframework.security.oauth2.core.oidc.user.OidcUser;
+
+/**
+ * Bridges OIDC-authenticated users to Roller's user store.
+ *
+ * <p>The OIDC subject (formatted as {@code issuer#sub}) is matched against the
+ * User.openIdUrl column. Users who authenticate for the first time are
+ * provisioned just-in-time from their OIDC claims. Either way the returned
+ * OidcUser carries the Roller roles as authorities, so authorization works on
+ * the very first request after login.
+ */
+public class RollerOidcUserService implements
OAuth2UserService<OidcUserRequest, OidcUser> {
+
+ private static final Log log =
LogFactory.getLog(RollerOidcUserService.class);
+ private final OidcUserService delegate = new OidcUserService();
+
+ @Override
+ public OidcUser loadUser(OidcUserRequest userRequest) throws
OAuth2AuthenticationException {
+ return resolveUser(delegate.loadUser(userRequest));
+ }
+
+ /**
+ * Resolves the Roller account behind an authenticated OIDC user and
returns
+ * a principal carrying that account's Roller roles as authorities.
+ */
+ OidcUser resolveUser(OidcUser oidcUser) throws
OAuth2AuthenticationException {
+ if (!WebloggerFactory.isBootstrapped()) {
+ throw new OAuth2AuthenticationException(new
OAuth2Error("roller_not_bootstrapped"),
+ "Roller is not bootstrapped; cannot resolve OIDC user");
+ }
+
+ String oidcSubject = toOidcSubject(oidcUser);
+
+ try {
+ UserManager umgr =
WebloggerFactory.getWeblogger().getUserManager();
+ User rollerUser = umgr.getUserByOpenIdUrl(oidcSubject);
+
+ if (rollerUser == null) {
+ rollerUser = linkExistingUser(umgr, oidcUser, oidcSubject);
+ }
+ if (rollerUser == null) {
+ rollerUser = provisionUser(umgr, oidcUser, oidcSubject);
+ }
+ if (!Boolean.TRUE.equals(rollerUser.getEnabled())) {
+ throw new OAuth2AuthenticationException(new
OAuth2Error("user_disabled"),
+ "Roller user is disabled: " +
rollerUser.getUserName());
+ }
+
+ List<GrantedAuthority> authorities = new ArrayList<>();
+ for (String role : umgr.getRoles(rollerUser)) {
+ authorities.add(new SimpleGrantedAuthority(role));
+ }
+ return new RollerOidcUser(authorities, oidcUser.getIdToken(),
oidcUser.getUserInfo(),
+ rollerUser.getUserName());
+
+ } catch (OAuth2AuthenticationException e) {
+ throw e;
+ } catch (Exception e) {
+ log.error("Error resolving Roller user for OIDC subject: " +
oidcSubject, e);
+ throw new OAuth2AuthenticationException(new
OAuth2Error("user_resolution_failed"),
+ "Could not resolve Roller user for OIDC subject: " +
oidcSubject, e);
+ }
+ }
+
+ /**
+ * Adopts a pre-existing Roller account whose username matches the one
+ * asserted by the provider, which is how database users carry over when a
+ * site turns on OIDC. Linking requires the provider to have verified an
+ * email address matching the account, so that control of an unverified
+ * address at the provider cannot be used to take over a Roller account.
+ *
+ * @return the linked user, or null if there is no account to adopt
+ */
+ private User linkExistingUser(UserManager umgr, OidcUser oidcUser, String
oidcSubject) throws Exception {
+ String username = usernameOf(oidcUser);
+ User existing = umgr.getUserByUserName(username);
+ if (existing == null) {
+ return null;
+ }
+
+ String email = oidcUser.getEmail();
+ boolean emailVerified =
Boolean.TRUE.equals(oidcUser.getEmailVerified())
+ && email != null &&
email.equalsIgnoreCase(existing.getEmailAddress());
+
+ if (!emailVerified) {
+ throw new OAuth2AuthenticationException(new
OAuth2Error("account_link_required"),
+ "A Roller account named '" + username + "' already exists
but is not linked to "
+ + oidcSubject + ". An administrator must set its
federated identity, or the"
+ + " provider must assert a verified email address
matching the account.");
+ }
+
+ existing.setOpenIdUrl(oidcSubject);
+ umgr.saveUser(existing);
+ WebloggerFactory.getWeblogger().flush();
+ log.info("Linked existing Roller user '" + username + "' to OIDC
subject " + oidcSubject);
+ return existing;
+ }
+
+ /**
+ * Creates a Roller account from the OIDC claims. The account is linked to
+ * the identity provider by subject, and gets a random password since it is
+ * never used for authentication.
+ */
+ private User provisionUser(UserManager umgr, OidcUser oidcUser, String
oidcSubject) throws Exception {
+ String username = usernameOf(oidcUser);
+
+ // the identity provider decides who may sign in, so provisioning is a
+ // static config choice like users.ldap.autoProvision.enabled, not tied
+ // to the runtime form-registration toggle
+ if
(!WebloggerConfig.getBooleanProperty("users.oidc.autoProvision.enabled")) {
+ throw new OAuth2AuthenticationException(new
OAuth2Error("auto_provision_disabled"),
+ "OIDC auto-provisioning is disabled; no Roller account
exists for " + oidcSubject);
+ }
+
+ User user = new User();
+ user.setId(UUIDGenerator.generateUUID());
+ user.setUserName(username);
+
+ String fullName = oidcUser.getFullName();
+ if (fullName == null || fullName.isBlank()) {
+ fullName = username;
+ }
+ user.setFullName(fullName);
+ user.setScreenName(username);
+
+ String email = oidcUser.getEmail();
+ if (email == null || email.isBlank()) {
+ throw new OAuth2AuthenticationException(new
OAuth2Error("missing_email"),
+ "OIDC provider did not supply an email address for " +
username);
+ }
+ user.setEmailAddress(email);
+
+ user.setOpenIdUrl(oidcSubject);
+ user.setPassword(UUIDGenerator.generateUUID());
+ user.setDateCreated(new Timestamp(System.currentTimeMillis()));
+ user.setLocale(Locale.getDefault().toString());
+ user.setTimeZone(TimeZone.getDefault().getID());
+ user.setEnabled(Boolean.TRUE);
+
+ // grants the "editor" role, and "admin" if this is the first user
+ umgr.addUser(user);
+
+ // flush before granting so the roles addUser() just created are
visible
+ // to grantRole()'s duplicate check, which queries the database
Review Comment:
`addUser()` grants `admin` to the first enabled user when the default
`users.firstUserAdmin=true` (`JPAUserManagerImpl.java:98-120`). Therefore the
first auto-provisioned OIDC user receives Roller admin even when `claimRoles`
does not contain `admin`; the `nonAdminUserDoesNotGetAdminRole` mock test
misses this side effect. With auto-provisioning also defaulting to true, an
ordinary IdP user can become site administrator simply by signing in first.
Gate the first-user grant on the OIDC admin claim, or explicitly define and
enforce a safe bootstrap policy.
##########
app/src/main/resources/ApplicationResources.properties:
##########
@@ -1436,21 +1436,19 @@ may disable your account if he/she cannot reach you via
email.
userRegister.heading.authentication=How will you be authenticated?
-userRegister.tip.openid.disabled=Enter a password to be used when you login \
+userRegister.tip.password.db=Enter a password to be used when you login \
and confirm that password by entering it a second time.
-userRegister.tip.openid.hybrid=You can choose to login via username/password
or \
-<a href=\"http://openid.net\";>OpenID</a>. If you choose the latter, leave \
+userRegister.tip.password.dbOidc=You can choose to login via username/password
or \
+your identity provider. If you authenticated via your identity provider, leave
\
Review Comment:
These renamed bundle keys still have callers under their old names. In
particular, `Register.jsp:73` requests `userRegister.tip.openid.disabled`, so
even the default `db` registration page now displays an unresolved resource
key; the OIDC branches and Profile/UserEdit similarly reference the removed
OpenID keys. Update those JSP references when renaming the messages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
