From 061120a52dccb8db2a5b044f9b12d606b6c81ced Mon Sep 17 00:00:00 2001 From: Jeffrey van Ostayen Date: Thu, 28 Dec 2023 09:59:26 +0100 Subject: [PATCH] review comments --- services/tenants/apikeys/application.go | 20 +++-- .../tenants/transports/apikeys_http_test.go | 77 +++++++++++++------ 2 files changed, 65 insertions(+), 32 deletions(-) diff --git a/services/tenants/apikeys/application.go b/services/tenants/apikeys/application.go index 4bcd2270..79f30601 100644 --- a/services/tenants/apikeys/application.go +++ b/services/tenants/apikeys/application.go @@ -82,9 +82,17 @@ func (s *service) AuthenticateApiKey(base64IdAndKeyCombination string) (ApiKeyAu } isValid := hashed.compare(apiKey) if isValid { - return ApiKeyAuthenticationDTO{ - TenantID: hashed.TenantID, - }, nil + if hashed.ExpirationDate != nil { + exp := hashed.ExpirationDate.Unix() + return ApiKeyAuthenticationDTO{ + TenantID: hashed.TenantID, + Expiration: &exp, + }, nil + } else { + return ApiKeyAuthenticationDTO{ + TenantID: hashed.TenantID, + }, nil + } } return ApiKeyAuthenticationDTO{}, ErrKeyNotFound } @@ -94,10 +102,8 @@ type Filter struct { } type ApiKeyAuthenticationDTO struct { - TenantID int64 `json:"sub"` // Sub is how Ory Oathkeeper identifies the important information in the response - Expiration time.Duration - // TODO: check, how to make ory oathkeeper only use the first value and not 'refresh' this expiration each time a request is authenticated? - // TODO: is there a diference in flow: just check existing token or get new token? + TenantID int64 `json:"sub"` // Sub is how Ory Oathkeeper identifies the important information in the response + Expiration *int64 `json:"expiration_date"` } type ApiKeyDTO struct { diff --git a/services/tenants/transports/apikeys_http_test.go b/services/tenants/transports/apikeys_http_test.go index d7f28cf0..59b61968 100644 --- a/services/tenants/transports/apikeys_http_test.go +++ b/services/tenants/transports/apikeys_http_test.go @@ -19,7 +19,7 @@ func TestNewApiKeyInvalidJsonBody(t *testing.T) { // Arrange svc := apiKeyServiceMock{} transport := testTransport(&svc) - req, _ := http.NewRequest("POST", "/api-keys", strings.NewReader(`blablabla`)) + req, _ := http.NewRequest("POST", "/tenants/api-keys", strings.NewReader(`blablabla`)) // Act rr := httptest.NewRecorder() @@ -35,7 +35,7 @@ func TestNewApiKeyNoName(t *testing.T) { // Arrange svc := apiKeyServiceMock{} transport := testTransport(&svc) - req, _ := http.NewRequest("POST", "/api-keys", strings.NewReader(`{"name": "", "organisation_id": 905}`)) + req, _ := http.NewRequest("POST", "/tenants/api-keys", strings.NewReader(`{"name": "", "organisation_id": 905}`)) req.Header.Add("content-type", "application/json") // Act @@ -52,7 +52,7 @@ func TestNewApiKeyNoOrganisationID(t *testing.T) { // Arrange svc := apiKeyServiceMock{} transport := testTransport(&svc) - req, _ := http.NewRequest("POST", "/api-keys", strings.NewReader(`{"name": "wasdasdas", "organisation_id": 0}`)) + req, _ := http.NewRequest("POST", "/tenants/api-keys", strings.NewReader(`{"name": "wasdasdas", "organisation_id": 0}`)) req.Header.Add("content-type", "application/json") // Act @@ -69,7 +69,7 @@ func TestNewApiKeyExpirationDateNotInTheFuture(t *testing.T) { // Arrange svc := apiKeyServiceMock{} transport := testTransport(&svc) - req, _ := http.NewRequest("POST", "/api-keys", + req, _ := http.NewRequest("POST", "/tenants/api-keys", strings.NewReader(fmt.Sprintf(`{"name": "wasdasdas", "organisation_id": 12, "expiration_date": "%s"}`, time.Now().Add(-time.Hour*24).Format(time.RFC3339)))) req.Header.Add("content-type", "application/json") @@ -93,7 +93,7 @@ func TestNewApiKeyTenantIsNotFound(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("POST", "/api-keys", strings.NewReader(`{"name": "whatever", "organisation_id": 905}`)) + req, _ := http.NewRequest("POST", "/tenants/api-keys", strings.NewReader(`{"name": "whatever", "organisation_id": 905}`)) req.Header.Add("content-type", "application/json") // Act @@ -116,7 +116,7 @@ func TestNewApiKeyErrorOccurs(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("POST", "/api-keys", strings.NewReader(`{"name": "whatever", "organisation_id": 905}`)) + req, _ := http.NewRequest("POST", "/tenants/api-keys", strings.NewReader(`{"name": "whatever", "organisation_id": 905}`)) req.Header.Add("content-type", "application/json") // Act @@ -141,7 +141,7 @@ func TestNewApiKeyIsCreatedWithExpirationDate(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("POST", "/api-keys", strings.NewReader(fmt.Sprintf(`{"name": "whatever", "organisation_id": 905, "expiration_date": "%s"}`, exp.Format("2006-01-02T15:04:05.999999999Z")))) + req, _ := http.NewRequest("POST", "/tenants/api-keys", strings.NewReader(fmt.Sprintf(`{"name": "whatever", "organisation_id": 905, "expiration_date": "%s"}`, exp.Format("2006-01-02T15:04:05.999999999Z")))) req.Header.Add("content-type", "application/json") // Act @@ -164,7 +164,7 @@ func TestNewApiKeyIsCreatedWithoutExpirationDate(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("POST", "/api-keys", strings.NewReader(`{"name": "whatever", "organisation_id": 905}`)) + req, _ := http.NewRequest("POST", "/tenants/api-keys", strings.NewReader(`{"name": "whatever", "organisation_id": 905}`)) req.Header.Add("content-type", "application/json") // Act @@ -181,7 +181,7 @@ func TestRevokeApiKeyInvalidApiKeyId(t *testing.T) { // Arrange svc := apiKeyServiceMock{} transport := testTransport(&svc) - req, _ := http.NewRequest("DELETE", "/api-keys/blablalb", nil) + req, _ := http.NewRequest("DELETE", "/tenants/api-keys/blablalb", nil) // Act rr := httptest.NewRecorder() @@ -202,7 +202,7 @@ func TestRevokeApiKeyRevokesApiKey(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("DELETE", "/api-keys/123", nil) + req, _ := http.NewRequest("DELETE", "/tenants/api-keys/123", nil) // Act rr := httptest.NewRecorder() @@ -223,7 +223,7 @@ func TestRevokeApiKeyRevokeFails(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("DELETE", "/api-keys/12343", nil) + req, _ := http.NewRequest("DELETE", "/tenants/api-keys/12343", nil) // Act rr := httptest.NewRecorder() @@ -244,7 +244,7 @@ func TestRevokeApiKeyKeyDoesNotExist(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("DELETE", "/api-keys/12343", nil) + req, _ := http.NewRequest("DELETE", "/tenants/api-keys/12343", nil) // Act rr := httptest.NewRecorder() @@ -260,7 +260,7 @@ func TestAuthenticateNoAuthorizationHeaderInRequest(t *testing.T) { // Arrange svc := apiKeyServiceMock{} transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/authenticate", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/authenticate", nil) // Act rr := httptest.NewRecorder() @@ -275,7 +275,7 @@ func TestAuthenticateAuthorizationHeaderIncorrectFormat(t *testing.T) { // Arrange svc := apiKeyServiceMock{} transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/authenticate", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/authenticate", nil) // Act req.Header["Authorization"] = []string{"wrong format!!"} @@ -296,7 +296,7 @@ func TestAuthenticateErrorOccursWhileValidatingApiKey(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/authenticate", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/authenticate", nil) // Act req.Header["Authorization"] = []string{"Bearer MjMxNDMyNDM6bXl2YWxpZGFwaWtleQ=="} @@ -318,7 +318,7 @@ func TestAuthenticateApiKeyIsNotFound(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/authenticate", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/authenticate", nil) // Act req.Header["Authorization"] = []string{"Bearer MjMxNDMyNDM6bXl2YWxpZGFwaWtleQ=="} @@ -340,7 +340,7 @@ func TestAuthenticateApiKeyInvalidEncodingErrorOccurs(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/authenticate", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/authenticate", nil) // Act req.Header["Authorization"] = []string{"Bearer blablabla"} @@ -353,18 +353,19 @@ func TestAuthenticateApiKeyInvalidEncodingErrorOccurs(t *testing.T) { assert.Len(t, svc.AuthenticateApiKeyCalls(), 1) } -func TestAuthenticateApiKeyIsValid(t *testing.T) { +func TestAuthenticateApiKeyIsValidNoExpirationDate(t *testing.T) { // Arrange svc := apiKeyServiceMock{ AuthenticateApiKeyFunc: func(base64IdAndKeyCombination string) (apikeys.ApiKeyAuthenticationDTO, error) { assert.Equal(t, "MjMxNDMyNDM6bXl2YWxpZGFwaWtleQ==", base64IdAndKeyCombination) return apikeys.ApiKeyAuthenticationDTO{ - TenantID: 431, + TenantID: 431, + Expiration: nil, }, nil }, } transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/authenticate", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/authenticate", nil) // Act req.Header["Authorization"] = []string{"Bearer MjMxNDMyNDM6bXl2YWxpZGFwaWtleQ=="} @@ -373,7 +374,33 @@ func TestAuthenticateApiKeyIsValid(t *testing.T) { // Assert assert.Equal(t, http.StatusOK, rr.Code) - assert.Equal(t, `{"sub":431}`+"\n", rr.Body.String()) + assert.Equal(t, `{"sub":431,"expiration_date":null}`+"\n", rr.Body.String()) + assert.Len(t, svc.AuthenticateApiKeyCalls(), 1) +} + +func TestAuthenticateApiKeyIsValidWithExpirationDate(t *testing.T) { + // Arrange + exp := time.Now().Add(time.Minute).Unix() + svc := apiKeyServiceMock{ + AuthenticateApiKeyFunc: func(base64IdAndKeyCombination string) (apikeys.ApiKeyAuthenticationDTO, error) { + assert.Equal(t, "MjMxNDMyNDM6bXl2YWxpZGFwaWtleQ==", base64IdAndKeyCombination) + return apikeys.ApiKeyAuthenticationDTO{ + TenantID: 431, + Expiration: &exp, + }, nil + }, + } + transport := testTransport(&svc) + req, _ := http.NewRequest("GET", "/tenants/api-keys/authenticate", nil) + + // Act + req.Header["Authorization"] = []string{"Bearer MjMxNDMyNDM6bXl2YWxpZGFwaWtleQ=="} + rr := httptest.NewRecorder() + transport.ServeHTTP(rr, req) + + // Assert + assert.Equal(t, http.StatusOK, rr.Code) + assert.Equal(t, fmt.Sprintf(`{"sub":431,"expiration_date":%d}`+"\n", exp), rr.Body.String()) assert.Len(t, svc.AuthenticateApiKeyCalls(), 1) } @@ -395,7 +422,7 @@ func TestListApiKeysReturnsPaginatedList(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/list", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/list", nil) // Act rr := httptest.NewRecorder() @@ -404,7 +431,7 @@ func TestListApiKeysReturnsPaginatedList(t *testing.T) { // Assert assert.Equal(t, http.StatusOK, rr.Code) assert.Equal(t, - `{"links":{"previous":"","next":"/api-keys/list?cursor=encoded_cursor"},"page_size":2,"total_count":0,"data":[{"name":"api-key-1","tenant_id":0,"expiration_date":null},{"name":"api-key-2","tenant_id":0,"expiration_date":null}]}`+"\n", rr.Body.String()) + `{"links":{"previous":"","next":"/tenants/api-keys/list?cursor=encoded_cursor"},"page_size":2,"total_count":0,"data":[{"name":"api-key-1","tenant_id":0,"expiration_date":null},{"name":"api-key-2","tenant_id":0,"expiration_date":null}]}`+"\n", rr.Body.String()) assert.Len(t, svc.ListAPIKeysCalls(), 1) } @@ -412,7 +439,7 @@ func TestListApiKeysInvalidParams(t *testing.T) { // Arrange svc := apiKeyServiceMock{} transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/list?tenant_id=blablalq", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/list?tenant_id=blablalq", nil) // Act rr := httptest.NewRecorder() @@ -432,7 +459,7 @@ func TestListApiKeysErrorsOccursWhileRetrievingData(t *testing.T) { }, } transport := testTransport(&svc) - req, _ := http.NewRequest("GET", "/api-keys/list", nil) + req, _ := http.NewRequest("GET", "/tenants/api-keys/list", nil) // Act rr := httptest.NewRecorder()