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

rageshake: Add file server with basic auth #3169

Merged
merged 2 commits into from
Feb 8, 2017
Merged

Conversation

kegsay
Copy link
Contributor

@kegsay kegsay commented Feb 8, 2017

Set via environment variables on startup.

Set via environment variables on startup.
if usr == "" || pass == "" {
fmt.Println("BUGS_USER and BUGS_PASS env vars not found. No authentication is running for /api/listing")
} else {
fs = basicAuth(fs, usr, pass, "Enter username and password")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't realm normally something like Riot bug reports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an arbitrary string which is sent to the client in a popup, which is usually prefixed with "This website says: REALM" hence the request to enter u/p. I don't care what this is called, so I'll change it to whatever.

Copy link
Member

@richvdh richvdh Feb 8, 2017

Choose a reason for hiding this comment

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

it's more than that. It is specifically used to identify things which should have the same u/p on a domain. Obviously if different apps with different auth all use "Enter username and password" that's no good.

Also, some clients may show "Enter the username and password for <realm>".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

} else {
fs = basicAuth(fs, usr, pass, "Enter username and password")
}
http.Handle("/api/listing/", fs)
Copy link
Member

Choose a reason for hiding this comment

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

so a link to an individual report will be /api/listing/2017-02-07/205222 ? How about /reports instead of /api/listing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the link that will be present for our deployment. The Apache is configured to route riot.im/bugreports/* to /api/* so the individual report is linked as https://riot.im/bugreports/listing/2017-02-07/205222. I can rename listing to reports if you want to be redundant.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. so why /api then? Why not just /submit and /listing (or /reports).

I suddenly realise I don't actually care about this, though. Do as you see fit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We as a team have certain conventions when it comes to Go and HTTP servers. One of them is that everything public is served under /api so that you can attach Prometheus metrics (which hook into /metrics) and pprof tooling (which hook into /debug/pprof) without making those endpoints visible to the wider internet which would be the case if we attached to /. See https://docs.google.com/document/d/1yu7qImAcr-_yN72Ndb5ht_zjyK0hxXQGZQYIKFaGBXw/edit

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

couple of things

@kegsay kegsay removed their assignment Feb 8, 2017
@richvdh richvdh merged commit d7bc47c into develop Feb 8, 2017
@richvdh
Copy link
Member

richvdh commented Feb 8, 2017

lgtm

@t3chguy t3chguy deleted the kegan/bugs-file-server branch May 12, 2022 08:56
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