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

improve saml config options #379

Merged
merged 3 commits into from
May 26, 2023

Conversation

petrjasek
Copy link
Member

we can set on company email domain which will be tested when authenticating via saml and if user email matches it it will assign the company to user.

there is also new config for custom client saml configs which allows us to configure user specific login page which picks the config to be used for SAML auth. then it uses the company domain to match the config value.

STTNHUB-222

we can set on company email domain which will be tested
when authenticating via saml and if user email matches it
it will assign the company to user.

there is also new config for custom client saml configs
which allows us to configure user specific login page which
picks the config to be used for SAML auth. then it uses
the company domain to match the config value.

STTNHUB-222
@petrjasek petrjasek requested a review from MarkLark86 May 25, 2023 15:10
@petrjasek petrjasek added this to the v2.5 milestone May 25, 2023
Copy link
Contributor

@MarkLark86 MarkLark86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, only a few small suggestions

@@ -81,6 +81,14 @@ export function EditCompanyDetails({company, companyTypes, users, errors, onChan
error={errors ? errors.contact_email : null}
/>

<TextInput
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we hide this input if SSO is not enabled, as there would be no need to show an input that has no affect.

@@ -255,3 +255,11 @@ def set_locale():
if locale and locale in app.config["LANGUAGES"]:
flask.session["locale"] = locale
return flask.redirect(flask.url_for("auth.login"))


@blueprint.route("/login/<client>", methods=["GET"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be better off in newsroom.auth.saml file instead, so we're not adding an endpoint that has no use if SSO is not enabled?

<div class="card-body pt-1">
<form class="form" role="form" method="post" action="{{ url_for('auth.set_locale') }}">
<div class="form-group">
<label for="language">{{ gettext('Language') }}</label>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id attribute matching this for is missing on the select tag

@petrjasek petrjasek merged commit b9ce268 into superdesk:develop May 26, 2023
@petrjasek petrjasek deleted the feat-saml-auth-domain branch May 26, 2023 13:12
@petrjasek petrjasek modified the milestones: v2.5, v2.3.3 Jun 1, 2023
petrjasek added a commit to petrjasek/newsroom-core that referenced this pull request Jun 1, 2023
* improve saml config options

we can set on company email domain which will be tested
when authenticating via saml and if user email matches it
it will assign the company to user.

there is also new config for custom client saml configs
which allows us to configure user specific login page which
picks the config to be used for SAML auth. then it uses
the company domain to match the config value.

STTNHUB-222
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants