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

feat: use force_revocable tokens in 7.50.3+ #45

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

TJM
Copy link
Contributor

@TJM TJM commented Mar 7, 2023

This enables setting the force_revocable flag and set and expires_in to MaxTLL

Screen Shot 2023-03-07 at 2 45 03 PM

Fixes #40

@TJM TJM requested a review from alexhung as a code owner March 7, 2023 22:02
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.

LGTM. I have 2 minor nitpicks.

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

@TJM TJM left a comment

Choose a reason for hiding this comment

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

This works, as is, but I think we could design the "version check" stuff better. I'd like to make it so that the version is detected once at config time, then maybe during "reload" ? Then we could switch the "checks" to just compare the version from config.

artifactory.go Outdated Show resolved Hide resolved
@TJM TJM force-pushed the feat/force-revokable branch 2 times, most recently from b8dd8a4 to d200fbc Compare March 8, 2023 18:05
@TJM
Copy link
Contributor Author

TJM commented Mar 8, 2023

Fixed spelling on revocable ... how do we get from "revoke" to "revocable" (inflammable means flammable, what a country) ;)

@TJM
Copy link
Contributor Author

TJM commented Mar 8, 2023

Sorry, I just noticed my "Makefile" fix for TOKEN_USERNAME on the local test artifactory snuck in while I was fixing the spelling. That only affects a local test environment, so I am going to leave it there unless there are objections.

alexhung
alexhung previously approved these changes Mar 8, 2023
@alexhung alexhung requested a review from shrajfr12 March 8, 2023 19:16
@TJM TJM changed the title feat: use force_revocable tokens in 7.50.3+ Draft: feat: use force_revocable tokens in 7.50.3+ Mar 8, 2023
@TJM
Copy link
Contributor Author

TJM commented Mar 8, 2023

@alexhung While doing the UserTemplate stuff, I think I might have made an error around the TTL. I assume that the role maxTTL was the number to use, but now I see there is more TTL related logic in the func (b *backend) pathTokenCreatePerform function. I am going to have to re-work this. I am not sure which way I will move the logic yet. I am leaning towards passing the expires_in as an argument (optional argument) to the createToken function.

EDIT: I think the easiest way to handle this will be to modify the role.MaxTTL that gets sent to the createToken function, in the event that there is a system level setting limiting maxTTL. That way we don't have to change the "interface" to createToken, and we don't have to mess with trying to move any logic around or parse/return any TTL values.

EDIT: FIXED, as per above, just set role.MaxTTL instead of keeping that as a local variable, which makes the original code still work, but respect the system level MaxLeaseTTL

@TJM TJM changed the title Draft: feat: use force_revocable tokens in 7.50.3+ feat: use force_revocable tokens in 7.50.3+ Mar 8, 2023
path_token_create.go Outdated Show resolved Hide resolved
@TJM
Copy link
Contributor Author

TJM commented Mar 10, 2023

UPDATE: Rebased (one commit) and pulled out the magic version stuff... as I found it it fails after a reload. I need to sort out initialize, which I will do as part of a separate MR. Let's keep this one simple.

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 72edf5d into jfrog:master Mar 13, 2023
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.

Set token expiration and force_revokable from Artifactory 7.50.3
3 participants