Skip to content

feat: add session actor middleware #897

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

Closed
wants to merge 11 commits into from
Closed

feat: add session actor middleware #897

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f868929
feat(cdr): add session actor types
deansheather Apr 6, 2022
ada9c1a
fixup! feat(cdr): add session actor types
deansheather Apr 6, 2022
999c197
feat(cdr): add session actor middleware
deansheather Apr 7, 2022
6f7b7d4
chore: add session actor middleware tests
deansheather Apr 7, 2022
1cf0c58
Merge branch 'main' into actor-types
deansheather Apr 7, 2022
175aa8c
chore: remove system actor
deansheather Apr 8, 2022
cb9ec1e
chore: write RequireAuthentication middleware
deansheather Apr 8, 2022
efcee3d
fixup! chore: write RequireAuthentication middleware
deansheather Apr 8, 2022
4c9dd06
chore: change to internal tests for session package
deansheather Apr 8, 2022
5bea2cb
chore: pr review
deansheather Apr 8, 2022
dadef23
Merge branch 'main' into actor-types
deansheather Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
chore: pr review
  • Loading branch information
@deansheather
deansheather committed Apr 8, 2022
commit 5bea2cb4389bee3a3efcb889076298dceba5cd26
41 changes: 26 additions & 15 deletions coderd/access/session/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,38 +73,30 @@ func UserActorFromRequest(ctx context.Context, db database.Store, rw http.Respon
return nil, true
}

// APIKeys are formatted: ${id}-${secret}. The ID is 10 characters and the
// secret is 22.
parts := strings.Split(cookie.Value, "-")
// TODO: Dean - workspace agent token auth should not share the same cookie
// name as regular auth
if len(parts) == 5 {
if strings.Count(cookie.Value, "-") == 4 {
// Skip anything that looks like a UUID for now.
return nil, true
}
if len(parts) != 2 || len(parts[0]) != 10 || len(parts[1]) != 22 {

keyID, _, hashedSecret, err := parseAPIKey(cookie.Value)
if err != nil {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: fmt.Sprintf("invalid API key cookie %q format", AuthCookie),
Message: fmt.Sprintf("invalid API key cookie %q: %v", AuthCookie, err),
})
return nil, false
}

// We hash the secret before getting the key from the database to ensure we
// keep this function fixed time.
var (
keyID = parts[0]
keySecret = parts[1]
hashedSecret = sha256.Sum256([]byte(keySecret))
)

// Get the API key from the database.
key, err := db.GetAPIKeyByID(ctx, keyID)
if xerrors.Is(err, sql.ErrNoRows) {
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
Message: apiKeyInvalidMessage,
})
return nil, false
} else if err != nil {
}
if err != nil {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: fmt.Sprintf("get API key by id: %s", err.Error()),
})
Expand Down Expand Up @@ -161,3 +153,22 @@ func UserActorFromRequest(ctx context.Context, db database.Store, rw http.Respon

return NewUserActor(u, key), true
}

func parseAPIKey(apiKey string) (id, secret string, hashed []byte, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Just and example @kylecarbs of a great function to unit test with an easy tabled test. To do this using the middleware would be such a pain because invalid apiKey strings might not even get far enough in the function to get to this parse.

Regardless if the invalid strings reach this in prod, I still think it's of value to know this function works as intended incase it's ever moved/reused.

Copy link
Member

Choose a reason for hiding this comment

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

If this function is well-tested outside of the middleware, it could be argued the middleware doesn't have to be. If both are expected to be well-tested, then tests become duplicative. I'm of the opinion testing the user-expected behavior is much more important than if the code actually works, because the user just wants it to work for them.

Copy link
Member

Choose a reason for hiding this comment

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

The middleware unit test will not test this one function to all of it's edge cases. For one, the middleware does some basic validation above this usage, so any tokens that fall those checks won't make it to parseAPIKey.

The code might be able to be refactored to ensure all keys hit parseAPIKey, but I'm speaking more generally than this one case.

I'm of the opinion testing the user-expected behavior is much more important than if the code actually works, because the user just wants it to work for them.

I don't see how these are not related. These functions are the building blocks, and often they will be reused/adapted by a developer in the future (months out).

I am of the opinion if I write helper functions like this, it's very easy to build a tabled test to confirm my implementation is correct. And if someone comes in later, the tabled tests can often serve better than a comment for what should happen in given cases, because let's be honest sometimes comments are vague/ambiguous. A tabled test does not have this ambiguity. Worse case the tabled test is missing an edge case, in which case the new developer can easily add it.

I do agree if you sufficiently test all the building blocks of a function, then you do not need to test the, in this case, the middle function as exhaustively.

Copy link
Member

Choose a reason for hiding this comment

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

The functions build the API surface, but the most important part of the code is that the API works as expected. I'm not arguing against testing the internals, but we should try to get away with testing via externals first to reduce complexity.

// APIKeys are formatted: ${id}-${secret}. The ID is 10 characters and the
// secret is 22.
parts := strings.Split(apiKey, "-")
if len(parts) != 2 || len(parts[0]) != 10 || len(parts[1]) != 22 {
return "", "", nil, xerrors.New("invalid API key format")
}

// We hash the secret before getting the key from the database to ensure we
// keep this function fixed time.
var (
keyID = parts[0]
keySecret = parts[1]
hashedSecret = sha256.Sum256([]byte(keySecret))
)

return keyID, keySecret, hashedSecret[:], nil
}
11 changes: 3 additions & 8 deletions coderd/access/session/user_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ func TestUserActor(t *testing.T) {
gotAPIKey, err := db.GetAPIKeyByID(r.Context(), apiKey.ID)
require.NoError(t, err)

assertTimesEqual(t, apiKey.LastUsed, gotAPIKey.LastUsed)
require.WithinDuration(t, apiKey.LastUsed, gotAPIKey.LastUsed, time.Second)
assertTimesNotEqual(t, apiKey.ExpiresAt, gotAPIKey.ExpiresAt)
})

Expand All @@ -204,7 +204,7 @@ func TestUserActor(t *testing.T) {
require.NoError(t, err)

assertTimesNotEqual(t, apiKey.LastUsed, gotAPIKey.LastUsed)
assertTimesEqual(t, apiKey.ExpiresAt, gotAPIKey.ExpiresAt)
require.WithinDuration(t, apiKey.ExpiresAt, gotAPIKey.ExpiresAt, time.Second)
})

t.Run("ValidUpdateExpiry", func(t *testing.T) {
Expand All @@ -229,7 +229,7 @@ func TestUserActor(t *testing.T) {
gotAPIKey, err := db.GetAPIKeyByID(r.Context(), apiKey.ID)
require.NoError(t, err)

assertTimesEqual(t, apiKey.LastUsed, gotAPIKey.LastUsed)
require.WithinDuration(t, apiKey.LastUsed, gotAPIKey.LastUsed, time.Second)
assertTimesNotEqual(t, apiKey.ExpiresAt, gotAPIKey.ExpiresAt)
})
}
Expand Down Expand Up @@ -299,11 +299,6 @@ func newAPIKey(t *testing.T, db database.Store, user database.User, lastUsed, ex
return apiKey, fmt.Sprintf("%v-%v", id, secret)
}

func assertTimesEqual(t *testing.T, a, b time.Time) {
t.Helper()
require.Equal(t, a.Truncate(time.Second), b.Truncate(time.Second))
}

func assertTimesNotEqual(t *testing.T, a, b time.Time) {
t.Helper()
require.NotEqual(t, a.Truncate(time.Second), b.Truncate(time.Second))
Expand Down