Add SSO feature for Store application #4316

npamudika · 2017-07-28T04:53:41Z

Proposed changes in this pull request

SSO implementation for Store application
Unit tests for AuthenticatorService class

When should this PR be merged

ASAP

Follow up actions

[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly?

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

fazlan-nazeem · 2017-07-28T05:09:23Z

...arbon.apimgt.core/src/test/java/org/wso2/carbon/apimgt/core/dao/impl/DBScriptRunnerUtil.java

@@ -124,7 +124,7 @@ private static void executeSQL(String sql, Connection connection) {

            String q = "select index_name,index_type,status,domidx_status,domidx_opstatus from user_indexes "
                    + "where index_type like '%DOMAIN%' and (domidx_status <> 'VALID' or domidx_opstatus <> 'VALID')";
-            try (Statement statement = connection.createStatement(); ResultSet rs = statement.executeQuery(q);) {
+            try (Statement statement = connection.createStatement(); ResultSet rs = statement.executeQuery(q)) {
                while (rs.next()) {
                    if (rs.getString("index_name").equals("API_INDEX")) { // re build index if it has failed.


Do the equals other way around to avoid null pointer exception. i.e "API_INDEX".equals(rs.getString("index_name")). Check equals operations in other places as well.

harsha89 · 2017-07-28T06:02:36Z

...pimgt.authenticator/src/test/java/org/wso2/carbon/apimgt/authenticator/LogInServiceTest.java

-                    return true;
-                }
-                return false;
+                return hostname.equals("localhost");


the equals should be otherway around "localhost".equals(hostname)

harsha89 · 2017-07-28T06:05:09Z

...main/java/org/wso2/carbon/apimgt/rest/api/authenticator/internal/ServiceReferenceHolder.java

+            if (configProvider != null) {
+                config = configProvider.getConfigurationObject(APIMStoreConfigurations.class);
+            } else {
+                log.error("Configuration provider is null");


is this actually an error?

Because configiProvider null means config is becoming null which you then set the default configurationss

Here configProvider is getting null due to an osgi error.
Default configurations has set in line 61.

harsha89 · 2017-07-28T06:08:26Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+            log.debug("Received consumer key & secret for " + appName + " application.");
+        }
+        try {
+            if (grantType.equals(KeyManagerConstants.AUTHORIZATION_CODE_GRANT_TYPE)) {


equals should be otherwaround

harsha89 · 2017-07-28T06:08:36Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+                    String errorMsg = "No Authorization Code available.";
+                    log.error(errorMsg, ExceptionCodes.ACCESS_TOKEN_GENERATION_FAILED);
+                }
+            } else if (grantType.equals(KeyManagerConstants.PASSWORD_GRANT_TYPE)) {


harsha89 · 2017-07-28T06:12:05Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+                        .createAccessTokenRequest(userName, password, grantType, refreshToken,
+                                null, validityPeriod, scopes,
+                                consumerKeySecretMap.get("CONSUMER_KEY"), consumerKeySecretMap.get("CONSUMER_SECRET"));
+                accessTokenInfo = getKeyManager().getNewAccessToken(accessTokenRequest);


since we dont have a else statement is there any possibility that it can effect the follow because accessTokenInfo is getting null?

When accessTokenInfo is null, it will be handled when calling getTokens() method at AuthenticatorAPI class.

harsha89 · 2017-07-28T06:15:21Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+     * Represents Payload of JWT.
+     */
+    private class JWTTokenPayload {
+        private String sub;


Why we need inner class?

Github issue created at,
#4324 [C5][Authenticator] Handle JWT

harsha89 · 2017-07-28T06:15:35Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+            // for the oAuthAppRequest constructor argument.
+            // This value is not related to DCR application creation.
+            OAuthAppRequest oAuthAppRequest = new OAuthAppRequest(clientName,
+                    callBackURL, "Application", grantTypes);


Application to a constant?

harsha89 · 2017-07-28T06:16:04Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+                oAuthData.addProperty(KeyManagerConstants.TOKEN_SCOPES, scopes);
+                oAuthData.addProperty(KeyManagerConstants.AUTHORIZATION_ENDPOINT,
+                        storeConfigs.getAuthorizationEndpoint());
+                oAuthData.addProperty("is_sso_enabled", storeConfigs.isSsoEnabled());


can we move this ti a constant?

harsha89 · 2017-07-28T06:16:16Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+                        storeConfigs.getAuthorizationEndpoint());
+                oAuthData.addProperty("is_sso_enabled", storeConfigs.isSsoEnabled());
+            } else {
+                String errorMsg = "No information available in OAuth application.";


Do we need to throw a exception here?

harsha89 · 2017-07-28T06:16:53Z

...henticator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorAPI.java

+        } else {
+            restAPIContext = AuthenticatorConstants.REST_CONTEXT + appContext;
+        }
+        String requestURL = (String) request.getProperty("REQUEST_URL");


move to a constant

pubudu538 · 2017-07-28T05:30:26Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+                oAuthData.addProperty(KeyManagerConstants.TOKEN_SCOPES, scopes);
+                oAuthData.addProperty(KeyManagerConstants.AUTHORIZATION_ENDPOINT,
+                        storeConfigs.getAuthorizationEndpoint());
+                oAuthData.addProperty("is_sso_enabled", storeConfigs.isSsoEnabled());


Let's move 'is_sso_enabled' to a constant.

nuwand · 2017-07-28T06:17:36Z

...henticator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorAPI.java

            AuthResponseBean authResponseBean = new AuthResponseBean();
            String appContext = AuthUtil.getAppContext(request);
            String restAPIContext;
-            if (appContext.contains("editor") || request.getUri().contains("publisher")) {
-                restAPIContext = AuthenticatorConstants.REST_CONTEXT + "/publisher";
+            if (appContext.contains(AuthenticatorConstants.EDITOR_APPLICATION) ||


I don't think we can compare application contexts in this manner since they can change. Please create a Github issue for this, we need to fix it properly in all places.

Github issue created at,
#4319 [C5][Authenticator] Compare application contexts

nuwand · 2017-07-28T06:30:11Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+        if (log.isDebugEnabled()) {
+            log.debug("Set scopes for " + appName + " application using swagger definition.");
+        }
+        // TODO: Get Consumer Key & Secret without creating a new app, from the IS side


Please create Github issues for TODOs, we'll forget them for sure otherwise :)

Github issue created at,
#4299 [C5][Authenticator] Getting details of an already existing DCR application

nuwand · 2017-07-28T06:43:32Z

...icator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/AuthenticatorService.java

+    private String getUsernameFromJWT(String jwt) throws KeyManagementException {
+        if (jwt != null && jwt.contains(".")) {
+            String[] jwtParts = jwt.split("\\.");
+            JWTTokenPayload jwtHeader = new Gson().fromJson(new String(Base64.getDecoder().decode(jwtParts[1]),


Shall we use a standard JWT library like Nimbus to handle JWT? If its hard to incorporate along with this feature please create a Github issue so that it doesn't get forgotten.

Github issue created at,
#4324 [C5][Authenticator] Handle JWT

nuwand · 2017-07-28T07:13:26Z

...uthenticator/src/main/java/org/wso2/carbon/apimgt/rest/api/authenticator/utils/AuthUtil.java

@@ -56,7 +56,7 @@ public static String getHttpOnlyCookieHeader(Cookie cookie) {

    public static String getAppContext(Request request) {
        //TODO this method should provide uuf app context. Consider the scenarios of reverse proxy as well.
-        return "/" + request.getProperty("REQUEST_URL").toString().split("/")[1];
+        return "/" + request.getProperty("REQUEST_URL").toString().split("/")[3];


I don't think this code will work when you reverse proxy the application. If you have a custom URL this logic will fail.

Related to the Github issue at,
#4319 [C5][Authenticator] Compare application contexts

nuwand · 2017-07-28T07:22:43Z

...ommons/src/main/java/org/wso2/carbon/apimgt/rest/api/common/impl/BasicAuthAuthenticator.java

-            return true;
-        }
-        return false;
+        return username.equals(password);


What's this? Do we have an idea to implement this? If so please create relevant GitHub issue.

Here we need to implement the authentication logic when the user name and password are provided.
Need to check for an API from IS.
Issue created. #4323

npamudika added 20 commits July 27, 2017 18:08

sso feature initial commit

d95126a

Add SSO feature - get Authorization Code changes

b435e31

SSO feature - Add Callback microservice

c4df213

Add SSO feature callback changes

cffbeb1

Add SSO feature - access token retrieval

f8cb146

Add SSO feature - Redirection and Configuration changes

6ae15f3

Add SSO Login feature changes

be8b4ef

Revert unwanted commit

7ed6f11

Revert unwanted changes

100a465

Revert commit changes

c56dceb

Review fixes

0aca22f

Review fixes

1f8eadc

Review fixes

2043226

Add AuthenticatorService, Move SSO related code to react authenticator

15d506e

Remove duplicate classes of SSO

da20943

Review fixes

012ecc8

Review fixes

67b0122

Add AuthenticatorService tests

303faba

Refactor code

1ef198b

AuthentictorService tests

6ac4344

fazlan-nazeem reviewed Jul 28, 2017

View reviewed changes

Review fixes - DBScriptRunnerUtil class

5f4b896

harsha89 requested changes Jul 28, 2017

View reviewed changes

pubudu538 reviewed Jul 28, 2017

View reviewed changes

nuwand requested changes Jul 28, 2017

View reviewed changes

Review fixes - Constants

a12ccb4

fazlan-nazeem approved these changes Jul 31, 2017

View reviewed changes

Review fixes - Throw exceptions

8e9f128

nuwand approved these changes Jul 31, 2017

View reviewed changes

harsha89 approved these changes Jul 31, 2017

View reviewed changes

harsha89 merged commit e353b6a into wso2:master Jul 31, 2017

npamudika mentioned this pull request Aug 8, 2017

[C5][Store] Supporting SSO in API Store #4368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSO feature for Store application #4316

Add SSO feature for Store application #4316

npamudika commented Jul 28, 2017

fazlan-nazeem Jul 28, 2017

harsha89 Jul 28, 2017

harsha89 Jul 28, 2017

harsha89 Jul 28, 2017

npamudika Jul 31, 2017

harsha89 Jul 28, 2017

harsha89 Jul 28, 2017

harsha89 Jul 28, 2017

npamudika Jul 31, 2017

harsha89 Jul 28, 2017

npamudika Jul 31, 2017

harsha89 Jul 28, 2017

harsha89 Jul 28, 2017

harsha89 Jul 28, 2017

harsha89 Jul 28, 2017

pubudu538 Jul 28, 2017

nuwand Jul 28, 2017

npamudika Jul 31, 2017

nuwand Jul 28, 2017

npamudika Jul 31, 2017

nuwand Jul 28, 2017

npamudika Jul 31, 2017

nuwand Jul 28, 2017

npamudika Jul 31, 2017

nuwand Jul 28, 2017

isharac Jul 31, 2017

Add SSO feature for Store application #4316

Add SSO feature for Store application #4316

Conversation

npamudika commented Jul 28, 2017

Proposed changes in this pull request

When should this PR be merged

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

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

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