Fix no "Register" tab if ldap auth is enabled (#24038)

What does this MR do?

It shows a "Register" tab if LDAP auth is enabled

Are there points in the code the reviewer needs to double check?

No.

Why was this MR needed?

The "Register" tab disappeared in 8.13 version.

Screenshots

Before:

After:

Does this MR meet the acceptance criteria?

• CHANGELOG entry added
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Closes #24038 (closed)

Marked the task Squashed related commits together as completed

Marked the task Branch has no merge conflicts with master (if it does - rebase it please) as completed

Thanks @ldidry!

@brycepj do you mind taking a look at this?

Added ~164274 label

Milestone changed to %8.14

Reassigned to @brycepj

Added ~127616 ~149423 labels

Milestone changed to %8.13

Mentioned in issue #23101 (closed)

@ldidry thanks for doing this!

This LGTM -- but could you provide before & screenshots? I'll test manually myself, but I think it would good for others who review this.

Also, there are tests for this here (https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/features/login_spec.rb#L219)... do you mind updating the spec to cover this fix?

Mentioned in merge request !7195 (closed)

Screenshots added. I'll take a look at the tests soon.

Well… I don't know a thing about spec and how to update it to cover this fix and I can't figure out how to do it. @brycepj , do you have a manual or something to help me?

Mentioned in issue #23906 (closed)

@smcgivern I can now see why the existing tests didn't catch this -- they aren't actually enabling ldap. Do you know how I can do that?

@brycepj it looks like the specs are attempting to stub out the methods needed to pretend that LDAP is enabled - what's wrong with that case? I understand that something is wrong, I just don't understand what

@smcgivern yeah, same feeling here, and I wrote the specs! I noticed when I was inspecting the HTML screenshot for a test I forced to fail that all the tests enabling ldap don't show the tab or pane for ldapmain.

The only thing that needs to be truthy for ldap tabs and panes to be rendered is form_based_providers. If signin_enabled and signup_enabled are both false, we also need ldap_enabled to be true. And we're enabling both in the specs.

@smcgivern

I realized I need to be stubbing @ldap_servers here as well. (See the relevant view)

Here's my attempt at that (https://gitlab.com/gitlab-org/gitlab-ce/commit/0c47b95d3215d6def6e15afaeadc26b4f8eaeaed)

This spec should fail when ldap is properly enabled, because the signup tab should be missing. But I can't get it to fail because ldap isn't being sufficiently mocked. I'm not sure if that's because of my feeble Ruby skills, or something larger that I'm missing. Thoughts?

@dblessing -- I'm wondering if you have any insight here?

@brycepj and I got this spec failing

@ldidry thanks again for fixing this. Trying to write a test for this turned into a bigger task than I expected.

I can't push directly to your fork, but can you add the following to spec/features/login_spec.rb after the test for 'when crowd is enabled' (~ line 260) and commit/push it:

    context 'when ldap is enabled', js: true do
      before do
        stub_application_setting(signup_enabled: true)
        # Mock enable LDAP
        @ldap_servers = [{ provider_name: 'ldapmain', label: 'ldapmain' }]
        allow(page).to receive(:form_based_providers).and_return([:ldapmain])
        allow(page).to receive(:ldap_enabled).and_return(true)
        allow(page).to receive(:ldap_servers).and_return(@ldap_servers)
        allow(Gitlab.config.ldap).to receive(:enabled).and_return(true)
        allow(Gitlab::LDAP::Config).to receive(:servers).and_return(@ldap_servers)

        visit new_user_session_path
      end

      it 'correctly renders tabs and panes' do
        ensure_tab_pane_correctness(false)
      end
    end

You can also cherry-pick this commit onto your branch if you know what you're doing.

Thanks, and let me know if you run into any problems.

Added 1 commit:

• 5010d857 - Fix no "Register" tab if ldap auth is enabled (#24038 (closed))

Compare with previous version

Added 1 commit:

• 2bb39ba7 - Fix no "Register" tab if ldap auth is enabled (#24038 (closed))

Compare with previous version

@brycepj Done (added to my fork), thx for your test

@ldidry looks good -- let's cross our fingers that the build passes

If you want to test it locally, run this:

bundle exec rspec spec/features/login_spec.rb

If that passes we should be good.

Marked the task Added for this feature/bug as completed

I don't have a complete development environment, so I can't run the tests locally. I know that my patch works because I patched our production Gitlab server before forking.

Mmh, the tests failed: https://gitlab.com/ldidry/gitlab-ce/builds/5988037

Any idea?

@ldidry Agh... you're right... I just tested it locally, and the DOM is even worse than without this attempt at configuring LDAP. It looks like that solution isn't going to work.

I'm thinking the problem goes a bit beyond the scope of this MR. Given that I've tested this fix manually, and it's quite simple, but very important, I'd say we go ahead and get it merged, and create an issue for testing LDAP configurations better.

What do you think @smcgivern?

Mentioned in issue #24335 (moved)

@brycepj if you're happy with that, I am. Thanks for your help with this @ldidry.

You're welcome

I tried @dblessing's suggestion and I couldn't get it to work either.

@ldidry can you revert the change to login_spec? Sorry for all the back and forth on this.

Added 1 commit:

• 05e3a00e - Fix no "Register" tab if ldap auth is enabled (#24038 (closed))

Compare with previous version

Added 1 commit:

• 8bfcbba0 - Fix no "Register" tab if ldap auth is enabled (#24038 (closed))

Compare with previous version

No problem

It's done and building.

@ldidry thank you! @smcgivern are you able to merge, or should we assign to someone else?

Reassigned to @smcgivern

Enabled an automatic merge when the build for 8bfcbba0 succeeds

Thanks @ldidry, I've set to merge when the build passes! Sorry for all the back-and-forth

Oh no we got failed builds @smcgivern @ldidry

@ldidry actually, can you credit yourself in the changelog entry as well, as we have to restart the builds anyway? Thanks!

Reassigned to @ldidry

Added 374 commits:

• 8bfcbba0...87cc458a - 373 commits from branch gitlab-org:master
• 45c9d1e7 - Fix no "Register" tab if ldap auth is enabled (#24038 (closed))

Compare with previous version

Done and building.

Enabled an automatic merge when the build for 45c9d1e7 succeeds

Reassigned to @smcgivern

Mentioned in commit 3e089d7f

Status changed to merged

Mentioned in issue #24326 (closed)

Picked into 8-13-stable, will go into 8.13.6.

Removed ~149423 label

Mentioned in commit bcee8e00