Fix the issue https://github.com/wso2/product-is/issues/21192 #2587

DMHP · 2024-10-07T09:11:31Z

Proposed changes in this pull request

Fix Inconsistent Error Responses in Device Flow

When should this PR be merged

Immediately

Follow-up actions

Add an integration test to cover this flow
Add unit tests

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.

codecov · 2024-10-07T09:26:36Z

Codecov Report

Attention: Patch coverage is 46.66667% with 24 lines in your changes missing coverage. Please review.

Project coverage is 53.88%. Comparing base (61494ba) to head (0e81592).
Report is 70 commits behind head on master.

Files with missing lines	Patch %	Lines
.../identity/oauth2/device/dao/DeviceFlowDAOImpl.java	0.00%	12 Missing ⚠️
.../identity/oauth2/device/grant/DeviceFlowGrant.java	63.63%	9 Missing and 3 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #2587       +/-   ##
=============================================
+ Coverage     41.99%   53.88%   +11.89%     
- Complexity     6005     8504     +2499     
=============================================
  Files           601      632       +31     
  Lines         44553    50603     +6050     
  Branches       7010     8533     +1523     
=============================================
+ Hits          18710    27268     +8558     
+ Misses        22382    19222     -3160     
- Partials       3461     4113      +652

Flag	Coverage Δ
unit	`38.08% <46.66%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thumimku · 2024-10-11T06:17:42Z

...entity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/device/dao/DeviceFlowDAOImpl.java

@@ -53,6 +54,8 @@
 import java.util.TimeZone;
 import java.util.UUID;

+import static org.wso2.carbon.identity.oauth2.device.constants.Constants.SLOW_DOWN;


We already imported

org.wso2.carbon.identity.oauth2.device.constants.Constants

in line 36.

We don't need to import this static constant, we can directly use

Constants.SLOW_DOWN

...entity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/device/grant/DeviceFlowGrant.java

Thumimku · 2024-10-11T06:58:38Z

...entity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/device/grant/DeviceFlowGrant.java

+            deviceFlowDO.setDeviceCode(deviceCode);
+            setLastPollTime(deviceCode);
+        } catch (IdentityOAuth2Exception e) {
+            deviceStatus = e.getMessage();


we don't need this line right?

As we need to pass it here, https://github.com/wso2-extensions/identity-inbound-auth-oauth/pull/2587/files#diff-04c8d9d96bce4315cd10faf6cee77e67804ef26f765ac63eda3f36ab5767b945R80 we need to specify here.

this catch block don't get execute after line 76, cause handleInvalidRequests method throws identityOAuthException anyway.

...entity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/device/grant/DeviceFlowGrant.java

Thumimku

can improve the code with said comments, LGTM. Let's run integration tests for safe side.

DMHP · 2024-10-14T15:04:26Z

This PR contains a basic set of unit tests to cover the flow I added. However, this flow requires comprehensive unit tests to cover the whole flow. I will work on improving those.

shashimalcse · 2024-10-15T17:48:20Z

...ty.oauth/src/test/java/org/wso2/carbon/identity/oauth2/device/grant/DeviceFlowGrantTest.java

@@ -47,14 +61,19 @@ public class DeviceFlowGrantTest {
    private Timestamp newTime = new Timestamp(date.getTime());
    private DeviceFlowDO deviceFlowDO1 = new DeviceFlowDO();
    private DeviceFlowDO deviceFlowDO2 = new DeviceFlowDO();
+    private DeviceFlowGrant deviceFlowGrant;
+    private OAuthTokenReqMessageContext oAuthTokenReqMessageContext;
+    private DeviceFlowDO deviceFlowDO3;


cant we move near to other DOs

Fix the issue wso2/product-is#21192

0545870

DMHP mentioned this pull request Oct 7, 2024

Inconsistent Error Responses in Device Flow wso2/product-is#21192

Open

Thumimku reviewed Oct 11, 2024

View reviewed changes

DMHP force-pushed the device_code branch from 2ab53e2 to 659e2ab Compare October 11, 2024 06:53

Thumimku reviewed Oct 11, 2024

View reviewed changes

...entity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/device/grant/DeviceFlowGrant.java Outdated Show resolved Hide resolved

Thumimku previously approved these changes Oct 11, 2024

View reviewed changes

DMHP dismissed Thumimku’s stale review via 6e6e436 October 11, 2024 07:13

DMHP force-pushed the device_code branch 2 times, most recently from 6e6e436 to eac5a95 Compare October 11, 2024 07:29

Thumimku previously approved these changes Oct 11, 2024

View reviewed changes

DMHP dismissed Thumimku’s stale review via 71ca1ff October 14, 2024 14:25

DMHP force-pushed the device_code branch from eac5a95 to 71ca1ff Compare October 14, 2024 14:25

Resolving comments

0e81592

DMHP force-pushed the device_code branch from 71ca1ff to 0e81592 Compare October 14, 2024 15:02

shashimalcse reviewed Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the issue https://github.com/wso2/product-is/issues/21192 #2587

Fix the issue https://github.com/wso2/product-is/issues/21192 #2587

DMHP commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

Thumimku Oct 11, 2024

Thumimku Oct 11, 2024

DMHP Oct 11, 2024

Thumimku Oct 11, 2024

Thumimku left a comment

DMHP commented Oct 14, 2024

shashimalcse Oct 15, 2024

Fix the issue https://github.com/wso2/product-is/issues/21192 #2587

Are you sure you want to change the base?

Fix the issue https://github.com/wso2/product-is/issues/21192 #2587

Conversation

DMHP commented Oct 7, 2024 • edited Loading

Proposed changes in this pull request

When should this PR be merged

Follow-up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

Thumimku Oct 11, 2024

Choose a reason for hiding this comment

Thumimku Oct 11, 2024

Choose a reason for hiding this comment

DMHP Oct 11, 2024

Choose a reason for hiding this comment

Thumimku Oct 11, 2024

Choose a reason for hiding this comment

Thumimku left a comment

Choose a reason for hiding this comment

DMHP commented Oct 14, 2024

shashimalcse Oct 15, 2024

Choose a reason for hiding this comment

DMHP commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading