X Tutup
Skip to content

[full-ci] Do not check for the username if if has been processed already#675

Merged
jnweiger merged 6 commits intomasterfrom
loginname_process_chain_ldap
Feb 24, 2023
Merged

[full-ci] Do not check for the username if if has been processed already#675
jnweiger merged 6 commits intomasterfrom
loginname_process_chain_ldap

Conversation

@jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Aug 11, 2021

Targets owncloud/core#39105
Changes are backwards-compatible, so owncloud/core#39105 isn't a real requirement, but this PR won't do anything without it.

@jvillafanez jvillafanez self-assigned this Aug 11, 2021
@jvillafanez jvillafanez mentioned this pull request Aug 11, 2021
Merged
11 tasks
@jvillafanez jvillafanez force-pushed the loginname_process_chain_ldap branch from d8a4102 to 0b66083 Compare September 6, 2021 09:17
@jvillafanez jvillafanez marked this pull request as ready for review September 6, 2021 09:17
@jvillafanez jvillafanez force-pushed the loginname_process_chain_ldap branch from 0b66083 to 69e5e28 Compare September 7, 2021 11:24
@AlexAndBear AlexAndBear self-requested a review October 7, 2021 10:50
@jvillafanez jvillafanez force-pushed the loginname_process_chain_ldap branch from 69e5e28 to fe8bb44 Compare October 7, 2021 11:49
@ownclouders
Copy link
Contributor

ownclouders commented Oct 7, 2021

💥 Acceptance tests pipeline apiUserLDAP-master-mysql8.0-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3521/28/1

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 7, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ownclouders
Copy link
Contributor

ownclouders commented Oct 7, 2021

💥 Acceptance tests pipeline apiUserLDAPS-master-mysql8.0-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3522/42/1

@ownclouders
Copy link
Contributor

ownclouders commented Oct 8, 2021

💥 Acceptance tests pipeline apiUserLDAP-master-mysql8.0-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3524/29/1

@AlexAndBear
Copy link

AlexAndBear commented Oct 11, 2021

@jvillafanez you might want to rebase master ?

@jvillafanez jvillafanez force-pushed the loginname_process_chain_ldap branch from fe8bb44 to fb26662 Compare October 11, 2021 08:27
@ownclouders
Copy link
Contributor

ownclouders commented Oct 11, 2021

💥 Acceptance tests pipeline apiUserLDAPS-master-mysql8.0-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3529/42/1

@jvillafanez
Copy link
Member Author

jvillafanez commented Oct 11, 2021

@phil-davis any idea of what could be happening?
I think we should remove the log check from the tests. If Alice is a local user, ownCloud shouldn't contact with LDAP any longer, so the log entry shouldn't appear. The entry showing in the logs could be from a previous test.

@jvillafanez
Copy link
Member Author

jvillafanez commented Oct 22, 2021

I've double-checked the behavior in my machine, and it's different. Alice gets a 200 HTTP status for the "/index.php/apps/files" request, likely because we aren't hitting the LDAP server any longer if it's a DB user.

    Given LDAP config "LDAPTestId" has key "ldapHost" set to "not-existent"                                        # UserLdapGeneralContext::ldapConfigHasKeySetTo()
[Fri Oct 22 08:22:45 2021] 127.0.0.1:54848 Accepted
[Fri Oct 22 08:22:45 2021] 127.0.0.1:54848 [200]: GET /index.php/apps/files
    When user "Alice" requests "/index.php/apps/files" with "GET" using basic auth                                 # AuthContext::userRequestsURLWithUsingBasicAuth()
[Fri Oct 22 08:22:45 2021] 127.0.0.1:54848 Closing
    Then the HTTP status code should be "401"                                                                      # FeatureContext::thenTheHTTPStatusCodeShouldBe()
->>      HTTP status code 200 is not the expected value 401  <<-
->>      Failed asserting that 200 matches expected '401'.  <<-
    And the last lines of the log file should contain log-entries containing these attributes:                     # LoggingContext::theLastLinesOfTheLogFileShouldContainEntriesWithTheseAttributes()
      | app       | message                                                 |
      | user_ldap | Error when searching: Can't contact LDAP server code -1 |
      | user_ldap | Attempt for Paging?                                     |
      | core      | Login failed: 'Alice' (Remote IP:                       |
    When the administrator sets the LDAP config "LDAPTestId" key "ldapHost" to "%ldap_host%" using the occ command # UserLdapGeneralContext::ldapConfigHasKeySetTo()
    And user "Alice" requests "/index.php/apps/files" with "GET" using basic auth                                  # AuthContext::userRequestsURLWithUsingBasicAuth()
    Then the HTTP status code should be "200"                                                                      # FeatureContext::thenTheHTTPStatusCodeShouldBe()

AlexAndBear
AlexAndBear reviewed Oct 28, 2021
View reviewed changes
@jvillafanez jvillafanez force-pushed the loginname_process_chain_ldap branch from fb26662 to f5219b9 Compare January 19, 2022 07:47
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ownclouders
Copy link
Contributor

ownclouders commented Jan 19, 2022

💥 Acceptance tests pipeline apiUserLDAP-master-postgres9.4-php7.3 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/3736/32/1

@jvillafanez jvillafanez force-pushed the loginname_process_chain_ldap branch from f5219b9 to 3a95291 Compare November 2, 2022 16:15
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ownclouders
Copy link
Contributor

ownclouders commented Nov 2, 2022 •
edited
Loading

💥 Acceptance tests pipeline apiUserLDAP-master-mysql8.0-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/user_ldap/4390/29

@jvillafanez jvillafanez force-pushed the loginname_process_chain_ldap branch from 3a95291 to e6d8716 Compare February 23, 2023 09:21
@pako81
Copy link

pako81 commented Feb 23, 2023

do we want to have this in 0.17.0?

@jnweiger jnweiger mentioned this pull request Feb 23, 2023
Closed
42 tasks
@jvillafanez
Copy link
Member Author

jvillafanez commented Feb 23, 2023

If it doesn't give any problems, I think so. I just bumped into this again recently: login with the admin took a lot of time because the ldap server was disconnected, which seems bad. Anyway, I don't think it's a priority.

@pako81
Copy link

pako81 commented Feb 23, 2023

Got it. If we can have it, even better.

@jvillafanez
Copy link
Member Author

jvillafanez commented Feb 23, 2023

I suspect the failure comes from the logs, because the test expects an error that doesn't happen any longer. Not sure who can confirm this.

@phil-davis
Copy link
Contributor

phil-davis commented Feb 24, 2023

https://drone.owncloud.com/owncloud/user_ldap/4390/29/13

runsh: Total unexpected failed scenarios throughout the test run:
apiUserLDAP/serverConnection.feature:48

  Scenario: authentication does not work when the hostname contains a wrong protocol             # /var/www/owncloud/testrunner/apps/user_ldap/tests/acceptance/features/apiUserLDAP/serverConnection.feature:48
    Given LDAP config "LDAPTestId" has key "ldapHost" set to "http://%ldap_host_without_scheme%" # UserLdapGeneralContext::ldapConfigHasKeySetTo()
    When user "Alice" requests "/index.php/apps/files" with "GET" using basic auth               # AuthContext::userRequestsURLWithUsingBasicAuth()
    Then the HTTP status code should be "401"                                                    # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the last lines of the log file should contain log-entries containing these attributes:   # LoggingContext::theLastLinesOfTheLogFileShouldContainEntriesWithTheseAttributes()
      | app | message                                                                              |
      | PHP | ldap_connect(): Could not create session handle: Bad parameter to an ldap routine at |
      | PHP | ldap_set_option(): supplied argument is not a valid ldap link resource at            |
      log entry:
      {"reqId":"AmcK0v6TNog2YKjXnJzG","level":3,"time":"2023-02-24T02:08:42+00:00","remoteAddr":"192.168.30.6","user":"--","app":"PHP","method":"GET","url":"\/index.php\/apps\/files","message":"ldap_set_option(): supplied argument is not a valid ldap link resource at \/var\/www\/owncloud\/server\/apps\/user_ldap\/lib\/LDAP.php#296"}

      Failed asserting that 'ldap_set_option(): supplied argument is not a valid ldap link resource at /var/www/owncloud/server/apps/user_ldap/lib/LDAP.php#296' contains "ldap_connect(): Could not create session handle: Bad parameter to an ldap routine at".

I will have a look...

@phil-davis phil-davis changed the title Do not check for the username if if has been processed already [full-ci] Do not check for the username if if has been processed already Feb 24, 2023
@phil-davis
Copy link
Contributor

phil-davis commented Feb 24, 2023 •
edited
Loading

I removed one of the log file checks in the failing test. The ldap_connect() error message does not happen any longer.

And I added full-ci in the PR title. That will allow all CI pipelines to run to completion, so we can see all the tests that pass or fail.

@AlexAndBear AlexAndBear requested review from AlexAndBear and removed request for AlexAndBear February 24, 2023 08:59
@phil-davis
Copy link
Contributor

phil-davis commented Feb 24, 2023

https://drone.owncloud.com/owncloud/user_ldap/4391/29/12

    And the last lines of the log file should contain log-entries containing these attributes:   # LoggingContext::theLastLinesOfTheLogFileShouldContainEntriesWithTheseAttributes()
      | app | message                                                                   |
      | PHP | ldap_set_option(): supplied argument is not a valid ldap link resource at |
      log entry:
      {"reqId":"mifRRq5OUjjbqWcqgQT5","level":2,"time":"2023-02-24T08:39:56+00:00","remoteAddr":"192.168.45.6","user":"--","app":"core","method":"GET","url":"\/index.php\/apps\/files","message":"Login failed: 'Alice' (Remote IP: '192.168.45.6')"}
      
      Failed asserting that 'core' contains "PHP".

I don't think that trying to check the detail of the log output in the test is doing anything really useful.
The existing check for HTTP status 401 "unauthorized" seems enough to me.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@jnweiger jnweiger merged commit d7d54b9 into master Feb 24, 2023
@delete-merged-branch delete-merged-branch bot deleted the loginname_process_chain_ldap branch February 24, 2023 10:32
@jnweiger jnweiger mentioned this pull request Feb 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@AlexAndBear AlexAndBear AlexAndBear approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jvillafanez jvillafanez

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jvillafanez @ownclouders @AlexAndBear @pako81 @phil-davis @jnweiger
X Tutup