Skip to content

Commit

Permalink
removing oidc groupname restriction
Browse files Browse the repository at this point in the history
  • Loading branch information
@Sunandadadi
Sunandadadi committed Feb 26, 2024
1 parent fb5dca6 commit 834d370
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 114 deletions.
17 changes: 17 additions & 0 deletions data/model/team.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -597,3 +597,20 @@ def delete_all_team_members(team):
return query.execute()

return 0


def get_oidc_team_from_groupname(group_name, login_service_name):
"""
Fetch TeamSync row synced with login_service_name from `group_name` in TeamSync.config
"""
with db_transaction():
query_result = (
TeamSync.select()
.join(LoginService)
.where(TeamSync.config.contains(group_name), LoginService.name == login_service_name)
)
for row in query_result:
if json.loads(row.config).get("group_name", None) == group_name:
return row

return None
44 changes: 11 additions & 33 deletions data/users/externaloidc.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,6 @@ def query_users(self, query, limit):
"""
return ([], self.federated_service, None)

def fetch_org_team_from_oidc_group(self, oidc_group):
"""
OIDC group name is in the format - <org_name>:<group_name>
Extract and return org name and group name
"""
try:
org_name, group_name = oidc_group.split(":")
# TODO: verify that org_name and group_name exist here
return org_name, group_name
except (ValueError, AttributeError):
logger.exception(
f'Incorrect OIDC group name: {oidc_group}. The expected format is : "<org_name>:<team_name> "'
)

return None, None

def sync_oidc_groups(self, user_groups, user_obj, service_name):
"""
Adds user to quay teams that have team sync enabled with an OIDC group
Expand All @@ -81,26 +65,23 @@ def sync_oidc_groups(self, user_groups, user_obj, service_name):
return

for oidc_group in user_groups:
org_name, group_name = self.fetch_org_team_from_oidc_group(oidc_group)
if not org_name or not group_name:
continue

# verify if team is in TeamSync table
team_synced = team.get_team_sync_information(org_name, group_name)
# fetch TeamSync row if exists, for the oidc_group synced with the login service
team_synced = team.get_oidc_team_from_groupname(oidc_group, service_name)
if not team_synced:
logger.debug(f"OIDC group: {oidc_group} is not synced with a team in quay")
continue

# verify if team is synced with login service
if team_synced.service.name != service_name:
logger.debug(
f"OIDC group: {oidc_group} is not synced with the {service_name} service"
f"OIDC group: {oidc_group} is either not synced with a team in quay or is not synced with the {service_name} service"
)
continue

# fetch team name and organization name for the Teamsync row
team_name = team_synced.team.name
org_name = team_synced.team.organization.username
if not team_name or not org_name:
logger.debug(f"Cannot retrieve team for the oidc group: {oidc_group}")

# add user to team
try:
team_obj = team.get_organization_team(org_name, group_name)
team_obj = team.get_organization_team(org_name, team_name)
team.add_user_to_team(user_obj, team_obj)
except InvalidTeamException as err:
logger.exception(err)
Expand All @@ -123,7 +104,6 @@ def resync_quay_teams(self, user_groups, user_obj, login_service_name):
# fetch user's quay teams that have team sync enabled
existing_user_teams = team.get_federated_user_teams(user_obj, login_service_name)
user_groups = user_groups or []

for user_team in existing_user_teams:
try:
sync_group_info = json.loads(user_team.teamsync.config)
Expand All @@ -132,9 +112,7 @@ def resync_quay_teams(self, user_groups, user_obj, login_service_name):
sync_group_info.get("group_name", None)
and sync_group_info["group_name"] not in user_groups
):
org_name, group_name = self.fetch_org_team_from_oidc_group(
sync_group_info["group_name"]
)
org_name = user_team.teamsync.team.organization.username
team.remove_user_from_team(org_name, user_team.name, user_obj.username, None)
logger.debug(
f"Successfully removed user: {user_obj.username} from team: {user_team.name} in organization: {org_name}"
Expand Down
38 changes: 9 additions & 29 deletions test/test_external_oidc.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,18 @@ def test_sync_for_non_empty_oidc_groups(self):
)

team_1 = model.team.create_team("team_1", test_org_1, "member")
assert model.team.set_team_syncing(team_1, "oidc", None)
assert model.team.set_team_syncing(team_1, "oidc", {"group_name": "test_org_1_team_1"})

team_2 = model.team.create_team("team_2", test_org_2, "member")
assert model.team.set_team_syncing(team_2, "oidc", None)
assert model.team.set_team_syncing(team_2, "oidc", {"group_name": "test_org_2_team_2"})

team_3 = model.team.create_team("team_3", test_org_3, "member")
assert model.team.set_team_syncing(team_3, "ldap", None)
assert model.team.set_team_syncing(team_3, "ldap", {"group_name": "test_org_3_team_3"})

user_groups = [
"test_org_1:team_1",
"test_org_2:team_2",
"test_org_3:team_3",
"test_org_1_team_1",
"test_org_2_team_2",
"test_org_3_team_3",
"wrong_group_name",
]
user_teams_before_sync = TeamMember.select().where(TeamMember.user == user_obj).count()
Expand Down Expand Up @@ -130,15 +130,15 @@ def test_resync_for_non_empty_quay_teams(self):

# add user to team that has team sync enabled with oidc login service
team_1 = model.team.create_team("team_1", test_org_1, "member")
assert model.team.set_team_syncing(team_1, "oidc", {"group_name": "test_org_1:team_1"})
assert model.team.set_team_syncing(team_1, "oidc", {"group_name": "test_org_1_team_1"})
assert model.team.add_user_to_team(user_obj, team_1)

# add user to another team that has team sync enabled with oidc login service
team_2 = model.team.create_team("team_1", test_org_2, "member")
assert model.team.set_team_syncing(team_2, "oidc", {"group_name": "test_org_2:team_1"})
assert model.team.set_team_syncing(team_2, "oidc", {"group_name": "test_org_2_team_1"})
assert model.team.add_user_to_team(user_obj, team_2)

user_groups = ["test_org_1:team_1", "another:group"]
user_groups = ["test_org_1_team_1", "another_group"]
# user should be removed from team_2
self.oidc_instance.resync_quay_teams(user_groups, user_obj, "oidc")
assert (
Expand All @@ -147,23 +147,3 @@ def test_resync_for_non_empty_quay_teams(self):
.count()
== 0
)


@pytest.mark.parametrize(
("name, expected_org_name, expected_group_name"),
[
("", None, None),
("f", None, None),
("foobar", None, None),
(1, None, None),
(256, None, None),
(" ", None, None),
("foo:bar", "foo", "bar"),
("foooooo:baaaaar", "foooooo", "baaaaar"),
],
)
def test_fetch_org_team_from_oidc_group(name, expected_org_name, expected_group_name):
obj = OIDCAuthTests()
org_name, group_name = obj.fake_oidc().fetch_org_team_from_oidc_group(name)
assert expected_org_name == org_name
assert expected_group_name == group_name
17 changes: 8 additions & 9 deletions web/cypress/e2e/team-sync.cy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,28 +38,27 @@ describe('OIDC Team Sync', () => {
cy.get('#team-members-toolbar').within(() =>
cy.get('button:contains("Enable Directory Sync")').click(),
);
cy.get('#oidc-team-sync-helper-text').contains(
'The expected OIDC group name format is - org_name:team_name. Must match ^[a-z0-9][a-z0-9]+:[a-z0-9]+$',
);
cy.get('button:contains("Enable Sync")').should('be.disabled');
cy.get('#directory-sync-modal')
.find('input[id="team-sync-group-name"]')
.type('random');
cy.get('button:contains("Enable Sync")').should('be.disabled');
cy.get('button:contains("Enable Sync")').should('not.be.disabled');

cy.get('#directory-sync-modal')
.find('input[id="team-sync-group-name"]')
.clear();

cy.get('#directory-sync-modal')
.find('input[id="team-sync-group-name"]')
.type('1:1');
.type(' ');
cy.get('button:contains("Enable Sync")').should('be.disabled');
cy.get('#directory-sync-modal')
.find('input[id="team-sync-group-name"]')
.clear();

cy.get('#directory-sync-modal')
.find('input[id="team-sync-group-name"]')
.type('org:team');
.type('team_name');
cy.get('button:contains("Enable Sync")').should('not.be.disabled');
});

Expand All @@ -77,7 +76,7 @@ describe('OIDC Team Sync', () => {
);
cy.get('#directory-sync-modal')
.find('input[id="team-sync-group-name"]')
.type('org:team');
.type('org_team_group');
cy.get('button:contains("Enable Sync")').click();
cy.wait('@getTeamSyncSuccess');
cy.contains('Successfully updated team sync config');
Expand All @@ -99,7 +98,7 @@ describe('OIDC Team Sync', () => {
).should('exist');
cy.contains('Directory Synchronization Config').should('exist');
cy.contains('Bound to group').should('exist');
cy.contains('teamsyncorg:groupname').should('exist');
cy.contains('testteam_teamsync_group').should('exist');
cy.contains('Last Updated').should('exist');
cy.contains('Never').should('exist');
cy.contains('tr', 'teamsyncorg+robotacct');
Expand Down Expand Up @@ -181,7 +180,7 @@ describe('OIDC Team Sync', () => {
cy.contains('button', 'Enable Directory Sync').click();
cy.get('#directory-sync-modal')
.find('input[id="team-sync-group-name"]')
.type('org:team');
.type('org_team_group');
cy.get('button:contains("Enable Sync")').click();
cy.wait('@getTeamSyncSuccess');
cy.contains('Successfully updated team sync config');
Expand Down
2 changes: 1 addition & 1 deletion web/cypress/fixtures/teamsynced-members-superuser.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"synced": {
"service": "oidc",
"config": {
"group_name": "teamsyncorg:groupname"
"group_name": "testteam_teamsync_group"
},
"last_updated": ""
}
Expand Down
44 changes: 2 additions & 42 deletions web/src/components/modals/OIDCTeamSyncModal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,47 +2,18 @@ import {useState} from 'react';
import {
Alert,
Button,
HelperText,
HelperTextItem,
Modal,
Text,
TextInput,
TextVariants,
} from '@patternfly/react-core';
import Conditional from 'src/components/empty/Conditional';
import ExclamationCircleIcon from '@patternfly/react-icons/dist/esm/icons/exclamation-circle-icon';

interface Validation {
message: string;
isValid: boolean;
type: 'default' | 'error';
}

const defaultMessage: Validation = {
message:
'The expected OIDC group name format is - org_name:team_name. Must match ^[a-z0-9][a-z0-9]+:[a-z0-9]+$',
isValid: false,
type: 'default',
};

export default function OIDCTeamSyncModal(props: OIDCTeamSyncModalProps) {
const [groupName, setGroupName] = useState<string>('');
const [validation, setValidation] = useState<Validation>(defaultMessage);

const handleInputChange = (value: string) => {
const regex = /^[a-z0-9][a-z0-9]+:[a-z0-9]+$/;
if (!regex.test(value)) {
setValidation({
message:
'The expected OIDC group name format is - org_name:team_name. Must match ^[a-z0-9][a-z0-9]+:[a-z0-9]+$',
isValid: false,
type: 'error',
});
} else {
defaultMessage['isValid'] = true;
setValidation(defaultMessage);
}
setGroupName(value);
setGroupName(value.trim());
};

return (
Expand All @@ -57,7 +28,7 @@ export default function OIDCTeamSyncModal(props: OIDCTeamSyncModalProps) {
key="confirm"
variant="primary"
onClick={() => props.onConfirmSync(groupName)}
isDisabled={!validation.isValid}
isDisabled={groupName == ''}
>
Enable Sync
</Button>,
Expand All @@ -77,18 +48,7 @@ export default function OIDCTeamSyncModal(props: OIDCTeamSyncModalProps) {
type="text"
onChange={(_event, value) => handleInputChange(value)}
id="team-sync-group-name"
validated={validation.type}
/>
<HelperText id="oidc-team-sync-helper-text">
<HelperTextItem
variant={validation.type}
{...(validation.type === 'error' && {
icon: <ExclamationCircleIcon />,
})}
>
{validation.message}
</HelperTextItem>
</HelperText>
</div>
<br />
<Conditional if={props.alertText != null}>
Expand Down

0 comments on commit 834d370

Please sign in to comment.