-
Notifications
You must be signed in to change notification settings - Fork 3
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
Persistent tokens and hook deletion #22
Conversation
Instead of ${RANDOM}
The repopath->usertoken map is now replaced by files on disk. When a hook is created, a file is stored in a 'tokens' directory with the name of the repository (/ is replaced by -: "user-reponame"). The token is stored in the same way as with the gin-client: UserToken struct is stored using gob.
Use BasicAuth to retrieve existing tokens for the user when they log in and check if they already have a gin-valid token active. Uses the new GetTokens function in gin-cli
Mapping session->token and repo->token will now be handled by linking files on disk.
Since we deal in validation and validators, using "validate" in this function name might be misleading.
b32 encoding makes the names filesystem-friendly.
Requires changing the hook struct type that gets passed around for the templates: deletion requires hook ID. Hook state enum will be used to signal the state a hook is in beyond just active or not (misconfigured, set but disabled, etc) and offer to the user the option to fix the state or remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, very nice!
web/hooks.go
Outdated
sessionid, err := r.Cookie(cookiename) | ||
ut, err := getSessionOrRedirect(w, r) | ||
if err != nil { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is potentially not logged. getSessionOrRedirect
logs in one error case and does not in another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSessionOrRedirect
logs the error only if the user has a session cookie set and the server doesn't have a session with that ID. This is the second case where the error is Invalid session found in cookie
. The second case, No session cookie found
isn't really an error, it just occurs when a user goes to a page that requires login and has no session cookie. I don't think it's necessary to log this, but I can add it if you want.
web/hooks.go
Outdated
|
||
ut, err := getSessionOrRedirect(w, r) | ||
if err != nil { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See getSessionOrRedirect
error comment above.
web/token.go
Outdated
|
||
// loadUserToken reads a token from disk using the username as filename. | ||
// The location is defined by config.Dir.Tokens. | ||
func loadUserToken(username string) (gweb.UserToken, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it makes sense to use the name getTokenByUser
analogous to getTokenBySession
?
|
||
// rmTokenRepoLink deletes a repository -> token link, removing our ability to | ||
// clone the repository. | ||
func rmTokenRepoLink(repopath string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are tokens and session symlinks ever cleaned up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Opened issue #23.
web/user.go
Outdated
@@ -300,7 +346,7 @@ func ShowRepo(w http.ResponseWriter, r *http.Request) { | |||
return | |||
} | |||
|
|||
session, err := getSessionOrRedirect(w, r) | |||
ut, err := getSessionOrRedirect(w, r) | |||
if err != nil { | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See getSessionOrRedirect
error comment above.
This PR introduces persistent tokens and user sessions and adds support for removing hooks from GIN repositories.
Hook deletion
Some changes to the page templates were made to support hook deletion.
A new enum is introduced to support hooks in multiple states (e.g., a hook exists but it is disabled, or misconfigured). The plan is to be able to inform the user of a bad configuration and offer to fix it.
Persistent tokens and session IDs
The session persistence uses token files stored as Go gobs (the same format used by gin-cli). The session storage and repository cloning works using the following strategy:
by-sessionid
.by-repo
.Both the session ID and the repository name are base32 encoded when linking to make the filesystem friendly (ASCII).