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

bridge: Fix credential reset after unauthorized sign-in #454

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

jkralik
Copy link
Member

@jkralik jkralik commented Mar 27, 2024

Summary by CodeRabbit

  • New Features
    • Introduced new configuration options for device cloud connection management, including automatic token refresh and reconnection capabilities.
    • Added customizable tick intervals for more flexible device cloud interactions.
    • Enhanced support for handling unsupported request codes in device-cloud communication.
  • Improvements
    • Improved credential updating process with a more flexible approach.
    • Enhanced sign-in logic to handle unauthorized responses more effectively.
  • Bug Fixes
    • Fixed an issue with setting the ValidUntil field during device sign-up.
  • Tests
    • Added new test functions for verifying device authorization and custom COAP gateway handlers.

Copy link

coderabbitai bot commented Mar 27, 2024

Walkthrough

This update introduces enhancements and new features to the device-cloud bridge management, focusing on connection resilience and configuration flexibility. Key additions include configurable reconnection logic, token refreshment on demand, and dynamic tick interval settings. It also brings more customizable handling for device-cloud interactions and error responses in the CoAP gateway tests, aiming to improve reliability and adaptability in device communication with cloud services.

Changes

Files Change Summaries
.../cloud/manager.go Introduced forceRefreshToken, reconnect flags, dynamic tickInterval, enhanced token refresh logic, and reconnection handling. Added Reconnect method. Updated serveCOAP for unsupported requests and connection logging.
.../cloud/options.go Added tickInterval field to OptionsCfg.
.../cloud/refreshToken.go Modified credential update mechanism in refreshToken function for flexibility.
.../cloud/signIn.go, .../cloud/signUp.go Adjusted sign-in for forced token refresh on unauthorized response; modified sign-up response handling.
.../device.go, .../options.go Enhanced device initialization with cloud options configurability.
test/coap-gateway/... Implemented custom handler for CoAP operations, updated error handling in client.go, introduced delay and additional imports in refreshToken.go. Added new test function for unauthorized device state.

🐰✨🌟

In fields of code where logic threads weave,
A rabbit hopped, leaving changes to perceive.
With tokens refreshed and connections keen,
Through dynamic intervals, a resilient scene.
Celebrate we do, with carrots and gleam,
For adaptability in our digital dream.
🌈🚀🐾

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between af7fc65 and d849502.
Files selected for processing (2)
  • bridge/device/cloud/manager.go (3 hunks)
  • bridge/device/cloud/signIn.go (1 hunks)
Additional comments: 4
bridge/device/cloud/signIn.go (1)
  • 74-79: The logic for handling unauthorized sign-in attempts and setting forceRefreshToken based on the presence of a RefreshToken is correctly implemented.

Ensure that forceRefreshToken is reset appropriately after it's used to avoid unintended behavior in subsequent operations.

bridge/device/cloud/manager.go (3)
  • 99-99: The addition of forceRefreshToken to the Manager struct aligns with the objectives of improving the handling of unauthorized sign-in attempts.
  • 249-249: Resetting forceRefreshToken in resetCredentials is a good practice to ensure it does not affect subsequent operations unintentionally.
  • 508-510: Conditionally refreshing the token in connect based on forceRefreshToken is a good practice to only refresh the token when necessary.

@jkralik jkralik force-pushed the jkralik/fix/bridge-unregistering branch from d849502 to 8cfadb0 Compare March 27, 2024 10:56
@jkralik jkralik changed the title bridge: fix resetting credentials after get unauthorized code for sign-in bridge: Fix credential reset after unauthorized sign-in Mar 27, 2024
@Danielius1922 Danielius1922 self-requested a review March 27, 2024 10:58
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between af7fc65 and 8cfadb0.
Files selected for processing (9)
  • bridge/device/cloud/manager.go (15 hunks)
  • bridge/device/cloud/manager_test.go (3 hunks)
  • bridge/device/cloud/options.go (3 hunks)
  • bridge/device/cloud/signIn.go (1 hunks)
  • bridge/device/device.go (1 hunks)
  • bridge/device/options.go (2 hunks)
  • test/coap-gateway/customHandler.go (1 hunks)
  • test/coap-gateway/service/client.go (2 hunks)
  • test/coap-gateway/service/refreshToken.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • bridge/device/cloud/signIn.go
Additional comments: 16
bridge/device/cloud/options.go (2)
  • 32-32: The addition of the tickInterval field to the OptionsCfg struct is appropriate and enhances the configuration capabilities of the cloud component.
  • 63-67: The WithTickInterval function is correctly implemented and follows the established pattern for option-setting functions in the OptionsCfg struct.
test/coap-gateway/service/refreshToken.go (2)
  • 23-23: The import of the time package is necessary for the introduced delay and is correctly placed.
  • 36-36: Introducing a 10-millisecond delay before closing the client connection is a common practice to ensure pending operations complete. However, it's important to verify the necessity and appropriateness of this delay for the specific context of the refreshTokenPostHandler function.
bridge/device/options.go (2)
  • 45-45: The addition of the cloudOptions field to the OptionsCfg struct allows for flexible and modular configuration of cloud-related options, following the pattern of using functional options in Go.
  • 87-91: The WithCloudOptions function is correctly implemented, following the established pattern for option-setting functions in the OptionsCfg struct and allowing for flexible configuration of cloud-related options.
test/coap-gateway/service/client.go (2)
  • 30-30: The import of the coapStatus package is necessary for the enhancements in error handling and is correctly placed.
  • 121-127: The modifications to the sendErrorResponse function, including the introduction of a defaultCode parameter and conditional override based on the error, improve its flexibility and accuracy in error handling.
test/coap-gateway/customHandler.go (1)
  • 1-133: The implementation of the CustomHandler struct and its methods, including the use of atomic pointers for function types, is correct and follows best practices for concurrency in Go. This approach allows for dynamic and thread-safe updates to handler functions.
bridge/device/device.go (1)
  • 163-163: Appending o.cloudOptions to cloudOpts before initializing the cloud manager enhances flexibility and configurability. This change is correctly implemented using the append function in Go.
bridge/device/cloud/manager_test.go (3)
  • 30-30: The imports for the ocfCloud and status packages are necessary for the introduced test functionalities, facilitating the simulation and verification of unauthorized device behavior and handling of CoAP status codes.

Also applies to: 47-47

  • 74-78: The implementation of the getUnauthorizedError function is correct, serving its purpose of generating a standardized unauthorized error for testing using CoAP status codes.
  • 81-144: The TestManagerDeviceBecomesUnauthorized function is well-structured and effectively tests the device's behavior when it becomes unauthorized, using the getUnauthorizedError function to simulate unauthorized errors.
bridge/device/cloud/manager.go (3)
  • 102-102: The addition of the reconnect field using atomic.Bool to the Manager struct is appropriate for thread-safe operations, enhancing the capability to handle reconnection logic.
  • 193-199: The modifications to the handleTrigger method to include reconnection logic based on the reconnect flag are correctly implemented, improving the resilience and responsiveness of the cloud connection process.
  • 622-625: The introduction of the Reconnect method, which sets the reconnect flag and triggers the reconnection process, is a useful and correctly implemented addition, enhancing the cloud manager's flexibility.

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 82.19178% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 76.97%. Comparing base (af7fc65) to head (1ab6318).

Files Patch % Lines
bridge/device/cloud/manager.go 86.44% 6 Missing and 2 partials ⚠️
bridge/device/cloud/signIn.go 28.57% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #454      +/-   ##
==========================================
+ Coverage   76.87%   76.97%   +0.10%     
==========================================
  Files         115      115              
  Lines        6710     6750      +40     
==========================================
+ Hits         5158     5196      +38     
+ Misses       1013     1011       -2     
- Partials      539      543       +4     

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

@jkralik jkralik force-pushed the jkralik/fix/bridge-unregistering branch from 8cfadb0 to f7aa005 Compare March 27, 2024 11:44
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between af7fc65 and f7aa005.
Files selected for processing (9)
  • bridge/device/cloud/manager.go (16 hunks)
  • bridge/device/cloud/manager_test.go (3 hunks)
  • bridge/device/cloud/options.go (3 hunks)
  • bridge/device/cloud/signIn.go (1 hunks)
  • bridge/device/device.go (1 hunks)
  • bridge/device/options.go (2 hunks)
  • test/coap-gateway/customHandler.go (1 hunks)
  • test/coap-gateway/service/client.go (2 hunks)
  • test/coap-gateway/service/refreshToken.go (2 hunks)
Files skipped from review as they are similar to previous changes (9)
  • bridge/device/cloud/manager.go
  • bridge/device/cloud/manager_test.go
  • bridge/device/cloud/options.go
  • bridge/device/cloud/signIn.go
  • bridge/device/device.go
  • bridge/device/options.go
  • test/coap-gateway/customHandler.go
  • test/coap-gateway/service/client.go
  • test/coap-gateway/service/refreshToken.go

@jkralik jkralik force-pushed the jkralik/fix/bridge-unregistering branch from f7aa005 to 1ab6318 Compare March 27, 2024 11:48
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between af7fc65 and 1ab6318.
Files selected for processing (9)
  • bridge/device/cloud/manager.go (16 hunks)
  • bridge/device/cloud/manager_test.go (3 hunks)
  • bridge/device/cloud/options.go (3 hunks)
  • bridge/device/cloud/signIn.go (1 hunks)
  • bridge/device/device.go (1 hunks)
  • bridge/device/options.go (2 hunks)
  • test/coap-gateway/customHandler.go (1 hunks)
  • test/coap-gateway/service/client.go (2 hunks)
  • test/coap-gateway/service/refreshToken.go (2 hunks)
Files skipped from review as they are similar to previous changes (9)
  • bridge/device/cloud/manager.go
  • bridge/device/cloud/manager_test.go
  • bridge/device/cloud/options.go
  • bridge/device/cloud/signIn.go
  • bridge/device/device.go
  • bridge/device/options.go
  • test/coap-gateway/customHandler.go
  • test/coap-gateway/service/client.go
  • test/coap-gateway/service/refreshToken.go

@jkralik jkralik force-pushed the jkralik/fix/bridge-unregistering branch from 1ab6318 to 40462fa Compare March 27, 2024 16:31
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between af7fc65 and 40462fa.
Files selected for processing (11)
  • bridge/device/cloud/manager.go (14 hunks)
  • bridge/device/cloud/manager_test.go (3 hunks)
  • bridge/device/cloud/options.go (3 hunks)
  • bridge/device/cloud/refreshToken.go (1 hunks)
  • bridge/device/cloud/signIn.go (2 hunks)
  • bridge/device/cloud/signUp.go (1 hunks)
  • bridge/device/device.go (1 hunks)
  • bridge/device/options.go (2 hunks)
  • test/coap-gateway/customHandler.go (1 hunks)
  • test/coap-gateway/service/client.go (2 hunks)
  • test/coap-gateway/service/refreshToken.go (2 hunks)
Files skipped from review as they are similar to previous changes (9)
  • bridge/device/cloud/manager.go
  • bridge/device/cloud/manager_test.go
  • bridge/device/cloud/options.go
  • bridge/device/cloud/signIn.go
  • bridge/device/device.go
  • bridge/device/options.go
  • test/coap-gateway/customHandler.go
  • test/coap-gateway/service/client.go
  • test/coap-gateway/service/refreshToken.go
Additional comments: 2
bridge/device/cloud/refreshToken.go (1)
  • 72-76: The changes in the updateCredsByRefreshTokenResponse method introduce a more flexible way of updating credentials, which aligns well with the PR objectives. However, setting the signedIn flag to false is a significant change. It would be beneficial to add a comment explaining the rationale behind this decision to aid future maintainability.
bridge/device/cloud/signUp.go (1)
  • 83-83: The simplification of the ValidUntil field assignment in the signUp function improves code readability and maintainability. This change aligns well with the PR objectives of enhancing system resilience and user experience by ensuring efficient token management.

@jkralik jkralik force-pushed the jkralik/fix/bridge-unregistering branch from 40462fa to e47a6ed Compare March 27, 2024 16:37
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between af7fc65 and e47a6ed.
Files selected for processing (11)
  • bridge/device/cloud/manager.go (14 hunks)
  • bridge/device/cloud/manager_test.go (3 hunks)
  • bridge/device/cloud/options.go (3 hunks)
  • bridge/device/cloud/refreshToken.go (1 hunks)
  • bridge/device/cloud/signIn.go (2 hunks)
  • bridge/device/cloud/signUp.go (1 hunks)
  • bridge/device/device.go (1 hunks)
  • bridge/device/options.go (2 hunks)
  • test/coap-gateway/customHandler.go (1 hunks)
  • test/coap-gateway/service/client.go (2 hunks)
  • test/coap-gateway/service/refreshToken.go (2 hunks)
Files skipped from review as they are similar to previous changes (11)
  • bridge/device/cloud/manager.go
  • bridge/device/cloud/manager_test.go
  • bridge/device/cloud/options.go
  • bridge/device/cloud/refreshToken.go
  • bridge/device/cloud/signIn.go
  • bridge/device/cloud/signUp.go
  • bridge/device/device.go
  • bridge/device/options.go
  • test/coap-gateway/customHandler.go
  • test/coap-gateway/service/client.go
  • test/coap-gateway/service/refreshToken.go

Copy link

sonarcloud bot commented Mar 27, 2024

@jkralik jkralik merged commit 690a44f into main Mar 27, 2024
14 checks passed
@jkralik jkralik deleted the jkralik/fix/bridge-unregistering branch March 27, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants