This repository has been archived by the owner on Jan 30, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 3
Add API authentication mechanism #155
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c65ce31
Add API authentication mechanism
nickclyde 214c619
Decode HMAC string correctly
nickclyde 1f6dd43
Make server decide when to auth
nickclyde 4374f5f
Merge branch 'master' into api-auth
1cc3109
Fix call to removed grpc method (see https://github.com/grpc/grpc-go/…
4786b76
Fix api_client references
35f6f1e
Add login token to controller spec
f40ee5d
Stop naming our error variables
05bd4f2
Add comment about static auth token
57ee8e6
Fix merge error
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,4 +40,5 @@ message LoginUserRequest { | |
message LoginUserResponse { | ||
string error = 1; | ||
User user = 2; | ||
string hmac = 3; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If i understand this correctly, the user token is essentially static because Key + UserID should always result in the same token.
This isn't wonderful because it doesn't tie the authorization token to a specific session and it's not revocable. If somebody ever got the token value they could impersonate the user all day long.
Not an immediately critical thing to fix since some authorization is a step forward. But a thing to return to later.
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.
Yeah, this doesn't seem great. Stealing this token would let you impersonate the user literally forever. I'll throw in a TODO.
Adding some randomness to the token isn't hard, but if we do that, we have to store it. So perhaps in the future we should put it in the database, tied to the user, along with a timestamp so it will expire automatically?
AFAIK soapboxd doesn't really have the concept of a session yet, so I'm very open to suggestions here.
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 revocable bit is probably more important (since that's how most API tokens work for command-line etc.)
The session scoping would only make sense if we got rid of the CLI and went web-app only since then you could do an Oauth flow to get a JWT and present that.
Some authentication is better than none so may as well ship this and revisit.