Skip to content

Add InternalTokenURI to load/save InteralToken from an external file. #4412

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
Closed
wants to merge 3 commits into from
Closed

Add InternalTokenURI to load/save InteralToken from an external file. #4412

wants to merge 3 commits into from

Conversation

bkcsoft
Copy link
Member

@bkcsoft bkcsoft commented Jul 10, 2018 •
edited
Loading

  • URI HAVE TO start with 'file:' or 'file://'. Possibility to add http/s3
    support in the future.
  • On errors it WILL fail hard (in case of misconfiguration OR the storage not being available)
  • The file WILL be read if it exists and the token extracted from it.
  • The file WILL be created if it does not exist.

Closes #3246

@bkcsoft
Add InternalTokenURI to load/save InteralToken
4cec5ff 
... from an external file.

- URI HAVE TO start with 'file:' or 'file://'. Possibility to add http/s3
  support in the future.
- On errors it WILL ALWAYS fall back to reading AND WRITING
  InternalToken to the config file!
- The File HAVE TO exist to be used. Gitea WILL NOT create it for you.
  It can however be empty and a new token will be generated for you.
@bkcsoft
Copy link
Member Author

bkcsoft commented Jul 10, 2018

Do we deprecate INTERNAL_TOKEN in favour of INTERNAL_TOKEN_URI ? 🤔

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 10, 2018
@codecov-io
Copy link

codecov-io commented Jul 10, 2018 •
edited
Loading

Codecov Report

Merging #4412 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4412   +/-   ##
=======================================
  Coverage   20.06%   20.06%           
=======================================
  Files         153      153           
  Lines       30769    30769           
=======================================
  Hits         6174     6174           
  Misses      23652    23652           
  Partials      943      943

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 3e445cc...423d98a. Read the comment docs.

@bkcsoft
Copy link
Member Author

bkcsoft commented Jul 10, 2018

I'm also thinking about doing the same for LFS_JWT_SECRET 🤔

@lafriks
Copy link
Member

lafriks commented Jul 10, 2018

@bkc I think it should always use INTERNAL_TOKEN_URI if it is not empty and fail with error and not fall back to INTERNAL_TOKEN path in different error cases.
otherwise it can lead to strange behavior in case of misconfiguration or when added other storage options and that storage is inaccessible in that moment.

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Jul 10, 2018
@lafriks lafriks added this to the 1.6.0 milestone Jul 10, 2018
@lafriks lafriks added type/enhancement An improvement of existing functionality and removed type/enhancement An improvement of existing functionality labels Jul 10, 2018
bkcsoft
bkcsoft commented Jul 10, 2018
View reviewed changes
modules/setting/setting.go
default:
log.Fatal(4, "Unsupported URI-Scheme %q (INTERNAL_TOKEN_URL = %q)", tempURI.Scheme, uri)
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

My linter complains about a missing return even though we default to log.Fatal (which should be equivalent to os.Exit() ...)

@lafriks
Copy link
Member

lafriks commented Jul 10, 2018 •
edited
Loading

@bkcsoft it would be better to change return type to (string, error) for both functions and handle error in calling place. Also change would be needed for log.Fatal to return "", fmt.Errorf

@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Sep 3, 2018
@techknowlogick techknowlogick modified the milestones: 1.7.0, 1.8.0 Dec 19, 2018
@bkcsoft
Copy link
Member Author

bkcsoft commented Jan 23, 2019

Not working on this anymore, feel free to continue it if anyone wants to.

@bkcsoft bkcsoft closed this Jan 23, 2019
@techknowlogick techknowlogick deleted the remote-internal-token branch January 23, 2019 15:17
@techknowlogick techknowlogick mentioned this pull request Jan 23, 2019
Merged
@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
No reviews
Assignees
No one assigned
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.

Feature Request: Move INTERNAL_TOKEN value out of app.ini
4 participants
@bkcsoft @codecov-io @lafriks @techknowlogick