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

Adding authorization functionality to increase request limit #19

Merged
merged 2 commits into from
Feb 8, 2021

Conversation

orchestr7
Copy link
Owner

What's done:

  1. New diktat version
  2. Authorization functionality
  3. Refactoring

### What's done:
1. New diktat version
2. Authorization functionality
3. Refactoring
@orchestr7 orchestr7 requested a review from petertrr February 7, 2021 21:27
ArgType.String,
shortName = "a",
description = """GitHub username and authorization token separated by ':'
| (can be used to extend the number of requests)""".trimMargin()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, provide some info about token requirements (scopes, permissions)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

don't get what you mean

Copy link
Collaborator

Choose a reason for hiding this comment

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

Github tokens can have different scopes and give token bearers different permission (read only or write). You should specify required settings for token for your app.

Copy link
Owner Author

@orchestr7 orchestr7 Feb 8, 2021

Choose a reason for hiding this comment

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

we don't need any permissions, as we expect to access only open public repositories
token here is only to have a bigger limit of attempts

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, but if I want to use git-stat with token and I open token creation page what should I choose? If I generate token with minimum permissions, will it be enough, or will I have to regenerate it again? I'd feel confused in this situation.

src/main/kotlin/org/akuleshov7/utils/Config.kt Outdated Show resolved Hide resolved
auth.let {
val splitAuth = auth.split(":")
if (splitAuth.size != 2) {
"""Incorrect value ($auth) is passed to 'auth' property.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you should log it here - user sees the command they just entered, and logging secret token might be considered a security risk.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Completely agree

src/main/kotlin/org/akuleshov7/utils/Credentials.kt Outdated Show resolved Hide resolved
@@ -10,6 +10,7 @@ import kotlin.system.exitProcess

/**
* Yes, it is yet another logger, because the quality and functionality of the current loggers is extremely poor
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really see a reason for this: for JVM projects you still can use kotlin-logging as a facade and logback/log4j as an implementation.

Copy link
Owner Author

@orchestr7 orchestr7 Feb 8, 2021

Choose a reason for hiding this comment

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

will make it later, but I do not think that it is a good idea actually

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I don't insist and for a small CLI tool it's probably fine to have a small custom logger, I'm just not quite convinced of advantages over existing solutions.

@orchestr7 orchestr7 merged commit e7d2aa9 into main Feb 8, 2021
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