[ELY-2583] Make requestURI and Source-Address available from RealmSuccessfulAuthenticationEvent and RealmFailedAuthenticationEvent #1952

Skyllarr · 2023-09-11T09:31:44Z

https://issues.redhat.com/browse/ELY-2583

fjuma

Thanks @Skyllarr! I've added some feedback.

fjuma · 2023-09-12T19:56:49Z

auth/server/base/src/main/java/org/wildfly/security/auth/server/MechanismInformation.java

@@ -53,6 +53,10 @@ public interface MechanismInformation {
     */
    String getProtocol();

+    default String getRequestURI() {


MechanismInformation already provides getHost and getProtocol methods. Just wondering if we can make use of these existing methods instead of introducing a new getRequestURI method. (We'd need to check if the information provided by the host and protocol would be sufficient for the reporter.)

Note that MechanismInformation is public API.

So if we decide that a new method is needed, we should check with @OndrejKotek to see if adding a new method here would be ok for 1.15.x.

@fjuma We can make use of the existing getHost and getProtocol methods instead. The URI information would not have a full requested path that way, just the host and protocol.

However if we want to avoid adding a new method to the public API we might not have a better choice. So if @OndrejKotek thinks we should not introduce a new API method to the maintenance branch, we can check with the reporter about providing only the host+port information

This is not up to me to decide. Open a discussion on https://issues.redhat.com/browse/JBEAP-25337 please as it's up to the QE CP (Peter Mackay, Dan Cihak) and SET team to ack such changes.

fjuma · 2023-09-12T20:10:10Z

tests/base/src/test/java/org/wildfly/security/auth/server/TestCustomRealm.java

+        @Override
+        public void handleRealmEvent(RealmEvent event) {
+            if (event instanceof RealmSuccessfulAuthenticationEvent) {
+                assertEquals("10.12.14.16", ((RealmSuccessfulAuthenticationEvent) event).getAuthorizationIdentity().getRuntimeAttributes().get("Source-Address").get(0));


As a sanity check, would also be good to verify that it's possible to retrieve the principal in both the successful and failed cases.

@fjuma Added

…LY-2583] Make requestURI and Source-Address available from RealmSuccessfulAuthenticationEvent and RealmFailedAuthenticationEvent

Skyllarr · 2023-10-23T11:58:29Z

@fjuma This PR was updated to use a new Callback to avoid using MechanismInformation interface since that is meant to carry information about the mechanism. This also avoids introducing new method to public interface

fjuma · 2023-11-15T18:25:02Z

auth/server/base/src/main/java/org/wildfly/security/auth/server/RequestInformationCallback.java

+import java.net.URI;
+
+/**
+ * A {@link javax.security.auth.callback.Callback} to inform a server authentication context about current authenticaiton request.


s/authenticaiton/authentication

fjuma · 2023-11-15T18:25:55Z

auth/server/base/src/main/java/org/wildfly/security/auth/server/RequestInformationCallback.java

+     */
+    private final URI requestUri;
+
+    public  RequestInformationCallback(URI requestUri) {


Should add Javadoc for this.

fjuma · 2023-11-15T18:26:51Z

auth/server/base/src/main/java/org/wildfly/security/auth/server/RequestInformationCallback.java

+        this.requestUri = requestUri;
+    }
+
+    /**


Just minor but since this is public API, we should also add a description for the method.

fjuma · 2023-11-15T18:35:06Z

...il/src/main/java/org/wildfly/security/http/util/SetMechanismInformationMechanismFactory.java

@@ -110,7 +113,7 @@ public String getMechanismName() {
                        public String getHostName() {
                            return hostName;
                        }
-                    })});
+                    }), new RequestInformationCallback(request.getRequestURI())});


Should we be handling the new RequestInformationCallback from this class? Wondering if it would make more sense to introduce a new SetRequestionInformationMechanismFactory. WildFly Core would need to be updated accordingly.

@fjuma I was not sure at first since the HttpServerAuthenticationMechanismFactory is documented as Factory to create authentication mechanisms which did not seem related to logging or similar purpose, but it's true that this requestURI is a bit similar to already existing SocketAddressCallbackServerMechanismFactory and its socket address handling. So I made the changes, let me know what you think? Thank you!

@fjuma Here is affiliated commit for wildfly-core Skyllarr/wildfly-core@f9cf847 , if you like the changes I can create a WFCORE issue and submit this as a PR

The Core changes look good to me.

fjuma · 2023-11-20T15:31:03Z

...il/src/main/java/org/wildfly/security/http/util/SetMechanismInformationMechanismFactory.java

@@ -73,6 +74,7 @@ public String getMechanismName() {
            public void evaluateRequest(HttpServerRequest request) throws HttpAuthenticationException {
                String host = request.getFirstRequestHeaderValue(HOST);
                String resolvedHostName = null;
+                URI requestedUri = request.getRequestURI();


Is this needed here? It doesn't look like it's used.

fjuma · 2023-11-20T15:32:13Z

...util/src/main/java/org/wildfly/security/http/util/SetRequestInformationMechanismFactory.java

+ *
+ * @author <a href="mailto:dvilkola@redhat.com">Diana Krepinska</a>
+ */
+public class SetRequestInformationMechanismFactory implements HttpServerAuthenticationMechanismFactory {


I think this should be called RequestInformationCallbackServerMechanismFactory to be consistent with other the factories, right?

I see now, we also have a SetMechanismInformationMechanismFactory so that's probably why you named this SetRequestInformationMechanismFactory. That's fine then.

@fjuma I renamed this because I was thinking the same before. The SocketAddressCallbackServerMechanismFactory also uses the Callback in its name. I think the RequestInformationCallbackServerMechanismFactory is more clear?

fjuma · 2023-11-20T15:32:50Z

...util/src/main/java/org/wildfly/security/http/util/SetRequestInformationMechanismFactory.java

+ */
+public class SetRequestInformationMechanismFactory implements HttpServerAuthenticationMechanismFactory {
+
+    private HttpServerAuthenticationMechanismFactory delegate;


This can be final.

fjuma · 2023-11-20T15:33:24Z

...util/src/main/java/org/wildfly/security/http/util/SetRequestInformationMechanismFactory.java

+     * @param delegate the wrapped mechanism factory
+     */
+    public SetRequestInformationMechanismFactory(final HttpServerAuthenticationMechanismFactory delegate) {
+        this.delegate = delegate;


There should be a check here to make sure the delegate isn't null.

fjuma · 2023-11-20T15:36:58Z

...util/src/main/java/org/wildfly/security/http/util/SetRequestInformationMechanismFactory.java

+                try {
+                    callbackHandler.handle(new Callback[]{new RequestInformationCallback(request.getRequestURI())});
+                } catch (Throwable e) {
+                    throw log.unableToLocateMechanismConfiguration(e).toHttpAuthenticationException();


Should this be updated to request configuration?

@fjuma What do you mean by request configuration? I think now that I should update this to:

catch (IOException | UnsupportedCallbackException e) {
throw new HttpAuthenticationException(e);
}

wdyt?

"unableToLocateMechanismConfiguration" seems specific to the "SetMechanismInformationMechanismFactory"

Your suggested update looks fine.

@fjuma Thank you, updated

Skyllarr · 2023-11-20T15:53:47Z

Hi @darranl , this PR now adds a new RequestInformationCallback and new SetRequestInformationCallbackServerMechanismFactory to pass the request URI to server authentication context . Do you agree with this approach? Thanks!

darranl

Looks good for me.

Looking at this could be tempted to even go one step further and have a CallBack that allows a mechanism to generically send over attributes it feels appropriate but that should probably have further discussion.

Skyllarr · 2023-11-28T11:13:08Z

Looks good for me.

Looking at this could be tempted to even go one step further and have a CallBack that allows a mechanism to generically send over attributes it feels appropriate but that should probably have further discussion.

@darranl Thanks for review! Based on your comment I decided to change the callback to have a HashMap<String, Object> properties instead of only URI to make it more generic for the future. Can you please have another look? Thank you!

Skyllarr · 2023-11-28T15:31:32Z

.../server/base/src/main/java/org/wildfly/security/auth/server/ServerAuthenticationContext.java

+                    HashMap<String, Object> props = ((RequestInformationCallback) callback).getProperties();
+                    Attributes runtimeAttributes = new MapAttributes();
+                    for (Map.Entry<String, Object> entry : props.entrySet()) {
+                        runtimeAttributes.addFirst(entry.getKey(), entry.getValue().toString());


@darranl @fjuma Updated this PR to use a map of <String, Function> as properties in the RequestInformationMechanismFactory. I am not sure if it is okay to add all of the properties to the runtimeAttributes and not just Request-URI ? I wanted to introduce a public static final String REQUEST_URI somewhere in this repo, but I did not find a good place without introducing new dependencies or classes with constants, and this is a maintenance branch also. But maybe putting all of the properties from the callback into the runtimeAttributes indiscriminately makes sense now.

"Request-URI" constant is defined in the wildfly-core repo PR.

Also, this is relying on the toString() method being implemented well on the entry.getValue().

Let me know what you think. Thanks!

Since this is more generic now, adding all of the properties to the runtimeAttributes makes sense to me.

For the REQUEST_URI constant, maybe that could be added to the SetRequestInformationCallbackMechanismFactory and then it could be used from WildFly Core?

@fjuma Thanks, it is a good idea, though it might make make an unneeded precedence that the REQUEST_URI should be one of the properties or that all possible properties should be placed there.
@darranl WDYT ?

I think I would be tempted to try and keep the list of properties out of Elytron for now but in WildFly Core it should define Functions for all properties.

Late on I think custom implementations of the functions could be added to extract Cookie information etc so may be good to control the default behaviour over there?

@darranl Okay sounds good to me, and that is the current state of these PRs. Request-URI and its function is currently defined in wildfly-core as seen in the PR and if any other functions will be added in the future, those can be added to the same place in wildfly-core. Please check that your approval on wildfly-core still holds as the code has been changed since your last approval. Thank you!

@fjuma Let us know if you agree and if so, can you please approve this PR and the wildfly-core PR? Thank you!

…cessfulAuthenticationEvent and RealmFailedAuthenticationEvent

fjuma

Thanks, @Skyllarr! Looks good.

Skyllarr added the Maintenance label Sep 11, 2023

Skyllarr force-pushed the JBEAP-25337 branch from 2ec6427 to 6c8e29d Compare September 12, 2023 13:28

Skyllarr marked this pull request as ready for review September 12, 2023 13:28

fjuma requested changes Sep 12, 2023

View reviewed changes

Skyllarr force-pushed the JBEAP-25337 branch from 6c8e29d to 785cbb3 Compare September 15, 2023 10:29

Skyllarr force-pushed the JBEAP-25337 branch 2 times, most recently from ee94a77 to c84d020 Compare October 23, 2023 11:46

fjuma requested changes Nov 15, 2023

View reviewed changes

Skyllarr force-pushed the JBEAP-25337 branch from c84d020 to d21994a Compare November 20, 2023 13:14

fjuma reviewed Nov 20, 2023

View reviewed changes

Skyllarr force-pushed the JBEAP-25337 branch from d21994a to 9f0768b Compare November 20, 2023 15:56

darranl approved these changes Nov 22, 2023

View reviewed changes

Skyllarr force-pushed the JBEAP-25337 branch 2 times, most recently from 4e62872 to 5e79fa5 Compare November 27, 2023 20:33

Skyllarr force-pushed the JBEAP-25337 branch 2 times, most recently from 1a53535 to 39d356d Compare November 28, 2023 15:28

Skyllarr commented Nov 28, 2023

View reviewed changes

Skyllarr mentioned this pull request Nov 28, 2023

[WFCORE-6627] Make use of the SetRequestInformationMechanismFactory to set the request information during HTTP authentication wildfly/wildfly-core#5779

Merged

Skyllarr force-pushed the JBEAP-25337 branch from 39d356d to 8337be0 Compare November 28, 2023 15:42

[ELY-2583] Make requestURI and Source-Address available from RealmSuc…

8bf64cd

…cessfulAuthenticationEvent and RealmFailedAuthenticationEvent

Skyllarr force-pushed the JBEAP-25337 branch from 8337be0 to 8bf64cd Compare November 28, 2023 15:44

fjuma approved these changes Dec 4, 2023

View reviewed changes

ivassile mentioned this pull request Jan 4, 2024

[ELY-2583] Make requestURI and Source-Address available from RealmSuccessfulAuthenticationEvent and RealmFailedAuthenticationEvent #2082

Closed

Skyllarr merged commit a0c9828 into wildfly-security:1.15.x Jan 10, 2024
3 checks passed

[ELY-2583] Make requestURI and Source-Address available from RealmSuccessfulAuthenticationEvent and RealmFailedAuthenticationEvent #1952

[ELY-2583] Make requestURI and Source-Address available from RealmSuccessfulAuthenticationEvent and RealmFailedAuthenticationEvent #1952

Conversation

Skyllarr commented Sep 11, 2023 • edited by fjuma

fjuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr Nov 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr commented Nov 20, 2023

darranl left a comment

Choose a reason for hiding this comment

Skyllarr commented Nov 28, 2023

Skyllarr Nov 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma left a comment

Choose a reason for hiding this comment

Skyllarr commented Sep 11, 2023 •

edited by fjuma

Skyllarr Nov 20, 2023 •

edited

Skyllarr Nov 28, 2023 •

edited