STEP-7 Code coverage added. #7

ekhvalov · 2023-12-29T06:35:10Z

No description provided.

MysterySuperhero

Граф зависимостей – гуд.
Схемы и их визуализация – гуд. В идеале опоясать их текстовыми пояснениями по моментам, описанным задании про документацию. Иначе полноценной документацией это назвать нельзя
Приседания с jwt показались половинчатыми, как будто оставили работу на пол пути. Стоит доработать, или явно пояснить почему оставили в таком состоянии и к чему стремились.

2 из 3 норм – лайк. На Stepik также засчитываем модуль.

MysterySuperhero · 2024-02-05T17:09:45Z

internal/middlewares/keycloak_claims.go

@@ -47,14 +46,12 @@ func (a access) HasRole(name string) bool {
 	return false
 }

-// Valid returns errors:
+// Validate checks validity of the claims (will be called by jwt-go library parser).
+// possible errors:
 // - from StandardClaims validation;


Комментарий к методу не соответствует действительности 🙂 Ошибок от StandardClaims больше нет

Исправил

MysterySuperhero · 2024-02-05T17:14:17Z

internal/middlewares/keycloak_claims.go

 )

 type claims struct {
-	jwt.StandardClaims
 	Aud            keycloakclient.Audition `json:"aud"`


Можно теперь спилить и пользоваться зеркальным полем из jwt.RegisteredClaims. У нас была специфическая нужда выделить отдельное поле из jwt.StandardClaims, т.к. там aud строго строка

Исправил

MysterySuperhero · 2024-02-05T17:16:42Z

internal/middlewares/keycloak_claims.go

 )

 type claims struct {
-	jwt.StandardClaims
 	Aud            keycloakclient.Audition `json:"aud"`
 	AuthTime       int64                   `json:"auth_time"`


Раз уж начали глядеть сюда, то зачем вам это поле? Вроде, не используется 🙂

Удалил

MysterySuperhero · 2024-02-05T17:18:20Z

internal/middlewares/keycloak_jwt_parser.go

+	}, nil
+}
+
+// q: write documentation


Академический интерес: что значит q:?

Артифакт от общения с GitHub Copilot :)

MysterySuperhero · 2024-02-05T17:26:07Z

internal/middlewares/keycloak_jwt_parser.go

+	issuer          string          `option:"mandatory" validate:"required"`
+}
+
+func NewJWTParser(opts JWTParserOptions) (*JWTParser, error) {


Ловко вышло, но:

Если переезжаем на что-то вместо, то старое хорошо бы спиливать. Не у дел остался код из keycloakclient. Ради этого же была затея с переездом? 🙂

Парсеру не место, кмк, в пакете с миддлварями. Возможно, стоит его запихнуть в тот же пакет, где лежит бывший клиент keycloak.

В keycloakclient ещё есть код авторизации, который используется в тестах, так что удалил только то, что связано с интроспекцией токена.
Парсер перенёс в пакет internaljwt.

MysterySuperhero · 2024-02-05T17:48:35Z

docs/ARCHITECTURE.md

+# C4 model architecture diagrams
+
+## System Context
+<details>


Не знал, как в MD-формате делать раскрывающиеся списки. Лайк 🙂

MysterySuperhero · 2024-02-05T17:56:59Z

Taskfile.yml

@@ -8,6 +8,7 @@ vars:
    docker-compose.yml
    docker-compose.sentry.yml
    docker-compose.swagger-ui.yml
+    docker-compose.structurizr.yml


Опечатка у вас, нет такого файла 🙂

Исправил.

MysterySuperhero · 2024-02-05T18:01:57Z

docs/workspace.dsl

+# https://docs.structurizr.com/dsl/basics#basics
+# To see diagrams:
+#   1. Run dependencies COMPOSE_PROFILES=structurizr-ui task deps
+#   2. Open http://localhost:8070


Прикольная штука – лайк 👍

Академический интерес: используете ли в работе? Поделитесь, пожалуйста, юзкейсами.

Пока только в этом проекте использовал.

MysterySuperhero · 2024-02-05T18:13:06Z

internal/config/parse_test.go

@@ -23,3 +25,11 @@ func TestParseAndValidate(t *testing.T) {
 	require.NoError(t, err)
 	assert.NotEmpty(t, cfg.Log.Level)
 }
+
+func TestParseAndValidateWithEnv(t *testing.T) {
+	assert.NoError(t, os.Setenv(fmt.Sprintf("%s_LOG_LEVEL", config.EnvPrefix), "debug"))


Тут, скорее, нужен require

Исправил.

MysterySuperhero · 2024-02-05T18:13:58Z

internal/config/parse_test.go

@@ -23,3 +25,11 @@ func TestParseAndValidate(t *testing.T) {
 	require.NoError(t, err)
 	assert.NotEmpty(t, cfg.Log.Level)
 }
+
+func TestParseAndValidateWithEnv(t *testing.T) {


Скорее, это тест на переопределение ENV-переменной значения из конфига 🙂

Переименовал тест.

Test renamed.

Task commands to run Swagger and Structurizr ui added.

JWT parser moved to package 'internaljwt'.

MysterySuperhero

По-прежнему лайк. Стало гораздо чище 🫡

MysterySuperhero · 2024-02-14T16:53:24Z

internal/config/config.go

+	BasePath     string `fig:"base_path" validate:"required,url"`
+	Realm        string `fig:"realm" validate:"required"`
+	ClientID     string `fig:"client_id" validate:"required"`
+	ClientSecret string `fig:"client_secret" validate:"required"`


Вы правы, я сам это упустил это при ревью. Лайк 👍

MysterySuperhero · 2024-02-14T17:09:14Z

internal/middlewares/keycloak_token_auth.go

-			}
-			if err := c.Valid(); err != nil {
-				return false, err
+				// var ve *jwt.ValidationError


А тут какую цель преследовали, пытаясь анализировать содержимое ошибки? 🙂

Судя по тому, что тесты проходят в keycloak_token_auth_test.go, ошибки из keycloak_claims.go стали прозрачно возвращаться -> можно спилить закоментированный код

Похожие артефакты как будто в func (s *KeycloakTokenAuthSuite) TestInvalidIssuer() {

MysterySuperhero · 2024-02-14T17:14:13Z

internal/middlewares/keycloak_token_auth_test.go

+	authMdlwr       echo.MiddlewareFunc
+	req             *http.Request
+	resp            *httptest.ResponseRecorder
+	ctx             echo.Context
 }

 func (s *KeycloakTokenAuthSuite) SetupTest() {


Лайк за адаптацию теста 🫡

MysterySuperhero · 2024-02-14T17:19:55Z

internal/middlewares/keycloak_token_auth.go

-			}
-			token, c, err := parseTokenAndClaims(tokenStr)
+			tokenStr := extractToken(auth, secWsProtocol)
+			// if result, err := introspector.IntrospectToken(eCtx.Request().Context(), tokenStr); err != nil {


Почти, осталось чуть-чуть подчистить, кмк 🙂

ekhvalov added 14 commits December 29, 2023 15:34

STEP-7 Code coverage added.

d7ba62b

STEP-7 Code coverage fixed.

b8f3b0e

STEP-7 Code coverage fixed 2.

ca7ca9e

STEP-7 Code coverage workflow removed.

6bf749f

STEP-7 Missed ginkgo libs fixed.

9c6ee3e

STEP-7 Support of environment variables added to config.

ffa9829

STEP-7 Keycloak token offline validation.

422b3dd

STEP-7 Dependency graph added.

2e6952b

STEP-7 C4 diagrams, Context, Container, Component (manager only).

4f9f711

STEP-7 Vendor.

e30087c

STEP-7 go.mod version fixed.

9819119

STEP-7 go version fixed.

73cec0a

STEP-7 Vendor 2.

af57474

STEP-7 ARCHITECTURE.md added.

550d53a

MysterySuperhero self-assigned this Feb 5, 2024

MysterySuperhero approved these changes Feb 5, 2024

View reviewed changes

ekhvalov added 3 commits February 14, 2024 10:38

STEP-7 Error check fixed.

686c556

Test renamed.

STEP-7 Typo fixed.

8778dae

Task commands to run Swagger and Structurizr ui added.

STEP-7 Keycloak unnecessary code removed.

5d69e71

JWT parser moved to package 'internaljwt'.

MysterySuperhero approved these changes Feb 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STEP-7 Code coverage added. #7

STEP-7 Code coverage added. #7

ekhvalov commented Dec 29, 2023

MysterySuperhero left a comment

MysterySuperhero Feb 5, 2024

ekhvalov Feb 14, 2024

MysterySuperhero Feb 5, 2024

ekhvalov Feb 14, 2024

MysterySuperhero Feb 5, 2024

ekhvalov Feb 14, 2024

MysterySuperhero Feb 5, 2024

ekhvalov Feb 7, 2024

MysterySuperhero Feb 5, 2024

ekhvalov Feb 14, 2024

MysterySuperhero Feb 5, 2024

MysterySuperhero Feb 5, 2024

ekhvalov Feb 14, 2024

MysterySuperhero Feb 5, 2024

ekhvalov Feb 7, 2024

MysterySuperhero Feb 5, 2024

ekhvalov Feb 14, 2024

MysterySuperhero Feb 5, 2024

ekhvalov Feb 14, 2024

MysterySuperhero left a comment

MysterySuperhero Feb 14, 2024

MysterySuperhero Feb 14, 2024

MysterySuperhero Feb 14, 2024

MysterySuperhero Feb 14, 2024

MysterySuperhero Feb 14, 2024

STEP-7 Code coverage added. #7

Are you sure you want to change the base?

STEP-7 Code coverage added. #7

Conversation

ekhvalov commented Dec 29, 2023

MysterySuperhero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MysterySuperhero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment