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

Allow selection of secondary OIDC provider #1824

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

sbreker
Copy link
Member

@sbreker sbreker commented Jun 11, 2024

Add the ability to define a secondary OIDC provider for authentication and select it using a query param on the request URL.

E.g. Use the 'sample_provider' provider by modifying the AtoM URL before pressing "Log in with SSO":

http://127.0.0.1:63001/index.php?secondary=sample_provider

@sbreker sbreker requested a review from a team June 11, 2024 21:25
@sbreker sbreker force-pushed the dev/oidc-allow-secondary-provider branch from 3e0b828 to 34c1f5a Compare June 11, 2024 21:29
Copy link
Contributor

@anvit anvit left a comment

Choose a reason for hiding this comment

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

I think I found a couple minor lines of code that were accidentally left in, but everything else looks great!

{
$authenticated = false;
$user = null;
$authenticateResult = false;
$email = null;

$this->logger->err(sprintf('%s', json_encode(sfConfig::getAll())));
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a debug message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is! 😆 🤦

Thanks for catching this! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed - see latest commit

} catch (Exception $e) {
$this->setProviderId();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this repeated function call inside the catch a typo?

Copy link
Member Author

@sbreker sbreker Jun 19, 2024

Choose a reason for hiding this comment

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

I was trying to ensure that setProviderId() would be run even if there was an exception generated in signOut().

I am wondering now if I could just move getSessionProviderID() and setProviderId() to before the try{}. I will have a closer look.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated and tested. See latest commit.

@sbreker sbreker force-pushed the dev/oidc-allow-secondary-provider branch from 6b0d6fa to 0d11646 Compare June 20, 2024 06:30
@sbreker sbreker requested a review from anvit June 20, 2024 06:33
Copy link
Contributor

@anvit anvit left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @sbreker

@sbreker sbreker force-pushed the dev/oidc-allow-secondary-provider branch from 0d11646 to f813789 Compare June 20, 2024 23:25
Add the ability to define a secondary OIDC provider for authentication
and select it using a query param on the request URL.

E.g. Use the 'sample_provider' provider by modifying the AtoM URL before
pressing "Log in with SSO":

http://127.0.0.1:63001/index.php?secondary=sample_provider
@sbreker sbreker force-pushed the dev/oidc-allow-secondary-provider branch from f813789 to 52d7f01 Compare June 20, 2024 23:26
@sbreker sbreker merged commit 52d7f01 into qa/2.x Jun 20, 2024
5 of 6 checks passed
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