Skip to content
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.

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

First commit for oidc handler. #14

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@hasinidilanka
Copy link
Contributor

commented May 24, 2018

GSoC 2018 — OpenID Connect Relying Party Implementation for Apache Sling

OpenID Connect Introduction

OpenID Connect(OIDC) is an authentication protocol. There are three participants that engage in OIDC message passing.

  • End user — End user is same as the resource owner who is requested for identity information.
  • Relying Party(RP) — Relying Party is same as the OAuth2.0 client requiring End-User authentication and claims from an OpenID Provider.
  • OpenID Provider(OP) — OpenID provider is same as the OAuth2.0 Authorization Server that is capable of authenticating the End-User and providing claims to a Relying Party about the Authentication event and the End-User.

The below diagram illustrates the high level request passing that happens in OIDC.

open-id-connect

First the RP will send an authentication request for the OpenID Provider(OP). Then OP will redirect the end-user to an authentication page. Then OP will authenticate the end-user and will obtain authorization. OP will send an access token and ID token to the RP. RP can send back the access token to OP’s userinfo endpoint and request user claims. Then OP can provide user claims back to RP.

Project Description

The purpose of this project is to implement OpenID Connect Relying party handler for Apache Sling. Following were identified as end deliverables for the proposed project.

  • Send an authentication request for a OP.
  • Request Access Token and ID Token from OP.
  • Validate ID Token using nonce value.
  • Get authenticated users.
  • Save user claims.

Below diagram shows the proposed approach of the implementation.
project proposal - apache sling 1
TODOs

  • Implementing test cases.
  • Cookie value implementation.

For more project information :

@rombert rombert self-assigned this May 24, 2018

@rombert

This comment has been minimized.

Copy link
Contributor

commented May 24, 2018

Thanks for the pull request @hasinidilanka , great to see this advancing! I will add some inline comments soon.

@rombert
Copy link
Contributor

left a comment

I added my thoughts to the changes. Don't be discouraged by the number of comments, the initial project setup is probably the most boring part and the least documented one for Sling :-) But also it's quite important to get right.

Besides the individual comments, I also want to discuss a bit more how the project is structured.

You currently propose a reactor POM with two modules:

- reactor
\- core
\- ui

Have you considered the pros and cons for having these 3 modules versus having a single module? You can have both Java classes and content resources and scripts in a single OSGi bundle.

Another think I want to bring to your attention is that we prefer deploying content resources ( like the one you added to SLING-INF/content/my-first-node.xml ) and then handling them using scripts ( like the one you added to /SLING-INF/scripts/html.esp ). This is much more flexible in terms of where the link is created. I suggest you take a look at https://sling.apache.org/documentation/the-sling-engine/url-to-script-resolution.html to get the big picture. Currently we support multiple scripting engines, but the most widely used are JSP ( https://sling.apache.org/documentation/bundles/scripting/scripting-jsp.html ) and HTL ( https://github.com/Adobe-Marketing-Cloud/htl-spec, https://sling.apache.org/documentation/bundles/scripting/scripting-htl.html ) . You can pick either of these.

Looking forward to your reply.

--><project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<artifactId>org.apache.sling.oidchandler</artifactId>

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

I suggest you use the 'sling' parent pom, this will allow you to remove quite a number of extra configurations from the POM file. You can keep the reactor pom below https://github.com/apache/sling-whiteboard/blob/master/oidc-handler/pom.xml, but that one will only have the <modules> entry.

<version>1.0.0-SNAPSHOT</version>
</parent>
<groupId>org.apache.sling</groupId>
<artifactId>core</artifactId>

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

We prefix the artifactId with org.apache.sling . And since the artifact id becomes the Bundle-SymbolicName and therefore should be representative of that the bundle does it should also probably include auth and/or oidc.

<groupId>org.apache.sling</groupId>
<artifactId>core</artifactId>
<version>1.0.0-SNAPSHOT</version>
<name>core</name>

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

This should be "Apache Sling ..." where ... should be replaced by a compact name , for instance we have "Apache Sling Commons Log"

<artifactId>core</artifactId>
<version>1.0.0-SNAPSHOT</version>
<name>core</name>
<description>org.apache.sling - core</description>

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

Unless you have something extra to add here you can probably remove this.

<plugin>
<groupId>biz.aQute.bnd</groupId>
<artifactId>bnd-maven-plugin</artifactId>
<version>3.5.0</version>

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

The version should be inherited from the parent POM.

</dependencies>


<!--<build>-->

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

Please remove all commented-out code.

@@ -0,0 +1,56 @@
<!DOCTYPE html>

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

This seems to have lots of extra classes which are referenced but not added to the page. I suggest that you start with a minimal page with no extra markup, we can take care of styling it later.

@@ -0,0 +1,42 @@
<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

This is not referenced anywhere, I don't think you need it.

@@ -0,0 +1,32 @@
//

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

You're not using custom node types so you can remove it.

@@ -0,0 +1,37 @@
<!DOCTYPE html PUBLIC \"-//IETF//DTD HTML 2.0//EN\">

This comment has been minimized.

Copy link
@rombert

rombert May 24, 2018

Contributor

This is not used anywhere so you can remove it.

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch from 9eebed0 to 091d64a May 24, 2018

@hasinidilanka

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2018

@rombert Thank you for the comments, they were very helpful for me. I resolved some of the comments.

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch 2 times, most recently from a76d0dd to e09faf5 May 28, 2018

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch from 981614c to d350543 Jun 6, 2018

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch 3 times, most recently from 9681453 to 8d9a1ec Jul 5, 2018

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch 2 times, most recently from 4fea79a to 7284107 Jul 18, 2018

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch 2 times, most recently from 0f87f49 to 4ea7954 Aug 2, 2018

@hasinidilanka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

@rombert Please review the PR.

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch 3 times, most recently from 0b46432 to 53f7245 Aug 2, 2018

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch 2 times, most recently from 3078d65 to 646685a Aug 11, 2018

@hasinidilanka

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2018

@rombert Can you please check the PR. I have finished the implementation of flow with state and nonce value validations.

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch from 646685a to 382b951 Aug 13, 2018

@rombert

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Thanks for the updates @hasinidilanka . I'll re-post my feedback here into a single message, to make it easier to review. I'll separate it into the two parts:

  1. What is needed to merge the initial submission
  2. What is needed to make this a 'proper' Sling module and move it to its own repository

Initial merge

  1. You have a reactor pom at /oidc-hander/pom.xml and a module pom at /oidc-handler/core/pom.xml. We only need one, so please move everything under /oidc-hander. This will make it easier to see the history of further changes.
  2. The errorEndpoint and loginEndpoint class names must start with an uppercase letter
  3. All occurences of e.printStackTrace must be replaced with either a log call or with rethrowing the exception

Feature completion

There are three directions of code review here:

  • Making code better suited to working in Sling - no more static methods/fields, use @Component and @Reference
  • various naming/design issues
  • complete the implementation of the OIDCAuthenticationHandler

Take the time and read through them, make sure you understand them and ask for clarification when needed and feel free to approach them in any order and submit PRs whenever you have something fixed.

  1. You are using the OIDCConfigServlet only to store configuration, so it's not really a servlet :-) My suggestion would be to remove this class and expose the configuration to the OIDCAuthenticationHandler. Then, if you need to pass it to other classes, you can do so from the there.

Note that you can also reference the component, e.g.

   @Reference
   private OIDCAuthenticationHandler authHandler.
  1. In the OIDCConfiguration I suggest you remove the 'boolean'/'string'/'password' suffixes. They're not really needed.

  2. The AuthenticationError class is not an error, just has utility method to redirect to an error page. Maybe rename or move it somewhere else?

  3. The Handler class could be named better. What exactly does it do?

  4. The Handler class has some hardcoded fields - stateValue and nonceValue. What do those do? And should they not be secret or configurable?

  5. The IdTokenHandler class only has a verify method, maybe find a more precise name than that?

  6. There are unused imports in IdTokenHandler.

  7. The OIDCAuthenticationHandler class is incomplete. Credential extraction is done in the extractCredentials, and that is good. But we're also missing a proper implementation of requestCredentials, which - if I got this correctly - should replace whatever the loginEndpoint is doing. You should also implement dropCredentials

  8. The UserManagerImpl class should not use FrameworkUtil and the BundleContext to get a SlingRepository. It should instead be an OSGi component and obtain the repository using annotations, e.g.

@Component
public class UserManagerImpl {
    @Reference
    private ResourceResolverFactory factory;

    public boolean createUser() {
        ResourceResolver resolver = factory.loginAdministrative(null);
        Session session = resolver.adaptTo(Session.class);
    }

}
  1. In the UserManagerImpl, don't cache sessions and make sure to close them after you're done with them.

  2. The field values from OPConstant are only used in the OIDCConfiguration class. Any reason not to move them there? You can always use something like

tring oidc_callback_url_string() default "http://localhost:8080/";

@hasinidilanka hasinidilanka force-pushed the hasinidilanka:master branch from 382b951 to 1a6d592 Sep 10, 2018

@hasinidilanka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2018

@rombert
Sorry for taking a long time to fix this. I have fixed the first three requests for initial submission. Can you please review and merge the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.