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

Fixes #21987 - New UI for auth sources #5560

Closed
wants to merge 1 commit into from

Conversation

dhlavac
Copy link
Member

@dhlavac dhlavac commented May 15, 2018

This PR changes Administration for Authentication sources

Now is by default added External auth source to database. In setting and for running systems by migration.
Every authentication source have card and display number of users set up by authentication sources

Administer page for Authentication sources
jhz1jwx

Administer page for Authentication sources with no LDAPs
auth_sources_empty

Drop-downs for External and LDAP authentication sources
auth_sources_with_external

@theforeman-bot
Copy link
Member

Issues: #21987

test/integration/auth_source_test.rb Outdated Show resolved Hide resolved
app/helpers/auth_sources_helper.rb Outdated Show resolved Hide resolved
app/helpers/auth_sources_helper.rb Outdated Show resolved Hide resolved
app/controllers/auth_sources_controller.rb Outdated Show resolved Hide resolved
app/controllers/auth_source_ldaps_controller.rb Outdated Show resolved Hide resolved
app/controllers/auth_source_externals_controller.rb Outdated Show resolved Hide resolved
Copy link
Member

@amirfefer amirfefer left a comment

Choose a reason for hiding this comment

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

Thanks @dhlavac, looks great!
I'd like to see this card view as a react component.
there are patternfly-react card and kebab components already.

When a pull request is still in progress, you can add [WIP] on the PR title.

@ohadlevy
Copy link
Member

I too would love to see it in react, but maybe prior to that we should review the UX with @Rohoover as well? I could imagine different views (besides tables) that might be useful here?

@ares
Copy link
Member

ares commented May 16, 2018

This is the outcome of discussion with @Rohoover, it's a midterm solution, until we have org/loc association per external hostgroup, until them we need table for ldap auth sources, as users need to create many of them in multiorg scenario. After that's done, auth sources should be cards only.

@ohadlevy
Copy link
Member

ohadlevy commented May 16, 2018 via email

@ares
Copy link
Member

ares commented May 18, 2018

I don't recall if the meeting back then was recorded. Here's the motivation, today we can setup mapping between external auth sources and user groups, user groups can have roles assigned. With external user groups mapping, administrators can manage permissions externally to Foreman, only by assigning user groups in their LDAP. This works fine for permissions but not for ogranizations and locations, which is our secondary authorization system. For that we added orgs/locs associations to ldap auth sources. When user account is created, it gets associated to orgs/locs the LDAP is assigned to.

The problem is that if I want to have single LDAP for multiple organizations, each use belonging to different one, the model does not fit. People workaround that by having multiple LDAP definitions for the same LDAP server but with different filter and organization assigned. That means easily >10 LDAP authsources in env with multiple orgs.

We started adding the same model to external auth source (using just REMOTE_USER) but in order to do it, we had to add API and UI for manipulating it's org/loc association. As part of that, @dhlavac adds UI for it. This is a temporary solution, where we'll still have table for ldap authsources (as there can be a lot of them because of ^) and cards for internal and external (both have single instance). Later we want to add org/loc association also to external user group which should dramatically decrease the number of LDAP auth sources defined here and at that point we'd like to convert the table to cards. So at the end, all auth source instances should be represented by card.

Hope that puts more light on the change. When @dhlavac started working on this, there were no cards implemented and patternfly roadmap was unclear about this. Since it was few months ago, I think we can revisit the decision. Though I'm not sure I'd block this PR because of that single card partial. It's not the crucial part of it. But I'd definitely support the effort that would follow right after this gets in, so that we'll get card implemented as a react component if @dhlavac agrees.

test/integration/auth_source_test.rb Outdated Show resolved Hide resolved
test/integration/auth_source_test.rb Outdated Show resolved Hide resolved
test/integration/auth_source_test.rb Outdated Show resolved Hide resolved
test/integration/auth_source_test.rb Outdated Show resolved Hide resolved
test/controllers/auth_source_externals_controller_test.rb Outdated Show resolved Hide resolved
app/helpers/auth_sources_helper.rb Outdated Show resolved Hide resolved
@dhlavac dhlavac force-pushed the feature/21987 branch 3 times, most recently from 204d262 to 5da939c Compare October 8, 2018 16:05
@dhlavac
Copy link
Member Author

dhlavac commented Oct 22, 2018

@ares PR is ready I hope. @ohadlevy @amirfefer If it isn't problem, for now I will let card component in erb. It's a midterm solution, until we have org/loc association per external hostgroup and then implement react component for cards.

@ares
Copy link
Member

ares commented Nov 13, 2018

There seems to be several hound/rubocop issues, mind to rebase and fix them?

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

rubocop failures, rebase would also be good (no conflicts)

@dhlavac dhlavac force-pushed the feature/21987 branch 2 times, most recently from ad06896 to 5301b89 Compare November 26, 2018 12:49
@ares
Copy link
Member

ares commented Nov 30, 2018

this needs to be rebased

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

couple of new findings, please take a look

app/models/auth_source.rb Outdated Show resolved Hide resolved
app/views/auth_source_ldaps/_form.html.erb Outdated Show resolved Hide resolved
app/views/auth_sources/_card.html.erb Outdated Show resolved Hide resolved
app/views/auth_sources/index.html.erb Show resolved Hide resolved
app/views/auth_sources/index.html.erb Show resolved Hide resolved
@dhlavac
Copy link
Member Author

dhlavac commented Dec 19, 2018

@ares Changes done

@dhlavac
Copy link
Member Author

dhlavac commented Jan 16, 2019

@ares Anything else what can I do on this PR ?

@mmoll
Copy link
Contributor

mmoll commented May 4, 2019

@dhlavac please rebase :)
@ares any more comments?

@dhlavac
Copy link
Member Author

dhlavac commented May 15, 2019

@mmoll @ares rebased

@theforeman-bot
Copy link
Member

@dhlavac, this pull request is currently not mergeable. Please rebase against the develop branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream develop

This message was auto-generated by Foreman's prprocessor

@mmoll
Copy link
Contributor

mmoll commented Nov 11, 2019

@dhlavac This needs a rebase :)

@dhlavac
Copy link
Member Author

dhlavac commented Nov 11, 2019

@mmoll I don't know if it is still planned to use this feature. @ares should I rebase that or close PR ?

@ezr-ondrej
Copy link
Member

@dhlavac if you're not plannig to continue I was planning to take over. I do not know when I will get to it though.

@tbrisker
Copy link
Member

closing in favor of #7206

@tbrisker tbrisker closed this Nov 25, 2019
@tbrisker
Copy link
Member

Thanks for the effort here @dhlavac !

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

Successfully merging this pull request may close these issues.

10 participants