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

Implement connection.update-secret to support rabbit_auth_backend_oauth2 #1160

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kratkyzobak
Copy link

Implementation of "simple" part of #1159

This PR implements method connection.update-secret and exposes it to library consumers as *Connection::updatePassword($password);. Naming were chosen this way as it is just updating previously set $password in constructor. Method also updates internal password properties of Connection classes. This enables to reconnect and/or support lazy connections.

@kratkyzobak kratkyzobak force-pushed the oauth2-update-secret branch 3 times, most recently from 1fc6787 to 950989c Compare February 9, 2024 12:55
Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (109b13d) 74.10% compared to head (6e1376a) 74.24%.
Report is 2 commits behind head on master.

Files Patch % Lines
PhpAmqpLib/Connection/AbstractConnection.php 90.90% 2 Missing ⚠️
PhpAmqpLib/Wire/IO/StreamIO.php 84.61% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1160      +/-   ##
============================================
+ Coverage     74.10%   74.24%   +0.13%     
- Complexity     1042     1051       +9     
============================================
  Files            39       39              
  Lines          3136     3168      +32     
============================================
+ Hits           2324     2352      +28     
- Misses          812      816       +4     

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

@@ -36,6 +36,7 @@
},
"require-dev": {
"ext-curl": "*",
"ext-openssl": "*",
Copy link
Member

Choose a reason for hiding this comment

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

Library itself does not need this extension, only tests.

Copy link
Author

Choose a reason for hiding this comment

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

Hence require-dev.

*/
public static function connectionUpdateSecretOk(AMQPReader $reader)
{
$response = array();
Copy link
Member

Choose a reason for hiding this comment

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

Why not just return []; ?

Copy link
Author

Choose a reason for hiding this comment

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

Method is generated by your code.

}

// for potential cloning
$this->replace_password_in_construct_params($password);
Copy link
Member

Choose a reason for hiding this comment

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

Constructor params are deprecated and will be dropped in version 4.0

Copy link
Author

Choose a reason for hiding this comment

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

When will version 4.0 released? Will version 4.0 handle OAuth?

/**
* @param string $password
*/
protected function replace_password_in_construct_params($password)
Copy link
Member

Choose a reason for hiding this comment

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

pointless method

define('JWT_TOKEN_1', getenv('TEST_RABBITMQ_JWT_TOKEN_1') ? getenv('TEST_RABBITMQ_JWT_TOKEN_1') : generateJwtToken(7200));
define('JWT_TOKEN_2', getenv('TEST_RABBITMQ_JWT_TOKEN_2') ? getenv('TEST_RABBITMQ_JWT_TOKEN_2') : generateJwtToken(7205));

function generateJwtToken($expire = 7200)
Copy link
Member

Choose a reason for hiding this comment

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

this should be part of test, abstract test class or something "on demand" but not defined as global constats, whats the point?

* Should be used for RabbitMQ's OAuth2 authentication backend to update current token.
* @param string $password
*/
public function updatePassword($password): void
Copy link
Member

Choose a reason for hiding this comment

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

missing type hint

@ramunasd
Copy link
Member

ramunasd commented Feb 9, 2024

Next minor version is only for bugfixes and deprecations. Afterwards we will refactor many things related to connections. So we can't accept this for 3.x series. At best for 4.0.

#1123

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

2 participants