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

Token authentication #4

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

Conversation

AskarZinurov
Copy link

Introduce modern authorization mechanism instead of deprecated one.

Copy link

@Vasfed Vasfed left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

def build_token
ts = Time.now.to_i
payload = { iss: account_id, iat: ts - 5, exp: ts + 64 }
JWT.encode(payload,
Copy link

Choose a reason for hiding this comment

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

JWT gem is not in dependencies, this may break if one does not have it already

@@ -51,6 +54,15 @@ def method_missing(name, *args)

protected

def build_token
ts = Time.now.to_i
payload = { iss: account_id, iat: ts - 5, exp: ts + 64 }
Copy link

Choose a reason for hiding this comment

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

Please provide more info on why we need "issued at" field in past

Copy link
Author

@AskarZinurov AskarZinurov Apr 4, 2023

Choose a reason for hiding this comment

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

I have just copied behaviour from their own python client

Comment on lines 20 to +21
@api_key = options[:api_key]
@private_key = options[:private_key]
Copy link

Choose a reason for hiding this comment

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

Probably here should be a check that only one auth method is enabled at a time

Copy link
Author

Choose a reason for hiding this comment

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

Technically, client can use both kind of auth methods at same time. If so, the decision of is request authenticated or not will be completely on api provider side. We can perform validation for at least one auth method provided during client initialization, WDYT?

Copy link

Choose a reason for hiding this comment

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

"at least one" sounds good

@AskarZinurov AskarZinurov requested a review from Vasfed April 4, 2023 17:44
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.

2 participants