Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[redhat-3.11] oidc: ask for group object id for azure oauth login (PROJQUAY-6917) #2837

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions app.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,7 @@ def _request_end(resp):
billing = Billing(app)
sentry = Sentry(app)
build_logs = BuildLogs(app)
authentication = UserAuthentication(app, config_provider, OVERRIDE_CONFIG_DIRECTORY)
userevents = UserEventsBuilderModule(app)
usermanager = UserManager(app, authentication)
instance_keys = InstanceKeys(app)
label_validator = LabelValidator(app)
build_canceller = BuildCanceller(app)
Expand All @@ -264,6 +262,9 @@ def _request_end(resp):
oauth_login = OAuthLoginManager(app.config)
oauth_apps = [github_trigger, gitlab_trigger]

authentication = UserAuthentication(app, config_provider, OVERRIDE_CONFIG_DIRECTORY, oauth_login)
usermanager = UserManager(app, authentication)

image_replication_queue = WorkQueue(app.config["REPLICATION_QUEUE_NAME"], tf, has_namespace=False)
dockerfile_build_queue = WorkQueue(
app.config["DOCKERFILE_BUILD_QUEUE_NAME"], tf, has_namespace=True
Expand Down
46 changes: 25 additions & 21 deletions data/users/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from data.users.externaloidc import OIDCUsers
from data.users.federated import FederatedUsers
from data.users.keystone import get_keystone_users
from oauth.oidc import OIDCLoginService
from util.config.superusermanager import ConfigUserManager
from util.security.aes import AESCipher
from util.security.secret import convert_secret_key
Expand Down Expand Up @@ -45,7 +46,7 @@ def get_federated_service_name(authentication_type):
LDAP_CERT_FILENAME = "ldap.crt"


def get_users_handler(config, _, override_config_dir):
def get_users_handler(config, _, override_config_dir, oauth_login):
"""
Returns a users handler for the authentication configured in the given config object.
"""
Expand Down Expand Up @@ -139,38 +140,41 @@ def get_users_handler(config, _, override_config_dir):

return AppTokenInternalAuth()

if authentication_type == "OIDC":
client_id = config.get("CLIENT_ID")
client_secret = config.get("CLIENT_SECRET")
oidc_server = config.get("OIDC_SERVER")
service_name = config.get("SERVICE_NAME")
login_scopes = config.get("LOGIN_SCOPES")
preferred_group_claim_name = config.get("PREFERRED_GROUP_CLAIM_NAME")

return OIDCUsers(
client_id,
client_secret,
oidc_server,
service_name,
login_scopes,
preferred_group_claim_name,
)
if authentication_type == "OIDC" and oauth_login:
for service in oauth_login.services:
if isinstance(service, OIDCLoginService):
config = service.config
client_id = config.get("CLIENT_ID", None)
client_secret = config.get("CLIENT_SECRET", None)
oidc_server = config.get("OIDC_SERVER", None)
service_name = config.get("SERVICE_NAME", None)
login_scopes = config.get("LOGIN_SCOPES", None)
preferred_group_claim_name = config.get("PREFERRED_GROUP_CLAIM_NAME", None)

return OIDCUsers(
client_id,
client_secret,
oidc_server,
service_name,
login_scopes,
preferred_group_claim_name,
)

raise RuntimeError("Unknown authentication type: %s" % authentication_type)


class UserAuthentication(object):
def __init__(self, app=None, config_provider=None, override_config_dir=None):
def __init__(self, app=None, config_provider=None, override_config_dir=None, oauth_login=None):
self.secret_key = None
self.app = app
if app is not None:
self.state = self.init_app(app, config_provider, override_config_dir)
self.state = self.init_app(app, config_provider, override_config_dir, oauth_login)
else:
self.state = None

def init_app(self, app, config_provider, override_config_dir):
def init_app(self, app, config_provider, override_config_dir, oauth_login):
self.secret_key = convert_secret_key(app.config["SECRET_KEY"])
users = get_users_handler(app.config, config_provider, override_config_dir)
users = get_users_handler(app.config, config_provider, override_config_dir, oauth_login)

# register extension with app
app.extensions = getattr(app, "extensions", {})
Expand Down
7 changes: 7 additions & 0 deletions data/users/externaloidc.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import json
import logging
from urllib.parse import urlparse

import app
from data.model import InvalidTeamException, UserAlreadyInTeam, team
Expand Down Expand Up @@ -30,6 +31,12 @@ def __init__(
self._preferred_group_claim_name = preferred_group_claim_name
self._requires_email = requires_email

def service_metadata(self):
for service in app.oauth_login.services:
if isinstance(service, OIDCLoginService):
return {"issuer_domain": urlparse(service.get_issuer()).netloc}
return {}

def is_superuser(self, username: str):
"""
Initiated from FederatedUserManager.is_superuser(), falls back to ConfigUserManager.is_superuser()
Expand Down
27 changes: 19 additions & 8 deletions endpoints/oauth/test/test_login.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from data import database, model
from data.users import DatabaseUsers, UserAuthentication, get_users_handler
from endpoints.oauth.login import _conduct_oauth_login
from oauth.loginmanager import OAuthLoginManager
from oauth.oidc import OIDCLoginService
from oauth.services.github import GithubOAuthService
from test.analytics import analytics
Expand Down Expand Up @@ -38,12 +39,13 @@ def oidc_login_service(app):


@pytest.fixture(params=["Database", "LDAP", "OIDC"])
def auth_system(request):
return _get_users_handler(request.param)
def auth_system(request, oidc_login_service):
return _get_users_handler(request.param, oidc_login_service)


def _get_users_handler(auth_type):
def _get_users_handler(auth_type, oidc_login_service):
config = {}
oauth_login = OAuthLoginManager(config)
if auth_type == "LDAP":
config["AUTHENTICATION_TYPE"] = auth_type
config["LDAP_BASE_DN"] = ["dc=quay", "dc=io"]
Expand All @@ -60,7 +62,9 @@ def _get_users_handler(auth_type):
config["LOGIN_SCOPES"] = ["openid", "roles"]
config["PREFERRED_GROUP_CLAIM_NAME"] = "groups"

return get_users_handler(config, None, None)
oauth_login.services.append(oidc_login_service)

return get_users_handler(config, None, None, oauth_login)


def test_existing_account(app, auth_system, login_service):
Expand Down Expand Up @@ -209,7 +213,7 @@ def test_new_account_via_ldap(binding_field, lid, lusername, lemail, expected_er
config["GITHUB"]["LOGIN_BINDING_FIELD"] = binding_field

external_auth = GithubOAuthService(config, "GITHUB")
internal_auth = _get_users_handler("LDAP")
internal_auth = _get_users_handler("LDAP", None)

with mock_ldap():
# Conduct OAuth login.
Expand Down Expand Up @@ -254,7 +258,7 @@ def test_existing_account_in_ldap(app):
config = {"GITHUB": {"LOGIN_BINDING_FIELD": "username"}}

external_auth = GithubOAuthService(config, "GITHUB")
internal_auth = _get_users_handler("LDAP")
internal_auth = _get_users_handler("LDAP", None)

# Add an existing federated user bound to the LDAP account associated with `someuser`.
bound_user = model.user.create_federated_user(
Expand Down Expand Up @@ -370,7 +374,14 @@ def test_existing_account_in_ldap(app):
],
)
def test_new_account_via_oidc(
binding_field, lid, lusername, lemail, additional_login_info, expected_error, app
binding_field,
lid,
lusername,
lemail,
additional_login_info,
expected_error,
app,
oidc_login_service,
):
existing_user_count = database.User.select().count()

Expand All @@ -379,7 +390,7 @@ def test_new_account_via_oidc(
config["GITHUB"]["LOGIN_BINDING_FIELD"] = binding_field

external_auth = GithubOAuthService(config, "GITHUB")
internal_auth = _get_users_handler("OIDC")
internal_auth = _get_users_handler("OIDC", oidc_login_service)

result = _conduct_oauth_login(
app.config,
Expand Down
7 changes: 7 additions & 0 deletions test/test_external_oidc.py
Original file line number Diff line number Diff line change
Expand Up @@ -313,3 +313,10 @@ def test_verify_credentials(discovery_handler, token_handler_password_grant, use
assert result.email == "foo@example.com"
assert result.id == "cooluser"
assert error_msg is None


def test_service_metadata(discovery_handler, token_handler_password_grant, userinfo_handler):
oidc_instance = OIDCAuthTests().fake_oidc()
with HTTMock(discovery_handler, token_handler_password_grant, userinfo_handler):
result = oidc_instance.service_metadata()
assert result["issuer_domain"] == "fakeoidc"
25 changes: 25 additions & 0 deletions web/cypress/e2e/team-sync.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,13 @@ describe('OIDC Team Sync', () => {
}).as('getPrototypes');
cy.intercept('GET', '/api/v1/organization/teamsyncorg/aggregatelogs?*', {
aggregated: [],
}).as('getAggregateLogs');
cy.intercept('GET', '/api/v1/organization/teamsyncorg/logs?*', {
start_time: '',
end_time: '',
logs: [],
}).as('getLogs');

cy.intercept(
'GET',
'/api/v1/organization/teamsyncorg/team/testteam/members?includePending=true',
Expand All @@ -43,6 +49,9 @@ describe('OIDC Team Sync', () => {
cy.get('#team-members-toolbar').within(() =>
cy.get('button:contains("Enable Team Sync")').click(),
);
cy.get('#directory-sync-modal').contains(
"Enter the group name you'd like to sync membership with:",
);
cy.get('button:contains("Enable Sync")').should('be.disabled');
cy.get('#directory-sync-modal')
.find('input[id="team-sync-group-name"]')
Expand Down Expand Up @@ -218,4 +227,20 @@ describe('OIDC Team Sync', () => {
);
cy.get('button[data-testid="admin-delete-icon"]').should('not.exist');
});

it('Verify oidc azure login modal', () => {
cy.intercept(
'GET',
'/api/v1/organization/teamsyncorg/team/testteam/members?includePending=true',
{
fixture: 'teamsync-azure.json',
},
).as('getAzureTeamMembers');
cy.visit('/organization/teamsyncorg/teams/testteam?tab=Teamsandmembership');
cy.wait('@getAzureTeamMembers');
cy.get('button:contains("Enable Team Sync")').click();
cy.get('#directory-sync-modal').contains(
"Enter the group Object Id you'd like to sync membership with:",
);
});
});
9 changes: 9 additions & 0 deletions web/cypress/fixtures/teamsync-azure.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "testteam",
"members": [],
"can_edit": true,
"can_sync": {
"service": "oidc",
"issuer_domain": "login.microsoftonline.com"
}
}
3 changes: 2 additions & 1 deletion web/cypress/fixtures/teamsync-team-members.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
],
"can_edit": true,
"can_sync": {
"service": "oidc"
"service": "oidc",
"issuer_domain": "keycloakdev.apps.quaytest.com"
}
}
1 change: 1 addition & 0 deletions web/src/hooks/UseMembers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ export interface ITeamMember {

export interface ITeamMembersCanSyncResponse {
service: string;
issuer_domain?: string;
}

export interface ITeamMembersSyncedResponse {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,9 @@ export default function ManageMembersList(props: ManageMembersListProps) {
onConfirmSync={(groupName) =>
enableTeamSync(groupName, teamCanSync?.service)
}
secondaryText="Enter the name of the group you'd like sync membership with:"
secondaryText={`Enter the group ${
teamCanSync?.issuer_domain?.includes('microsoft') ? `Object Id` : `name`
} you'd like to sync membership with:`}
alertText={`Please note that once team syncing is enabled, the membership of users who are already part of the team will be revoked. OIDC group will be the single source of truth. This is a non-reversible action. Team's user membership from within ${config?.config.REGISTRY_TITLE_SHORT} will be read-only.`}
/>
);
Expand Down