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

Update ocis-reva to 0.13.0 #496

Merged
merged 4 commits into from
Aug 27, 2020
Merged

Update ocis-reva to 0.13.0 #496

merged 4 commits into from
Aug 27, 2020

Conversation

PVince81
Copy link
Contributor

No description provided.

@phil-davis
Copy link
Contributor

phil-davis commented Aug 27, 2020

@PVince81 this will need similar changes to expected-failures and tests like owncloud/ocis-reva#454

Do you want me to push that stuff? - Answer: I did it ;)

@phil-davis
Copy link
Contributor

phil-davis commented Aug 27, 2020

Note: there are some core API test scenarios that started passing in owncloud/ocis-reva aand were removed from the expectd-failures list. But they are failing here in owncloud/ocis

https://cloud.drone.io/owncloud/ocis/1322/5/7

Scenario Outline: user with unusual username deletes a file                                   # /srv/app/testrunner/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature:258
    Given user "<username>" has been created with default attributes and without skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()
    And user "<username>" has uploaded file with content "to delete" to "/textfile0.txt"        # FeatureContext::userHasUploadedAFileWithContentTo()
    And using <dav-path> DAV path                                                               # FeatureContext::usingOldOrNewDavPath()
    When user "<username>" deletes file "/textfile0.txt" using the WebDAV API                   # FeatureContext::userDeletesFile()
    Then as "<username>" file "/textfile0.txt" should exist in the trashbin                     # TrashbinContext::asFileOrFolderExistsInTrash()
    But as "<username>" file "/textfile0.txt" should not exist                                  # FeatureContext::asFileOrFolderShouldNotExist()

    Examples:
      | dav-path | username |
      | old      | dash-123 |
      | old      | null     |
      | old      | nil      |
      | old      | 123      |
        Provisioning::usersHaveBeenCreatedUnexpected failure when creating a user: HTTP status 400 HTTP reason Bad Request OCS status 400 OCS message preferred_name '123' must be at least the local part of an email (Exception)
      | old      | -123     |
        Provisioning::usersHaveBeenCreatedUnexpected failure when creating a user: HTTP status 400 HTTP reason Bad Request OCS status 400 OCS message preferred_name '-123' must be at least the local part of an email (Exception)
      | old      | 0.0      |
        Provisioning::usersHaveBeenCreatedUnexpected failure when creating a user: HTTP status 400 HTTP reason Bad Request OCS status 400 OCS message preferred_name '0.0' must be at least the local part of an email (Exception)
      | new      | dash-123 |
      | new      | null     |
      | new      | nil      |
      | new      | 123      |
        Provisioning::usersHaveBeenCreatedUnexpected failure when creating a user: HTTP status 400 HTTP reason Bad Request OCS status 400 OCS message preferred_name '123' must be at least the local part of an email (Exception)
      | new      | -123     |
        Provisioning::usersHaveBeenCreatedUnexpected failure when creating a user: HTTP status 400 HTTP reason Bad Request OCS status 400 OCS message preferred_name '-123' must be at least the local part of an email (Exception)
      | new      | 0.0      |
        Provisioning::usersHaveBeenCreatedUnexpected failure when creating a user: HTTP status 400 HTTP reason Bad Request OCS status 400 OCS message preferred_name '0.0' must be at least the local part of an email (Exception)

It seems that the OCIS user provisioning cannot cope with numeric-looking userid-username. So they tests have no hope to pass because they do not even get started.

I raised https://github.com/owncloud/ocis-accounts/issues/97 to have a look at this. It does not block this PR.

@PVince81
Copy link
Contributor Author

I more wonder why the tests passed before. Maybe a new restriction has been added in reva recently?

@phil-davis
Copy link
Contributor

I more wonder why the tests passed before. Maybe a new restriction has been added in reva recently?

The tests did not pass before, they failed in reva because of a real bug. I was expecting to see them start passing here. But they still fail here because they were failing for 2 reasons - the account provisioning problem and the reva bug. Now the reva bug is gone, but still the account provisioning problem is there.

@PVince81
Copy link
Contributor Author

I grepped the code and I see there is a restriction in ocis-accounts: https://github.com/owncloud/ocis-accounts/blob/master/pkg/service/v0/accounts.go#L292

so this seems to be by design and we might need to permanently disable those tests as "expected behavior in OCIS".

@butonic can you confirm ?

@butonic
Copy link
Member

butonic commented Aug 27, 2020

yes, accounts MUST have a preferred_name (and actually also an on_premises_sam_account_name because that is used when checking the password via basic auth)

  • make glauth check preferred_name instead of on_premises_sam_account_name. on_premises_sam_account_name should be used for syncing with an external LDAP. ... well or make it configurable

@phil-davis
Copy link
Contributor

That has:

var usernameRegex = regexp.MustCompile("^[a-zA-Z_][a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]*(@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*)*$")

It insists that a username/userid/email-address-first-part/whatever-you-call-it must start with an alpha or underscore. But that is just not true. For example a University I was enrolled at used the student number for usernames and email address - e.g. username "247843222" and email address "[email protected]" - that all works fine for cloud file sharing in oC10, cs3org/reva and owncloud/ocis-reva. So, IMO, we better make sure it works for owncloud/ocis

@butonic
Copy link
Member

butonic commented Aug 27, 2020

hm, good point. I dug into the verification of unix usernames, because ultimately that username will appear as the username in an os that integrates with glauth to persist shares using acls. see https://unix.stackexchange.com/a/435120 and the systemd bug it caused: systemd/systemd#6237

@PVince81 PVince81 merged commit 40584f1 into master Aug 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the update-ocis-reva-0.13.0 branch August 27, 2020 08:34
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.

4 participants