Single Sign On - SAML Response generation

I am trying to implement a IDP initiated Single Sign On Solution (service similar to onelogin's) to Cloud based Service providers such as Google Apps, Salesforce etc.

The project is a Maven eclipse project (Web app) and the main servlet which consumes SAML Request sent via HTTP-GET / HTTP-POST and generates a valid SAML Response, digitally signs it and attempts to POST the same to the ACS url is the SamlHandler.java file.

The code of which I am posting below for a review:

(Please note that the project is for learning and not for any commercial purposes)

I am breaking down the code so that it becomes easier to review the same.

Also kindly excuse my haphazard coding style and messy comments. This is still very much unfinished and so I did not give much importance to the code formatting.

checkSession method to verify that the user session is still valid.

public boolean checkSession(HttpServletRequest request) {

    try {

        HttpSession session = request.getSession(false);

        if (session.getAttribute("loggedIn").equals("yes")) {
            System.out.println("Session is valid.");
            // If the session is valid retrieve the user credentials
            // Every user will be uniquely identified by the email and
            // domain.
            email = (String) session.getAttribute("email");
            domain = (String) session.getAttribute("domain");
            sessionId = (String) session.getId();
            return true;

        }

        else {
            System.out.println("User is not logged in.");
            return false;

        }

    } catch (Exception e) {
        // If the loggedIn attribute is not set.
        System.out.println("User session invalid / User is not logged in.");
        return false;
    }

}

doPost and doGet method which invokes the handleSamlResponse method which takes on further processing of the request to yield the response.

```
public void doPost(HttpServletRequest request, HttpS

the part whose implementation I am very doubtful of

I'm assuming your code works. If it doesn't, it's off-topic, and your question should get closed.

Duplication

String dateTimeFormat = "yyyy-MM-dd'T'HH:mm:ss'+05:30'";
    res_issueInstant = new DateTime().toString(dateTimeFormat).trim();
    res_notbefore = new DateTime().toString(dateTimeFormat).trim();
    res_notonorafter = new DateTime().plusMinutes(5)
            .toString(dateTimeFormat).trim();

You create multiple DateTime objects. It's possible that between these lines of code, the processor switches to a different process to do things. This doesn't last very long, as your processor switches a LOT of times each second, but what you will see is that there can be a difference between these times. Don't rely on this behavior. Create only 1 DateTime object, and either set res_issueInstant and res_notbefore to be the same value, or specifically add a certain amount to res_notbefore.

org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
            SAML_PROTOCOL_NS_URI_V20, "Extensions");
    if (nodeList.getLength() != 0) {
        xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);
    } else {
        nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Status");
        xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);
    }

The last line in each if statement branch is duplicated. Let's take it out of the if statement.

org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
            SAML_PROTOCOL_NS_URI_V20, "Extensions");
    if (nodeList.getLength() != 0) {
    } else {
        nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Status");
    }
    xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);

It's still not nice, because you have an empty if statement block. Invert the if conditional and swap the blocks.

org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
            SAML_PROTOCOL_NS_URI_V20, "Extensions");
    if (nodeList.getLength() == 0) {
        nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Status");
    } else {
    }
    xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);

Lastly, remove the unnecessary else block.

org.w3c.dom.NodeList nodeList = w3cElement.getElementsByTagNameNS(
            SAML_PROTOCOL_NS_URI_V20, "Extensions");
    if (nodeList.getLength() == 0) {
        nodeList = w3cElement.getElementsByTagNameNS(
                SAML_PROTOCOL_NS_URI_V20, "Status");
    }
    xmlSigInsertionPoint = nodeList.item(nodeList.getLength() - 1);

Another bit of code I saw:

strResponseXML = strResponseXML.replaceAll("_ASSERTION_ID",
            res_assertionId);
    strResponseXML = strResponseXML.replaceAll("_REQUEST_ID", req_Id);
    strResponseXML = strResponseXML.replaceAll("_RESPONSE_ID", res_Id);
    strResponseXML = strResponseXML.replaceAll("_ISSUE_INSTANT",
            res_issueInstant);
    strResponseXML = strResponseXML.replaceAll("_ISSUER", res_issuer);
    strResponseXML = strResponseXML.replaceAll("_NAMEID", res_nameId);
    strResponseXML = strResponseXML.replaceAll("_NOTBEFORE",
            res_notbefore);
    strResponseXML = strResponseXML.replaceAll("_NOTONORAFTER",
            res_notonorafter);
    strResponseXML = strResponseXML.replaceAll("_ACS_URL", acs);
    strResponseXML = strResponseXML.replaceAll("_DOMAIN", domain);

This seems like it could be a for loop.
I'd personally use a 2D String array here. A HashMap is a nice idea but you might want to preserve the order of operations, and a HashMap doesn't allow you to do so. Additionally, we want to make the code readable.

final String[][] replacementMappings = new String[][]{
    {"_ASSERTION_ID", res_assertionId},
    {"_REQUEST_ID", req_Id},
    {"_RESPONSE_ID", res_Id},
    {"_ISSUE_INSTANT", res_issueInstant},
    {"_ISSUER", res_issuer},
    {"_NAMEID", res_nameId},
    {"_NOTBEFORE", res_notbefore},
    {"_NOTONORAFTER", res_notonorafter},
    {"_ACS_URL", acs},
    {"_DOMAIN", domain}
}

I have declared the array to be final since we're not gonna be changing the mappings. Then, the for loop:

for(int i = 0; i < replacementMappings.length; i++){
    strResponseXML = strResponseXML.replaceAll(replacementMappings[i][0], replacementMappings[i][1];
}

This cleans up that section, mostly.

Constants

String JSR_105_PROVIDER = "org.jcp.xml.dsig.internal.dom.XMLDSigRI";
    String SAML_PROTOCOL_NS_URI_V20 = "urn:oasis:names:tc:SAML:2.0:protocol";

You don't assign further values to these variables. I suggest making them final. Perhaps even moving them outside the function. Typically, constants are placed near the top of the class, above the other variable declarations. Assuming this class isn't a singleton already, I'd make these variables `private static fina

StackExchange Code Review Q#59169, answer score: 6