Align sync pattern between users/groups #414
Conversation
| @@ -61,10 +61,6 @@ groups: | |||
| - name: cluster-CCC | |||
|
|
|||
| users: | |||
| - email: woz@example.com | |||
| groups: | |||
There was a problem hiding this comment.
Assigning groups declared in config directly to users is confusing. This was an artifact of adding groups functions before the feature of synchronizing them from the source, removing this functionality.
| @@ -174,29 +176,7 @@ func ApplyUserMapping(db *gorm.DB, users []ConfigUserMapping) error { | |||
| return usrReadErr | |||
| } | |||
|
|
|||
| // add the user to groups, these declarations can be overridden by external group syncing | |||
There was a problem hiding this comment.
This allowed for adding users declared in config to groups, this feature doesnt make sense anymore
| @@ -248,14 +205,36 @@ func ImportConfig(db *gorm.DB, bs []byte) error { | |||
| initialConfig = config | |||
|
|
|||
| return db.Transaction(func(tx *gorm.DB) error { | |||
| // gorm blocks global delete by default: https://gorm.io/docs/delete.html#Block-Global-Delete | |||
| if err := tx.Where("1 = 1").Delete(&Role{}).Error; err != nil { | |||
There was a problem hiding this comment.
I tried the drop/create table method here instead, but it didn't re-create the relational tables. This only runs on initial config import.
There was a problem hiding this comment.
Should we be deleting all rows here vs selectively adding/removing rows? (some of the thinking behind #241)
There was a problem hiding this comment.
Yeah, I could switch to that for sure. My opinion here was that it is less complex to just clear configuration on import (which only happens on start-up) where we want the state to exactly match the config anyway. I can see that building the database back on initial sync is a bit of a pain though.
There was a problem hiding this comment.
I don't see what protects it from running multiple times in the registry.go Run()
| @@ -209,43 +185,6 @@ func TestExistingSourceIsOverridden(t *testing.T) { | |||
| assert.Equal(t, fakeOktaSource.Domain, importedOkta.Domain) | |||
| } | |||
|
|
|||
| func containsUserRoleForDestination(db *gorm.DB, user User, destinationId string, roleName string) (bool, error) { | |||
There was a problem hiding this comment.
just moved this to the end of the file
- remove direct group assignment to users in config - only create group when it is active at the source - reimport config on group creation - update tests - clear role, users, and groups on first config application
- wrap sync errors with context - make sync its own function - time sync events
BruceMacD
force-pushed
the
align-sync-pattern
branch
from
d3d10e6
to
2631490
Compare
| @@ -248,14 +205,36 @@ func ImportConfig(db *gorm.DB, bs []byte) error { | |||
| initialConfig = config | |||
|
|
|||
| return db.Transaction(func(tx *gorm.DB) error { | |||
| // gorm blocks global delete by default: https://gorm.io/docs/delete.html#Block-Global-Delete | |||
| if err := tx.Where("1 = 1").Delete(&Role{}).Error; err != nil { | |||
There was a problem hiding this comment.
I don't see what protects it from running multiple times in the registry.go Run()
| if err != nil { | ||
| return err | ||
|
|
||
| if err := tx.Model(&group).Association("Users").Replace(users); err != nil { |
There was a problem hiding this comment.
this needs a len() check and a clear for the zero case?
There was a problem hiding this comment.
Good catch, I tested this one with an empty list and it's ok in this case
|
@ssoroka Edit: I get what you mean now, I'm going to leave #241 open and address incrementally building configuration for that ticket. That way this function would be safe to call anywhere. |
Closes:
#363 Align user/group sync patterns
#389
primary key requirederror no longer occurs when there are no inactive groups#417 general supportability refactoring
Partially addresses:
#241 User/group mappings are built incrementally at runtime after an initial clear on config import
This change brings users/groups into the same pattern which does not create the entity until it is found at the source, then a config import finds the relevant role bindings.
Changes: