mraible commented on code in PR #155:
URL: https://github.com/apache/roller/pull/155#discussion_r3772673174
##########
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:
Enforced server side now: the registration repository serves no providers
unless authentication.method is oidc or db-oidc (closing
/oauth2/authorization/* under db, ldap, and cma), RollerOidcUserService rejects
the flow outright, and RollerUserDetailsService refuses password lookups in
pure oidc mode. Covered by unit tests.
##########
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 updated conditionals existed one level up in the Bootstrap 5 UI
conversion, which is why the stacked browser tests passed all three auth modes.
They belong at this level, so Register.jsp, Profile.jsp, and UserEdit.jsp now
branch on OIDC/DB_OIDC here, drop the OpenID URL entry fields, and show the
provider-assigned identity read-only.
##########
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:
OAuth2 failures now redirect through their own
SimpleUrlAuthenticationFailureHandler with error=oidc, and Login.java selects
error.oidc.login for that marker, so provider failures are no longer diagnosed
as a wrong password.
##########
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:
A confidential client without a client-secret is now rejected at
configuration time with a log message pointing at the fix;
oidc.{id}.client-authentication-method=none supports public clients using PKCE.
##########
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:
Discovery results are now cached per provider in a ConcurrentHashMap with a
60s retry backoff for failures, so a healthy provider is never blocked by an
unreachable one and an IdP outage cannot serialize login-page traffic on
repeated discovery.
##########
.dockerignore:
##########
@@ -0,0 +1,4 @@
+.git
+docker/postgresql-data
Review Comment:
Fixed: .dockerignore now covers docker/postgresql-16-data, the directory the
updated compose file actually binds.
##########
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:
Good catch, treated as a security issue. Auto-provisioned accounts no longer
keep the users.firstUserAdmin bootstrap grant: it is revoked (and logged with
alternatives) unless the provider asserts an admin role claim or the new
users.oidc.firstUserAdmin property, default false, is enabled. Unit tests cover
the default, the opt-in, the admin claim, and the not-first-user cases.
##########
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:
The stale key references were in the same three JSPs and are fixed at this
level together with the auth method conditionals (see the other thread); the
pages no longer request the removed OpenID keys.
--
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]
