-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[#53368] add authentication for storage interaction #14954
[#53368] add authentication for storage interaction #14954
Conversation
# - :connected If a valid user token is available | ||
# - :failed_authorization If a user token is available, which is invalid and not refreshable | ||
# - :error If an unexpected error occurred | ||
def self.authorization_state(storage:, user:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore this method, I simply didn't want to cut this out into a separate commit. It is needed later, when authorization state is extracted from the connection manager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see you have the usage be somewhat like so:
CreateTemplateFolderCommand.call(auth: Strategy.new(:oauth_client_credentials), storage: ....)
# or on the inside of an object
class RmRfCommand
def call(strategy: :basic_auth, storage:, location:)
Authentication[strategy].call do |authed_http_session|
do_awesome_requests
end
end
end
...les/storages/app/common/storages/peripherals/oauth_configurations/nextcloud_configuration.rb
Outdated
Show resolved
Hide resolved
...rages/app/common/storages/peripherals/storage_interaction/authentication_strategies/error.rb
Outdated
Show resolved
Hide resolved
...rages/peripherals/storage_interaction/authentication_strategies/o_auth_client_credentials.rb
Outdated
Show resolved
Hide resolved
...rages/peripherals/storage_interaction/authentication_strategies/o_auth_client_credentials.rb
Outdated
Show resolved
Hide resolved
...n/storages/peripherals/storage_interaction/authentication_strategies/o_auth_configuration.rb
Outdated
Show resolved
Hide resolved
...mmon/storages/peripherals/storage_interaction/authentication_strategies/o_auth_user_token.rb
Outdated
Show resolved
Hide resolved
...mmon/storages/peripherals/storage_interaction/authentication_strategies/o_auth_user_token.rb
Outdated
Show resolved
Hide resolved
context 'if a timeout happens' do | ||
before do | ||
request = HTTPX::Request.new(:get, request_url) | ||
httpx_double = class_double(HTTPX, get: HTTPX::ErrorResponse.new(request, 'Timeout happens', {})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 You can actually mock a timeout with WebMock
stub_request(:get, 'https://timeout-as-a-service.com/').to_timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, that spec was part of the query tests before, hence webmock was no option (interfering with vcr).
I can change that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I will refactor that test with the first query that is moved to new auth strategies. Right now, I don't see a good sense in this whole test :/
I wanted to cover the behaviour of httpx to return network errors in a different object. But the handling of it happens inside the queries. So, I would suggest to add a network unit test with webmock, that runs on top of an example query, which is considered a placeholder for all other queries, too.
- fixed unit tests for storage authentication - fixed client credentials strategy
2afdf60
to
4f96d4a
Compare
- use only class, not instance in error payload
4f96d4a
to
bf8882c
Compare
#53368
WAT?