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

adding authorization to the already-existing authentication #365

Merged
merged 15 commits into from
Feb 7, 2022

Conversation

ForestJohnson
Copy link
Contributor

adding the following conditions:

  1. Email has been confirmed
  2. either the GTS instance is configured to not require admin approval for new accounts, or the admin has approved this account
  3. the account has not been disabled / suspended

TO:

  • the browser-based login handler and to
  • the TokenCheck function that is used throughout the API.


func ensureUserIsAuthorizedOrRedirect(ctx *gin.Context, user *gtsmodel.User) bool {
if user.ConfirmedAt.IsZero() {
ctx.Redirect(http.StatusSeeOther, CheckYourEmailPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

these redirects don't land anywhere yet right? just for my understanding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly, I haven't finished all of those parts yet, at least not on my current branch. I suppose its sort of disconnected like this because I was trying to do multiple smaller PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you would rather, I could make it 1 big one.

Copy link
Contributor

Choose a reason for hiding this comment

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

no no it's good as is, no worries

@@ -44,7 +44,7 @@ func (m *Module) AuthorizeGETHandler(c *gin.Context) {
s := sessions.Default(c)

if _, err := api.NegotiateAccept(c, api.HTMLAcceptHeaders...); err != nil {
c.JSON(http.StatusNotAcceptable, gin.H{"error": err.Error()})
c.HTML(http.StatusNotAcceptable, "error.tmpl", gin.H{"error": err.Error()})
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a m.clearSession(s) here? I forgot it last time when I added this error path; might as well fix it in this pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why does it need to clear the session in that case? That would only happen if someone was trying to hit the endpoint with a really weird client that probably doesn't implement sessions anyways

Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

Can you write some tests (in the same style as elsewhere in the codebase) to trigger !ensureUserIsAuthorizedOrRedirect? Once it's tested nicely I think this PR is good to go

@tsmethurst tsmethurst merged commit 6ed368c into superseriousbusiness:main Feb 7, 2022
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.

2 participants