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.

Sign up for GitHub

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

Jump to bottom

Add single sign on to sulu admin #3

Merged
merged 5 commits into from
Mar 14, 2024
Merged

Add single sign on to sulu admin #3

merged 5 commits into from
Mar 14, 2024

Conversation

martinlagler
Copy link

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets fixes #issuenum
Related issues/PRs #issuenum
License MIT
Documentation PR sulu/sulu-docs#prnum

What's in this PR?

Explain the contents of the PR.

Why?

Which problem does the PR fix? (remove this section if you linked an issue above)

Example Usage

// If you added new features, show examples of how to use them here
// (remove this section if not a new feature)

$foo = new Foo();

// Now we can do
$foo->doSomething();

BC Breaks/Deprecations

Describe BC breaks/deprecations here. (remove this section if not needed)

To Do

  • Create a documentation PR
  • Add breaking changes to UPGRADE.md

@martinlagler martinlagler force-pushed the feature/single-sign-on-frontend branch 3 times, most recently from 5f740e3 to 7a330fe Compare March 8, 2024 08:29
@martinlagler martinlagler force-pushed the feature/single-sign-on-frontend branch from 7a330fe to 627346e Compare March 11, 2024 10:09
@alexander-schranz alexander-schranz changed the title Add frontend for single sign on Add single sign on to sulu admin Mar 12, 2024
alexander-schranz
alexander-schranz requested changes Mar 12, 2024
View reviewed changes
config/routes/sulu_admin.yaml Outdated
Comment on lines 117 to 121
sulu_admin_single_sign_on:
path: /openid
controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
defaults:
route: sulu_admin
Copy link
Owner

Choose a reason for hiding this comment

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

This should be in the security bundle as sulu_security.single_sign_on

https://github.com/sulu/sulu/blob/2.5/src/Sulu/Bundle/SecurityBundle/Resources/config/routing.yml

src/Sulu/Bundle/AdminBundle/Controller/AdminController.php Outdated
Comment on lines 3 to 4
declare(strict_types=1);

Copy link
Owner

Choose a reason for hiding this comment

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

currently we do not add strict types to the sulu code base as it is a backwards compatibility break:

Suggested change
declare(strict_types=1);

src/Sulu/Bundle/AdminBundle/Resources/js/components/Input/Input.js Outdated
@@ -159,6 +161,7 @@ export default class Input<T: ?string | ?number> extends React.PureComponent<Inp
ref={inputRef ? this.setInputRef : undefined}
step={step}
type={type}
autoFocus={autoFocus}
Copy link
Owner

Choose a reason for hiding this comment

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

can we add a test for this new property? It looks like it currently not catched by the snapshot tests?

src/Sulu/Bundle/AdminBundle/Resources/js/containers/Login/Login.js Outdated
Comment on lines 142 to 143
hasSingleSignOn={userStore.hasSingleSignOn()}
hasOnlyPassword={userStore.hasJsonLogin}
Copy link
Owner

Choose a reason for hiding this comment

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

This two parameters conflicting to each other I may would go more with a single parameter enum based which maybe look something like this:

mode={userStore.hasSingleSignOn ? (userStore.hasJsonLogin ? 'password_only' : 'username_only') : 'username_password'}

with suggestion from above:

mode={userStore.hasSingleSignOn ? (userStore.loginMethod === 'json_login' ? 'password_only' : 'username_only') : 'username_password'}

src/Sulu/Bundle/AdminBundle/Resources/js/stores/userStore/userStore.js Outdated
@@ -30,6 +31,7 @@ class UserStore {
this.user = undefined;
this.contact = undefined;
this.loginError = false;
this.hasJsonLogin = false;
Copy link
Owner

Choose a reason for hiding this comment

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

hasJsonLogin is here a little bit confusing. I would go more with loginMethod 🤔

src/Sulu/Component/DocumentManager/Tests/Unit/Metadata/BaseMetadataFactoryTest.php Outdated
@@ -163,6 +163,6 @@ public function testAllMetadata(): void
$this->assertCount(2, $metadatas);
$this->assertContainsOnlyInstancesOf(Metadata::class, $metadatas);
$metadata = \reset($metadatas);
$this->assertEquals('Class\Page', $metadata->getClass());
$this->assertEquals('Class\Page', $metadata ? $metadata->getClass() : null);
Copy link
Owner

Choose a reason for hiding this comment

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

Tis looks unexpected change 🤔

src/Sulu/Component/DocumentManager/Tests/Unit/Metadata/BaseMetadataFactoryTest.php Outdated
@@ -163,6 +163,6 @@ public function testAllMetadata(): void
$this->assertCount(2, $metadatas);
$this->assertContainsOnlyInstancesOf(Metadata::class, $metadatas);
$metadata = \reset($metadatas);
$this->assertEquals('Class\Page', $metadata->getClass());
$this->assertEquals('Class\Page', $metadata ? $metadata->getClass() : null);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
$this->assertEquals('Class\Page', $metadata ? $metadata->getClass() : null);
$this->assertEquals('Class\Page', $metadata->getClass());

src/Sulu/Bundle/AdminBundle/Resources/js/containers/Login/Login.js Outdated
Comment on lines 73 to 91
if (userStore.hasJsonLogin) {
return;
}
Copy link
Owner

Choose a reason for hiding this comment

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

would like to check more for userStore.loginMethod == ''

src/Sulu/Bundle/AdminBundle/Resources/js/stores/userStore/userStore.js Outdated
@@ -106,6 +112,19 @@ class UserStore {
handleLogin = (data: Object) => {
this.setTwoFactorMethods([]);

if (data.method === 'redirect' && data.url) {
window.location.href = data.url;
Copy link
Owner

Choose a reason for hiding this comment

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

not sure about doing a redirect here 🤔

@martinlagler martinlagler force-pushed the feature/single-sign-on-frontend branch 5 times, most recently from ab28ad2 to e0be626 Compare March 13, 2024 07:31
@martinlagler martinlagler force-pushed the feature/single-sign-on-frontend branch from e0be626 to 095b60a Compare March 13, 2024 08:36
@martinlagler martinlagler force-pushed the feature/single-sign-on-frontend branch 2 times, most recently from ab0d22b to 7869645 Compare March 14, 2024 13:20
@martinlagler martinlagler force-pushed the feature/single-sign-on-frontend branch from 7869645 to 3d48921 Compare March 14, 2024 13:27
@alexander-schranz alexander-schranz merged commit b67b8ac into alexander-schranz:feature/signle-sign-on Mar 14, 2024
8 checks passed
alexander-schranz pushed a commit that referenced this pull request Mar 14, 2024
@martinlagler @alexander-schranz
Add single sign on to sulu admin (#3)
d3608e4
@alexander-schranz alexander-schranz mentioned this pull request Mar 14, 2024
Closed
13 tasks
alexander-schranz added a commit that referenced this pull request Oct 17, 2024
@alexander-schranz
Add Single Media Property Resolver and displayOption handling (#3)
69c006e
alexander-schranz pushed a commit that referenced this pull request Nov 14, 2024
@TheCadien @alexander-schranz
Add Article Selection Content Type (#3)
2a4a733
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-schranz alexander-schranz alexander-schranz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martinlagler @alexander-schranz