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.

Sign up for GitHub

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

Jump to bottom

feat: more token information on config read #39

Merged
merged 2 commits into from
Feb 23, 2023
Merged

feat: more token information on config read #39

merged 2 commits into from
Feb 23, 2023

Conversation

TJM
Copy link
Contributor

@TJM TJM commented Feb 22, 2023 •
edited
Loading

Show more useful token information when reading config. SHA256 is not useful after rotating token, show username, token_id and scope.

Example

$ vault read artifactory/config/admin
Key                    Value
---                    -----
access_token_sha256    def0def11280a56e538876459fe5bbef1ad15cb5acd25251aa57eccb8eb4eb34
jfrog_token_id         d1edc4a9-c3c9-4ff7-82f2-d0d24b44780a
scope                  applied-permissions/admin
url                    https://artifactory.company.com
username               tommy-test-vault

Fixes #36

@TJM TJM requested a review from alexhung as a code owner February 22, 2023 22:28
TJM
TJM commented Feb 22, 2023
View reviewed changes
path_config.go
@@ -118,6 +120,16 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, _ *f
"url": config.ArtifactoryURL,
}

// Optionally include token info if it parses properly
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I did this was to not have to mess with the unit tests. When I made the parsing required (i.e.

if err != nil {
  return nil, err
}

... the unit test failed because its bogus artifactory URL and token obviously won't parse correctly. (fails to detect the artifactory version)

alexhung
alexhung reviewed Feb 22, 2023
View reviewed changes
Copy link
Member

@alexhung alexhung left a comment

Choose a reason for hiding this comment

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

Look good to me, except the extra layer of function abstraction.

artifactory.go Outdated Show resolved Hide resolved
@TJM
Copy link
Contributor Author

TJM commented Feb 23, 2023

LGTM? :)

@alexhung
Copy link
Member

LGTM = Looks Good To Me 😄

@alexhung
Copy link
Member

Waiting for @shrajfr12 review

shrajfr12
shrajfr12 approved these changes Feb 23, 2023
View reviewed changes
Copy link

@shrajfr12 shrajfr12 left a comment

Choose a reason for hiding this comment

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

lgtm

@alexhung alexhung merged commit df74973 into jfrog:master Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shrajfr12 shrajfr12 shrajfr12 approved these changes

@alexhung alexhung alexhung approved these changes

@MahithaB MahithaB Awaiting requested review from MahithaB

@mritunjaykumar mritunjaykumar Awaiting requested review from mritunjaykumar

@Turhsus Turhsus Awaiting requested review from Turhsus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show token ID (maybe other details) for admin token
3 participants
@TJM @alexhung @shrajfr12