Session Delete Improvements #135

isurusamarasekara · 2021-09-27T06:19:51Z

Purpose

Includes new features to the session management API along with wso2/carbon-identity-framework#3750.

Goals

Includes new features to the session management API:

Terminate active sessions that fulfil the criteria determined by the filter parameter value.

Approach

Service Specification

https://app.swaggerhub.com/apis/sessionapi/wso-2_identity_server_user_session_management_api_definition/v1

Sessions API

Extended /sessions endpoint with delete implementation
Operation available for privileged users. Delete active sessions on the system retrieved based on. a filtering criteria.
Based off of #132

User stories

As a System Admin user, I want to terminate user sessions based on a criteria.

Release note

Session Management API enhancement.

Documentation

Impacts on User's Session Management API Definition - v1

Training

Link to the PR for changes to the training content in https://github.com/wso2/WSO2-Training, if applicable

Certification

N/A

Marketing

N/A

Automation tests

Unit tests

N/A
Integration tests

N/A

Security checks

Followed secure coding standards in http://wso2.com/technical-reports/wso2-secure-engineering-guidelines? yes
Ran FindSecurityBugs plugin and verified report? yes
Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets? yes

Samples

Sample Request

DELETE t/carbon.super/api/users/v1/sessions?filter=appName eq My Account
Host: localhost:9443
Authorization: Basic YWRtaW46YWRtaW4=

Related PRs

Session Delete Improvements carbon-identity-framework#3750

Migrations (if applicable)

N/A

Test environment

OS: macOS Big Sur
JDK version: OpenJDK 1.8.0_292
Databases: Embedded H2

Learning

N/A

…eFilter methods

CLAassistant · 2021-09-27T06:20:03Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ isurusamarasekara
❌ Isuru Samarasekara

Isuru Samarasekara seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

somindatommy · 2022-06-24T03:38:00Z

...on.identity.api.user.common/src/main/java/org/wso2/carbon/identity/api/user/common/Util.java

+    /**
+     * Validate whether the given filter is not empty and tenantDomain is valid.
+     *
+     * @param filter    filter to be applied for session termination
+     * @param tenantDomain  tenant domain of the requester
+     */


Shall we fix the formatting errors here?

Param descriptions are not aligned.

Start sentences with uppercase letter and end with a .

somindatommy · 2022-06-24T03:38:20Z

...on.identity.api.user.common/src/main/java/org/wso2/carbon/identity/api/user/common/Util.java

+     * @param filter    filter to be applied for session termination
+     * @param tenantDomain  tenant domain of the requester
+     */
+    public static void  validateFilter(String filter, String tenantDomain) {


Extra space before the method name

somindatommy · 2022-06-24T03:39:26Z

...in/java/org/wso2/carbon/identity/rest/api/user/session/v1/core/SessionManagementService.java

+     * @param filter    the filter based on which the sessions to be terminated are selected (Mandatory)
+     * @param limit maximum number of sessions to be selected (Optional)
+     * @param since timestamp data value that points to the start of the range of data to be returned (Optional)
+     * @param until timestamp data value that points to the end of the range of data to be returned (Optional)
+     */


Shall we fix formatting here as well?

somindatommy · 2022-06-24T03:40:31Z

...entity.api.user.common/src/main/java/org/wso2/carbon/identity/api/user/common/Constants.java

@@ -57,7 +57,10 @@ public enum ErrorMessage {
                "The provided userId is invalid."),
        ERROR_CODE_SERVER_ERROR(USER_MANAGEMENT_PREFIX.getPrefix() + "15001",
                "Unable to retrieve User.",
-                "Server Encountered an error while retrieving the user.");
+                "Server Encountered an error while retrieving the user."),
+        INVALID_TENANT_DOMAIN(USER_MANAGEMENT_PREFIX.getPrefix() + "10002",


Seems like this is a server error. Shall we start the error code with 150xx instead of 100xx? We use 100xx for client errors.

INVALID_TENANT_DOMAIN is a client error IMO... as the client could change the context tenantDomain to a non-existing one.

somindatommy · 2022-06-24T03:41:42Z

...entity.api.user.common/src/main/java/org/wso2/carbon/identity/api/user/common/Constants.java

+                "Server Encountered an error while retrieving the user."),
+        INVALID_TENANT_DOMAIN(USER_MANAGEMENT_PREFIX.getPrefix() + "10002",
+                "Invalid tenant domain.",
+                "Server Encountered an error while retrieving tenantId for tenantDomain.");


Can we add more context to this? Add the tenant domain at the end? WDYT?

implemented

senthalan · 2022-07-28T11:27:05Z

...r.session.v1/src/gen/java/org/wso2/carbon/identity/rest/api/user/session/v1/SessionsApi.java

-import javax.ws.rs.Path;
-import javax.ws.rs.Produces;
-import javax.ws.rs.QueryParam;
+import javax.ws.rs.*;


Let's not use * imports.

This is a auto generated file. So this should be fine.

lashinijay · 2022-08-29T03:38:23Z

...on.identity.api.user.common/src/main/java/org/wso2/carbon/identity/api/user/common/Util.java

@@ -36,8 +37,8 @@
 import javax.ws.rs.core.Response;

 import static org.wso2.carbon.identity.api.user.common.Constants.CORRELATION_ID_MDC;
-import static org.wso2.carbon.identity.api.user.common.Constants.ErrorMessage.ERROR_CODE_INVALID_USERNAME;
-import static org.wso2.carbon.identity.api.user.common.Constants.ErrorMessage.ERROR_CODE_SERVER_ERROR;
+import static org.wso2.carbon.identity.api.user.common.Constants.ErrorMessage.*;


Shalll we avoid * imports?

somindatommy · 2023-02-09T10:23:45Z

@isurusamarasekara can you address the comments?

Add terminateFilteredSessions to user session mgt service and validat…

49b1393

…eFilter methods

isurusamarasekara changed the base branch from master to session-api-temp September 27, 2021 06:20

somindatommy reviewed Jun 24, 2022

View reviewed changes

senthalan reviewed Jul 28, 2022

View reviewed changes

lashinijay reviewed Aug 29, 2022

View reviewed changes

isurusamarasekara added 2 commits February 22, 2023 14:03

address PR comments

a23f88f

address PR comments -> add tenant domain to error description

c424751

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Session Delete Improvements #135

Session Delete Improvements #135

isurusamarasekara commented Sep 27, 2021

CLAassistant commented Sep 27, 2021 •

edited

Loading

somindatommy Jun 24, 2022

isurusamarasekara Feb 22, 2023

somindatommy Jun 24, 2022

isurusamarasekara Feb 22, 2023

somindatommy Jun 24, 2022

isurusamarasekara Feb 22, 2023

somindatommy Jun 24, 2022

isurusamarasekara Feb 22, 2023

somindatommy Jun 24, 2022

isurusamarasekara Feb 22, 2023

senthalan Jul 28, 2022

madurangasiriwardena Feb 13, 2023

isurusamarasekara Feb 22, 2023

lashinijay Aug 29, 2022

isurusamarasekara Feb 22, 2023

somindatommy commented Feb 9, 2023

Session Delete Improvements #135

Are you sure you want to change the base?

Session Delete Improvements #135

Conversation

isurusamarasekara commented Sep 27, 2021

Purpose

Goals

Approach

Service Specification

Sessions API

User stories

Release note

Documentation

Training

Certification

Marketing

Automation tests

Security checks

Samples

Related PRs

Migrations (if applicable)

Test environment

Learning

CLAassistant commented Sep 27, 2021 • edited Loading

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

somindatommy commented Feb 9, 2023

CLAassistant commented Sep 27, 2021 •

edited

Loading