[CLOUDTRUST-2109] Authorization refactoring #156

harture · 2020-01-15T08:07:38Z

We plan to split the authorizations refactoring in 2 phases (2 PR).
First, we deploy the 1st PR which contains the authorization edition/management. We perform the authorizations configuration which will be stored in DB. This config will not be used, the JSON is still used to check the authorizations.
Second step, we deploy the 2nd PR which will remove the usage of JSON and compute the authorization from the DB configuration.
This strategy should allow us to minimize the downtime and migration risks.

coveralls · 2020-01-15T08:13:09Z

Pull Request Test Coverage Report for Build 1529

554 of 656 (84.45%) changed or added relevant lines in 24 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.9%) to 90.362%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/management/component.go	203	204	99.51%
pkg/statistics/authorization.go	22	23	95.65%
pkg/statistics/component.go	15	16	93.75%
pkg/event/http.go	2	4	50.0%
pkg/export/http.go	0	2	0.0%
pkg/export/storage.go	0	2	0.0%
pkg/statistics/endpoint.go	4	6	66.67%
pkg/export/component.go	0	3	0.0%
pkg/management/endpoint.go	64	67	95.52%
internal/keycloakb/configdbinstrumenting.go	0	25	0.0%

Totals
Change from base Build 1497:	-0.9%
Covered Lines:	3694
Relevant Lines:	4088

💛 - Coveralls

…LOUDTRUST-2109_AuthorizationRefactoring

api/management/api.go

fperot74 · 2020-01-20T10:20:38Z

internal/keycloakb/configdbmodule.go

@@ -67,3 +76,76 @@ func (c *configurationDBModule) GetConfiguration(context context.Context, realmI
 		return config, err
 	}
 }
+
+func (c *configurationDBModule) GetAuthorizations(ctx context.Context, realmID string, groupName string) ([]dto.Authorization, error) {


May be reused in paper-card-service

When we will retrieve the authorizations to apply authorizations, we will retrieve all of them not just one for a specific realm and group.

internal/keycloakb/configdbmodule_test.go

fperot74 · 2020-01-20T10:36:58Z

pkg/events/component.go

@@ -40,6 +42,23 @@ func (ec *component) reportEvent(ctx context.Context, apiCall string, values ...

 }

+// Get actions
+func (ec *component) GetActions(ctx context.Context) ([]api.ActionRepresentation, error) {


Could be part of the common lib

I agree this method is very similar for each Component but a simple and elegant way to do it ...
Would you have any idea ?

Let's say we will have a look at this when we'll have time for this... (it almost means never)

Why not sooner?

harture

Responses directly as comment

bsoniam · 2020-01-23T09:22:52Z

api/events/api.go

@@ -2,6 +2,12 @@ package events_api

 import "database/sql"

+// ActionRepresentation struct
+type ActionRepresentation struct {


The same structure is used by papercard module.
If for the long term this stays the same, then we can move them in common service

I will move it in common during PR2

bsoniam · 2020-01-23T09:25:17Z

api/events/swagger-api_events.yaml

+              schema:
+                type: array
+                items:
+                  type: string


Is not the same as you code in the paper card?
/cards/actions:
get:
tags:
- Actions
summary: Get the list of all possible actions
responses:
200:
description: successful operation
content:
application/json:
schema:
type: array
items:
** $ref: '#/components/schemas/Actions' **

You are right, it was a bug, it is fixed in the PR #159

bsoniam · 2020-01-23T09:26:09Z

api/management/api.go

+}
+
+// ActionRepresentation struct
+type ActionRepresentation struct {


Same as before

bsoniam · 2020-01-23T09:41:33Z

api/management/swagger-api_management.yaml

@@ -15,6 +15,20 @@ tags:
 - name: Roles
  description: Roles management
 paths:
+  /actions:
+    get:
+      tags:


Same as before

TODO in PR2, thks

bsoniam · 2020-01-23T09:42:14Z

api/statistics/api.go

@@ -15,6 +15,12 @@ const (
 	RegExpTwoDigitsNumber = `^\d{1,2}$`
 )

+// ActionRepresentation struct
+type ActionRepresentation struct {
+	Name  *string `json:"name"`


same as before

bsoniam · 2020-01-23T09:42:26Z

api/statistics/swagger-api_statistics.yaml

@@ -9,6 +9,20 @@ tags:
 - name: Statistics
  description: Statistics management
 paths:
+  /statistics/actions:
+    get:
+      tags:


same as before

bsoniam · 2020-01-23T09:51:39Z

internal/messages/errormessages.go

-	GroudIDs           = "groupIds"
+	GroupID            = "groupId"
+	GroupName          = "groupName"
+	Name               = "name"


name gives enough information?

I will fix it

bsoniam · 2020-01-23T09:53:23Z

pkg/events/authorization.go

+var actions []security.Action
+
+func newAction(as string, scope security.Scope) security.Action {
+	a := security.Action{


from commom-service, master branch:
type Action struct {
Id int
Name string
Scope Scope
}
Id should be defined?

I think I can remove this Id, we finally don't use it.

bsoniam · 2020-01-23T09:54:18Z

pkg/events/component.go

@@ -40,6 +42,23 @@ func (ec *component) reportEvent(ctx context.Context, apiCall string, values ...

 }

+// Get actions
+func (ec *component) GetActions(ctx context.Context) ([]api.ActionRepresentation, error) {


Why not sooner?

bsoniam · 2020-01-23T10:15:08Z

pkg/statistics/authorization.go

+var actions []security.Action
+
+func newAction(as string, scope security.Scope) security.Action {
+	a := security.Action{


harture added 7 commits January 15, 2020 09:22

Group CRUD API

bd469d9

Update authorizations API

9b4f3ed

Authorizations validations

beb3000

GetActions API

7c3994e

kc role mappings update

40c26cf

Swagger update

0ff766d

Add tests

c22abf5

harture force-pushed the CLOUDTRUST-2109_AuthorizationRefactoring branch from 7fa2c70 to c22abf5 Compare January 15, 2020 08:22

Comment and indent in main

5eb77b6

harture changed the title ~~Cloudtrust 2109 authorization refactoring~~ [CLOUDTRUST-2109] Authorization refactoring Jan 15, 2020

harture added 9 commits January 15, 2020 09:44

Stats Tests

317a645

Events tests

77dc361

Unittests

7bcaad6

Refactoring

7a7c63f

Remove tracing and logging of module

8b00402

Unittests

894aa02

Tests

e1a1de2

Error messages

8a8db46

Add scope

0a083c6

harture requested review from bsoniam and fperot74 and removed request for bsoniam and fperot74 January 16, 2020 15:14

Fix goupID

d17a782

cloudtrust deleted a comment from harture Jan 17, 2020

harture added 3 commits January 17, 2020 14:07

fix tests

cf50e31

fixup

9c1e47a

Merge branch 'master' of github.com:cloudtrust/keycloak-bridge into C…

ecb16eb

…LOUDTRUST-2109_AuthorizationRefactoring

harture requested a review from fperot74 January 20, 2020 07:45

harture requested a review from bsoniam January 20, 2020 07:45

fperot74 approved these changes Jan 20, 2020

View reviewed changes

harture added 2 commits January 20, 2020 17:08

fix

67f46d6

Update dep

0ae9100

harture commented Jan 20, 2020

View reviewed changes

Update according feedback

6742a61

harture merged commit ee02f6e into master Jan 21, 2020

bsoniam reviewed Jan 23, 2020

View reviewed changes

fperot74 deleted the CLOUDTRUST-2109_AuthorizationRefactoring branch January 6, 2021 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLOUDTRUST-2109] Authorization refactoring #156

[CLOUDTRUST-2109] Authorization refactoring #156

harture commented Jan 15, 2020 •

edited by bsoniam

coveralls commented Jan 15, 2020 •

edited

fperot74 Jan 20, 2020

harture Jan 20, 2020

fperot74 Jan 20, 2020

harture Jan 20, 2020

fperot74 Jan 21, 2020

bsoniam Jan 23, 2020

harture left a comment •

edited

bsoniam Jan 23, 2020

harture Jan 23, 2020

bsoniam Jan 23, 2020

harture Jan 23, 2020

bsoniam Jan 23, 2020

bsoniam Jan 23, 2020

harture Jan 23, 2020

bsoniam Jan 23, 2020

bsoniam Jan 23, 2020

bsoniam Jan 23, 2020

harture Jan 23, 2020

bsoniam Jan 23, 2020

harture Jan 23, 2020

bsoniam Jan 23, 2020

bsoniam Jan 23, 2020

[CLOUDTRUST-2109] Authorization refactoring #156

[CLOUDTRUST-2109] Authorization refactoring #156

Conversation

harture commented Jan 15, 2020 • edited by bsoniam

coveralls commented Jan 15, 2020 • edited

Pull Request Test Coverage Report for Build 1529

💛 - Coveralls

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

harture left a comment • edited

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

harture commented Jan 15, 2020 •

edited by bsoniam

coveralls commented Jan 15, 2020 •

edited

harture left a comment •

edited