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

Remove isUserLoggedIn check in TidalApi #182

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

ziad-halabi9
Copy link
Contributor

Currently the RequestHelper checks for a few conditions before making requests. One of the check is whether the user is logged in. It throws an error if that's not the case. This limits the TidalAPI to be only used when the user is authenticated.

This prevents its usage with Client Credentials auth flow where the client is authenticated and capable to make API requests that don't require user authentication. This PR removes the check of isUserLoggedIn == true.

This limits the TidalAPI to be only used when the user is authenticated. This prevents its usage with Client Credentials flow where the client is authenticated and capable to make API requests that don't require user authentication.
@ziad-halabi9 ziad-halabi9 added the bug Something isn't working label Jan 9, 2025
@ziad-halabi9 ziad-halabi9 requested a review from a team as a code owner January 9, 2025 13:43
Copy link
Collaborator

@asendra asendra left a comment

Choose a reason for hiding this comment

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

Minor comment, Your change I think it is fine

@@ -15,10 +15,7 @@ enum RequestHelper {
let credentials = try await credentialsProvider.getCredentials()
let requestBuilder = try await requestBuilder()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ultra-minor thing.

let requestBuilder = try await requestBuilder()

This line could be after the token check. No need for it before, so it would be a bit more optimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good catch, will follow up in another PR for this 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it is before the check because requestURL is used in the error thrown.

@ziad-halabi9 ziad-halabi9 added this pull request to the merge queue Jan 10, 2025
Merged via the queue into main with commit 06637c7 Jan 10, 2025
5 checks passed
@ziad-halabi9 ziad-halabi9 deleted the ziad/remove-user-login-check-tidal-api branch January 10, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants