Valid Groups name, compatibility between --sync-method groups and users_groups

[christiangda]

Is your feature request related to a problem? Please describe.

Since I introduced the flag --sync-method to implement a different way to sync Groups and their members I noticed the negative impact of it. Trying to figure out what happened and why the impact was bad, and basically reading the project reported issues I noticed the following.

I broke the groups mapping's fields with the new --sync-method "groups", what I mean:

1) if you use --sync-method "users_groups", which was the original behavior

Users Fields

AWS SSO SCIM Schema		Google Workspace	Notes
userName	<--	primaryEmail
name	<--	{givenName, familyName }
displayName	<--	givenName + " " + familyName
active:	<--	!suspended
emails:	<--	[ primaryEmail ]	ignored from google, used primaryEmail
addresses:	<--	[ {type: "work"} ]	ignored from google, Fixed

Go code for AWS SSO SCIM Schema

type User struct {
	ID       string   `json:"id,omitempty"`
	Schemas  []string `json:"schemas"`
	Username string   `json:"userName"`
	Name     struct {
		FamilyName string `json:"familyName"`
		GivenName  string `json:"givenName"`
	} `json:"name"`
	DisplayName string        `json:"displayName"`
	Active      bool          `json:"active"`
	Emails      []UserEmail   `json:"emails"`
	Addresses   []UserAddress `json:"addresses"`
}

Groups Fields

AWS SSO SCIM Schema		Google Workspace	Notes
displayName	<--	email	Group email

Go code for AWS SSO SCIM Schema

type Group struct {
	ID          string   `json:"id,omitempty"`
	Schemas     []string `json:"schemas"`
	DisplayName string   `json:"displayName"`
	Members     []string `json:"members"`
}

2) if you use --sync-method "groups", my introduced behavior

Users Fields

Users mapping still the same that --sync-method "users_groups"

Groups Fields

AWS SSO SCIM Schema		Google Workspace	Notes
displayName	<--	name	Group name

The Problem

1. As you see, the original behavior (--sync-method "users_groups") is using Google group email as AWS SSO group name and I implemented (--sync-method "groups") using Google group name as AWS SSO group name.

Additionally exist the following aspect

Group Names as a displayName

• Could be duplicated, not unique
• Could be empty
• May not follow AWS SSO Group Names pattern (see notes below)

Group Email as a displayName

• Looks like a user instead of a group, I.M.H.O: ugly group name
• May not follow AWS SSO Group Names pattern (see notes below)

2. When the code fills the internal structure to store the Google Groups and their members, I used the Google Group Name as a key

Go code

func getGroupOperations(awsGroups []*aws.Group, googleGroups []*admin.Group) (add []*aws.Group, delete []*aws.Group, equals []*aws.Group) {

	awsMap := make(map[string]*aws.Group)
	googleMap := make(map[string]struct{})

	for _, awsGroup := range awsGroups {
		awsMap[awsGroup.DisplayName] = awsGroup
	}

	for _, gGroup := range googleGroups {
		googleMap[gGroup.Name] = struct{}{} // --> HERE
	}

	// AWS Groups found and not found in google
	for _, gGroup := range googleGroups {
		if _, found := awsMap[gGroup.Name]; found {
			equals = append(equals, awsMap[gGroup.Name])
		} else {
			add = append(add, aws.NewGroup(gGroup.Name))
		}
	}

	// Google Groups founds and not in aws
	for _, awsGroup := range awsGroups {
		if _, found := googleMap[awsGroup.DisplayName]; !found {
			delete = append(delete, aws.NewGroup(awsGroup.DisplayName))
		}
	}

	return add, delete, equals
}

NOTES

• I didn't find the name restrictions or name convention over the field displayName (Group Name in Web Console) on AWS SSO SCIM API, but the weird is that using the AWS Web Console for AWS SSO service with SCIM off, this shows the following message "Can contain only alphanumeric characters, or any of the following: ._- Maximum of 128 characters". But is you use AWS SSO SCIM APIto create a group, White spaces and @ are allowed, so?

References:

• System for Cross-domain Identity Management: Core Schema
• https://docs.aws.amazon.com/singlesignon/latest/userguide/users-groups-provisioning.html#username-email-unique

Describe the solution you'd like

Options:

1. I can make a PR with the necessary changes to use Google group email as AWS SSO group name for (--sync-method "groups"), but this is going to broke the people that have implemented the method --sync-method "groups"
2. I can implement a new flag --groups-name-field with possible values [name|email] to maintain compatibility, but this could be confusing, I think.
3. In all cases validate the value of displayName before call the AWS SSO SCIM API to avoid errors and log the warning message ignoring group, bad name or something else

Describe alternatives you've considered

Just the above, or somebody has a better idea?

[christiangda]

@joshuachong let me know what do you think about it.

Related to it, do you know where I can find the AWS SSO SCIM API restrictions or pattern for the Groups Display name?, valid characters or regex pattern, I have an almost ready PR to validate the group Name before call the AWS API and avoid errors related to it.

[badgerspoke]

Since I'm trying to make sense of this project I'll stick my oar in. This project has accidentally turned into a bit of a pickle. No offence meant, but we really could do with the rc8 vs rc9 sides to call a truce: Perhaps this issue could be a way to bridge the gap since there are probably folks on rc8 or prior and then new folks like me on rc9 with different behaviours being experienced for the same input/flags.

I totally take the points @bfreis makes in #47 about the SSO group name change in rc9, and agree the level of change is not suitable for an RC level change. However that change has for better or worse already been done. And right now with rc9 or from master there's no way to opt for the rc8 behaviour AFAICT. So it would seem that we absolutely do need a way to enable the previous behaviour so rc8 folks can update and we can all be friends and relax knowing that changes/PRs might actually be accepted.

To that end I vote for option 2 above: a flag --sso-group-name-field [name|email] (default 'name') (awful param name I know) to enable rc8 folks to go back to the original behaviour (but the default being name so we don't then break rc9 folks too).

Agreed also that some validation of displayName is required (side note AWS if you're listening, please update the docs to detail the actual constraints).

[bfreis]

To that end I vote for option 2 above: a flag --sso-group-name-field [name|email] (default 'name') (awful param name I know) to enable rc8 folks to go back to the original behaviour (but the default being name so we don't then break rc9 folks too)

If such a parameter is introduced, it must have rc8's behavior as the default. Otherwise, the project is still broken for everyone who started using it before rc9. It puzzles me why it hasn't been reverted yet. It happened exactly as I predicted many months ago - after carelessly merging a broken PR, AWS apparently abandoned the project, with no comments, no action has been taken, and now more and more people are starting to the broken rc9 version. The resolution should be: - Revert all changes since rc8. - Add tests to validate the exact behavior expected by rc8. - Start reintroducing any new features as long as they don't break the existing behavior. Alternatively: - Revert all changes since rc8 - Bump version to 2.0.0-rc1 - Reapply the breaking changes I'd say anything other than that is just unacceptable.

[badgerspoke]

Thanks for responding @bfreis. Wouldn't it be impossible to revert the changes you mention without negatively affecting anyone who's started using the project since rc9 (just as you were when they were introduced)? This is the crux, how to move forward without breaking anything/anyone else. I also can't see how a new flag to switch to old behaviour can be made the default as that has the same effect: breaking things for folks on rc9+.

I understand your frustration at the situation and how we got here, but I can't see how the project can be reverted to the old way with the current version; and this isn't my project but I'd like to use it and I'd like PRs to be accepted so it improves, therefore I'm trying to help.

I think you make a good suggestion about versions, which perhaps I can expand on:

• release v1.0.0-rc.8 as v1.0.0 with suitable readme, release notes about the resulting SSO group names and filtering logic
• re-tag v1.0.0-rc.9 (cb8a8aa ) as v2.0.0-rc.0 (drop tag v1.0.0-rc.9, is that really bad?) with suitable docs update to reflect that it once was rc9 and that it contains breaking changes (SSO group name change highlighted in bold red)
• implement the flag suggested above --sso-group-name-field defaulting to name (rc9 behaviour)
• accept PR45 and PR47 since they fix real issues seen in the wild
• tag head as 2.0.0-rc1

But either way this tool desperately needs a dry-run mode so we can have a way to preview impact, and more tests.

[bfreis]

Wouldn't it be impossible to revert the changes you mention without negatively affecting anyone who's started using the project since rc9 (just as you were when they were introduced)?

Yes, it would be impossible. After all, rc9 introduced a breaking change. Anyone who started depending on the broken behavior would be affected by its fix.

This is the crux, how to move forward without breaking anything/anyone else.

It isn't possible.

You have to choose whether you want people who depended on the original behavior or on the broken behavior to have to deal with a lot of headaches. Specifically, updating their automation, testing to see if the version they'll be forced into won't break their environments, and finally fix their environments when they realize it actually broke stuff. Or you exclude that group from ever updating.

In other words: there's no way around it, you have to pick a side.

I also can't see how a new flag to switch to old behaviour can be made the default as that has the same effect: breaking things for folks on rc9+.

You seem to want to favor the group of people who started depending on the broken behavior.

but I can't see how the project can be reverted to the old way with the current version

It's easy: I sent a PR doing exactly that ages ago, but it seems that AWS had decided to completely abandon the project just before that.

drop tag v1.0.0-rc.9, is that really bad?

Yes, rc9 it is really that bad, and should be dropped. It broke -rc.8 in ways that are absolutely unexpected. Someone currently using -rc.8 or before that decides to upgrade to -rc.9+ will be surprised when they see their environment completely destroyed.

I'd like PRs to be accepted so it improves, therefore I'm trying to help

Yeah, I forked, and gave up on that. AWS seems to have completely abandoned the project after the mess they did by accepting the breaking change.

There was a bit of hope when they said they'd revert back to rc8 if the breaking behavior wasn't fixed quickly. But it's been 6 months, it wasn't fixed, and it wasn't reverted.

But either way this tool desperately needs a dry-run mode so we can have a way to preview impact, and more tests.

It also needs a lot more testing, and incredibly more careful strategy for maintenance, to avoid merging absolutely unacceptable breaking changes in "rc.x bumps".

[badgerspoke]

Look, we are where we are. Blame whomever you like, but that's done now. Folks on rc8 can't upgrade so have to fork and/or go their own way.

I would argue that there's likely a finite set of ppl using rc8, and a growing set of ppl using rc9 though I don't know which is the greater number. What you appear to want (to revert to rc8 behaviour for the default) isn't possible now. It just isn't. They can't accept your PR now that time has passed and folks are using rc9. Yes I favour the status quo since that's the point where I've come in, and so to me it's not broken behaviour, just behaviour.

There's no point being grumpy about it, what you are asking for isn't realistic. So I'm trying to find a potential path forward so this becomes maintained again. But I'm not gonna try forever; If folks wanna get entrenched or unwilling to compromise (no reverting to rc8 isn't a compromise, it's another breaking change at this point) I'll politely decline to help and we'll have to fork and go our own way too. But loads of ppl forking and going solo isn't really how this stuff is supposed to work, is it?

I don't think it's that hard to reach a compromise and move on with our lives. Ok so now I'm grumpy too apparently.

[bfreis]

Blame whomever you like, but that's done now.

I blame the author of the breaking change, and the maintainer who merged it and then abandoned the project before addressing the issue.

What you appear to want (to revert to rc8 behaviour for the default) isn't possible now.

It absolutely is! It's just not convenient for you and others who started depending on broken behavior. Just like it isn't convenient to move past rc8 for me or others who started using this long before the breaking change was introduced, many of whom have been trying to get it addressed for the past 6 months (e.g., see my prediction: #40 (comment)).

I don't think it's that hard to reach a compromise and move on with our lives.

It really is that hard, unfortunately: there's no middle ground. This shows just how bad the breaking change was.

EDIT —

Ok so now I'm grumpy too apparently.

LOL! Yeah, that's what breaking changes do to us 🤣

[badgerspoke]

Can we agree to stop calling it broken behaviour? - that's pretty subjective.

So this feels like an impasse. I'm suggesting that we call rc8 behaviour version 1.something, and rc9 behaviour as version 2.something as you kinda suggested, signifying the significant difference in behaviour. Would you be ok with that?

I guess my point is that whatever has been done in the past 6 months hasn't achieved anything so a new approach is required, no? Making your PR48 a hard requirement is understandable but hasn't happened in that time, and yeah will then be a 'breaking change' for the likes of me. Is your plan to keep arguing for that, forever?

[bfreis]

Can we agree to stop calling it broken behaviour? - that's pretty subjective.

I can't agree to that. It's not at all subjective to anyone on 1.0.0-rc.8 or prior versions: if you upgrade to 1.0.0-rc.9, it breaks. That is, the behavior is broken in the 1.x major version; there's no other way to call it, and I see no reason to sugarcoat it.

So this feels like an impasse. I'm suggesting that we call rc8 behaviour version 1.something, and rc9 behaviour as version 2.something as you kinda suggested, signifying the significant difference in behaviour. Would you be ok with that?

I'm fine with that approach (I proposed it, right?): a major version bump does signify breaking changes, which it is. It will be clear to anyone on v1.0.0-rc.8 that it's the end of the line, or that breaking changes will have to be dealt with. No one would then accidentally end up with broken behavior on their hands (like I did, which made me grumpy).

I guess my point is that whatever has been done in the past 6 months hasn't achieved anything so a new approach is required, no?

It seems at this point that the main issue is that the project has been abandoned. The maintainer disappeared right after the broken PR got merged and all this mess started. There's no one else around to maintain it now.

Is your plan to keep arguing for that, forever?

Nope — just until v1.* behavior is fixed (e.g. reverting rc.9, or dropping the rc.9 tag and calling it v2).

Either way, when I get notifications on this topic of new comments, like yours, I'll reply to them.

[badgerspoke]

Ok so we need someone from AWS/a project maintainer to get involved here and do some driving (pretty please).

@joshuachong if you're no longer involved/interested will you please find someone your side who is/can be.