CXF-8121 Improve STS REST interface #580

dtsybulka · 2019-09-28T07:23:04Z

No description provided.

…ent.

ashakirin

I like and support the idea to create STS Rest into separate module, because:

it makes start with Rest STS quickly and easier;
potentially helps to split Rest STS from SOAP overhead;
provides cleaner JAX-RS API for STS.

services/sts/sts-rest/src/main/java/org/apache/cxf/sts/rest/api/BaseResponse.java

services/sts/sts-rest/src/main/java/org/apache/cxf/sts/rest/api/GetTokenRequest.java

services/sts/sts-rest/src/main/java/org/apache/cxf/sts/rest/api/RealmSecurityTokenService.java

...s/sts-rest/src/main/java/org/apache/cxf/sts/rest/authentication/JwtAuthenticationFilter.java

services/sts/sts-rest/src/main/java/org/apache/cxf/sts/rest/impl/JwkOperation.java

...ts/sts-rest/src/main/java/org/apache/cxf/sts/rest/impl/RealmSecurityConfigurationFilter.java

services/sts/sts-rest/pom.xml

reta · 2019-11-10T19:03:00Z

Thanks @dtsybulka, LGTM, @ashakirin anything you want to add / comment / change?

coheigea · 2019-11-11T16:28:03Z

@reta, please hold off on the merge for a while, there are some things I need to check first with the PR.

reta · 2019-11-12T01:28:07Z

@coheigea no objections, won't merge without your and @ashakirin approvals, thanks!

dtsybulka · 2019-12-09T15:25:29Z

@coheigea could you please tell when you could check and review the PR? Thanks in advance.

coheigea · 2019-12-18T18:21:04Z

@dtsybulka It's on my TODO list, but realistically it will be early in the new year before I can review it properly.

dtsybulka · 2020-02-10T12:49:23Z

Hi @coheigea,
I hope you are fine. Could you please let me know if you had a chance to check the PR.
Best Regards,
Dmitry

coheigea · 2020-02-24T17:13:17Z

Sorry, it'll be a few weeks before I can look at it, but it is on my radar for CXF 3.4.0.

coheigea

I'm sorry for the very long delay in reviewing this PR. I'm happy to include it in CXF 3.4.0 if the changes I suggested are made. Do you have some time to address the changes in the near future?

coheigea · 2020-06-23T13:53:12Z

services/sts/sts-core/src/main/java/org/apache/cxf/sts/rest/RESTSecurityTokenServiceImpl.java

@@ -69,7 +69,7 @@
    public static final Map<String, String> DEFAULT_CLAIM_TYPE_MAP;
    public static final Map<String, String> DEFAULT_TOKEN_TYPE_MAP;

-    private static final Map<String, String> DEFAULT_KEY_TYPE_MAP = new HashMap<>();
+    public static final Map<String, String> DEFAULT_KEY_TYPE_MAP = new HashMap<>();


This map should be made an unmodifiable map as per the other public maps

coheigea · 2020-06-23T13:55:27Z

...sts/systests/basic/src/test/resources/org/apache/cxf/systest/sts/rest/cxf-realm-rest-sts.xml

+
+    <import resource="classpath:META-INF/cxf/cxf.xml"/>
+
+    <bean class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer"/>


Change all instances of PropertyPlaceholderConfigurer to org.springframework.context.support.PropertySourcesPlaceholderConfigurer instead

coheigea · 2020-06-23T13:58:06Z

services/sts/systests/basic/src/test/java/org/apache/cxf/systest/sts/rest/STSRealmRestTest.java

+
+    @org.junit.AfterClass
+    public static void cleanup() throws Exception {
+        SecurityTestUtil.cleanup();


This method is removed in the latest codebase

coheigea · 2020-06-23T14:02:42Z

services/sts/systests/basic/src/test/java/org/apache/cxf/systest/sts/rest/STSRealmRestTest.java

+        validateJWTToken(token);
+    }
+
+    @org.junit.Ignore


Why are these two tests Ignored?

coheigea · 2020-06-23T14:03:48Z

services/sts/systests/basic/src/test/java/org/apache/cxf/systest/sts/rest/STSRealmRestTest.java

+/**
+ * Some tests for the REST interface of the CXF STS.
+ */
+public class STSRealmRestTest extends AbstractBusClientServerTestBase {


There is a huge amount of duplicated code between this class and STSRESTTest. I wonder if we can't abstract the tests to a common base class, and subclass just the bits that get the tokens?

coheigea · 2020-06-23T14:43:25Z

services/sts/sts-rest/src/main/java/org/apache/cxf/sts/rest/api/RealmSecurityTokenService.java

+                description = "In case wrong configuration of RS security for requested realm")
+        }
+    )
+    JsonWebKeys getPublicVerificationKeys();


Is this method tested anywhere? I don't see it...

coheigea · 2020-06-23T15:37:27Z

...s/sts-rest/src/main/java/org/apache/cxf/sts/rest/authentication/JwtAuthenticationFilter.java

+        final Message message = getCurrentMessage();
+        final RequestSecurityTokenResponseType response = validateOperation.validate(
+            createValidateRequestSecurityTokenType(tokenString, "jwt"), null,
+            new WrappedMessageContext(message));


Why WrappedMessageContext here? We can just pass through "message" and then eliminate the jaxws frontend from the pom.

coheigea · 2020-06-23T15:51:16Z

...ts/sts-rest/src/main/java/org/apache/cxf/sts/rest/impl/RealmSecurityConfigurationFilter.java

+import static org.apache.cxf.jaxrs.utils.HttpUtils.getProtocolHeader;
+import static org.apache.cxf.message.Message.HTTP_REQUEST_METHOD;
+import static org.apache.cxf.message.Message.PROTOCOL_HEADERS;
+import static org.springframework.util.CollectionUtils.isEmpty;


Replace this with: org.apache.cxf.common.util.StringUtils.isEmpty

coheigea · 2020-06-23T15:56:54Z

services/sts/sts-rest/pom.xml

+            org.springframework.ldap*;resolution:=optional,
+            net.sf.ehcache*;resolution:=optional;version="[2.5, 3.0.0)",
+            org.opensaml*;version="${cxf.opensaml.osgi.version.range}",


I think these three lines can be removed.

coheigea · 2020-06-23T16:02:09Z

services/sts/sts-rest/src/main/java/org/apache/cxf/sts/rest/api/BaseResponse.java

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cxf.sts.rest.api;


The package name of "org.apache.cxf.sts.rest" is a bit problematic in OSGi, as it clashes with the module in STS Core. Can we rename this to something different?

Is org.apache.cxf.sts.rs will be OK?

Yes I guess so

coheigea · 2020-06-23T16:05:26Z

Also, there is a merge conflict with CustomParameterTest.

� Conflicts: � services/sts/systests/advanced/src/test/java/org/apache/cxf/systest/sts/custom/CustomParameterTest.java

dtsybulka added 2 commits September 28, 2019 10:20

CXF-8121 Improve STS REST interface

1262c12

CXF-8121: Add advanced tests. Small refactoring according the PR comm…

f19b3b8

…ent.

dtsybulka force-pushed the CXF-8121 branch from 2794e77 to f19b3b8 Compare October 8, 2019 18:40

ashakirin reviewed Oct 15, 2019

View reviewed changes

dtsybulka added 2 commits November 3, 2019 11:35

Changes according PR's comments

fed6dbc

Changes according PR's comments

ba9d9a8

reta reviewed Nov 4, 2019

View reviewed changes

services/sts/sts-rest/pom.xml Outdated Show resolved Hide resolved

reta reviewed Nov 4, 2019

View reviewed changes

services/sts/sts-rest/pom.xml Outdated Show resolved Hide resolved

Fix poms according PR commit. Small fixes in filters.

7b2a427

coheigea requested changes Jun 23, 2020

View reviewed changes

dtsybulka added 2 commits July 23, 2020 17:28

Merge branch 'master' of github.com:apache/cxf into CXF-8121

494f0e4

� Conflicts: � services/sts/systests/advanced/src/test/java/org/apache/cxf/systest/sts/custom/CustomParameterTest.java

[CXF-8121] Changes according PR's comments

b10b7b9

reta force-pushed the master branch from fe56bf6 to b15e358 Compare June 16, 2022 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CXF-8121 Improve STS REST interface #580

CXF-8121 Improve STS REST interface #580

dtsybulka commented Sep 28, 2019

ashakirin left a comment

reta commented Nov 10, 2019

coheigea commented Nov 11, 2019

reta commented Nov 12, 2019

dtsybulka commented Dec 9, 2019

coheigea commented Dec 18, 2019

dtsybulka commented Feb 10, 2020

coheigea commented Feb 24, 2020

coheigea left a comment

coheigea Jun 23, 2020

coheigea Jun 23, 2020

coheigea Jun 23, 2020

coheigea Jun 23, 2020

coheigea Jun 23, 2020

coheigea Jun 23, 2020

coheigea Jun 23, 2020

coheigea Jun 23, 2020

coheigea Jun 23, 2020

coheigea Jun 23, 2020

dmitrytsybulko Jul 23, 2020

coheigea Jul 23, 2020

coheigea commented Jun 23, 2020


		<import resource="classpath:META-INF/cxf/cxf.xml"/>

		<bean class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer"/>

CXF-8121 Improve STS REST interface #580

Are you sure you want to change the base?

CXF-8121 Improve STS REST interface #580

Conversation

dtsybulka commented Sep 28, 2019

ashakirin left a comment

Choose a reason for hiding this comment

reta commented Nov 10, 2019

coheigea commented Nov 11, 2019

reta commented Nov 12, 2019

dtsybulka commented Dec 9, 2019

coheigea commented Dec 18, 2019

dtsybulka commented Feb 10, 2020

coheigea commented Feb 24, 2020

coheigea 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

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

coheigea commented Jun 23, 2020