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

Create AuthorizedKeysCommand #5236

Merged
merged 2 commits into from Nov 1, 2018
Merged

Create AuthorizedKeysCommand #5236

merged 2 commits into from Nov 1, 2018

Conversation

zeripath
Copy link
Contributor

This pull-request adds a subcommand that can be used as an SSH AuthorizedKeysCommand.

The configuration is described in command-line.md.

techknowlogick
techknowlogick reviewed Oct 31, 2018
View reviewed changes
cmd/keys.go Outdated Show resolved Hide resolved
@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 31, 2018
@zeripath zeripath mentioned this pull request Oct 31, 2018
Merged
@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 31, 2018
techknowlogick
techknowlogick approved these changes Oct 31, 2018
View reviewed changes
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

One nit, Can you add setting to custom/conf/app.ini.sample

@zeripath
Copy link
Contributor Author

zeripath commented Oct 31, 2018 •
edited

@techknowlogick you mean add the NO_AUTHORIZED_KEYS_FILE=false setting to custom/conf/app.ini?

Hmm. I think if it's going to go in to the suggested app.ini by default, I should change it to SSH_CREATE_AUTHORIZED_KEYS_FILE=true so that we don't have a double negative.

@techknowlogick
Copy link
Member

techknowlogick commented Oct 31, 2018 via email •
edited

@zeripath zeripath force-pushed the AuthorizedKeysCommand branch 2 times, most recently from 8b8df05 to 9e87637 Compare October 31, 2018 20:28
@zeripath
Copy link
Contributor Author

Sorry I just noticed that XORM was reporting stuff on stdout that would conflict with the authorized_keys standard so adjusted the code.

@codecov-io
Copy link

codecov-io commented Oct 31, 2018 •
edited

Codecov Report

Merging #5236 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5236      +/-   ##
==========================================
- Coverage   37.62%   37.61%   -0.01%     
==========================================
  Files         310      310              
  Lines       46039    46040       +1     
==========================================
- Hits        17320    17319       -1     
- Misses      26240    26242       +2     
  Partials     2479     2479
Impacted Files Coverage Δ
models/ssh_key.go 39.14% <100%> (ø) ⬆️
modules/setting/setting.go 48.31% <100%> (+0.06%) ⬆️
models/repo_list.go 63.29% <0%> (-1.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00533d3...0b5115f. Read the comment docs.

@techknowlogick techknowlogick added this to the 1.7.0 milestone Nov 1, 2018
@techknowlogick techknowlogick added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 1, 2018
@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 1, 2018
@techknowlogick techknowlogick merged commit 7d9a191 into go-gitea:master Nov 1, 2018
@zeripath zeripath mentioned this pull request Nov 1, 2018
Closed
@zeripath
Copy link
Contributor Author

zeripath commented Nov 1, 2018

I guess this request partially closes #1870?

@zeripath zeripath deleted the AuthorizedKeysCommand branch November 1, 2018 22:50
@sapk
Copy link
Member

sapk commented Dec 14, 2019 •
edited

@zeripath I am looking at this to use it in the docker and remove the write to authorized_keys file but I can figure why the -type flag is required since not needed and seems not used. Do you have any insight ?

Don't mind I have understand. Sorry.

lafriks
lafriks reviewed Dec 15, 2019
View reviewed changes
cmd/keys.go
return err
}

publicKey, err := models.SearchPublicKeyByContent(content)
Copy link
Member

Choose a reason for hiding this comment

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

This should use internal api not models directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would have been a good spot >1 year ago... I guess we need to pop a bug fix up...

Copy link
Member

Choose a reason for hiding this comment

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

It just popped up in notifications 😅

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lafriks lafriks lafriks approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@zeripath @techknowlogick @codecov-io @sapk @lafriks @bkcsoft