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

[test-only] apiTest. change own password #4480

Merged
merged 2 commits into from
Aug 31, 2022
Merged

[test-only] apiTest. change own password #4480

merged 2 commits into from
Aug 31, 2022

Conversation

ScharfViktor
Copy link
Contributor

  • added api tests for changing own password

in case the user sends an incorrect current password, we get a 500 Server error. @rhafer Is it correct?

maybe we should return 400 with "incorrect current password"?

in the comparison oc10 returns 200 and message:
Screenshot 2022-08-31 at 09 31 41

@update-docs
Copy link

update-docs bot commented Aug 31, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@ownclouders
Copy link
Contributor

ownclouders commented Aug 31, 2022

💥 Acceptance test Core-API-Tests-ocis-storage-8 failed. Further test are cancelled...

@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 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

No Coverage information No Coverage information
No Duplication information No Duplication information

@rhafer
Copy link
Contributor

rhafer commented Aug 31, 2022

in case the user sends an incorrect current password, we get a 500 Server error. @rhafer Is it correct?

Currently that is intentional. See #3705 (comment) . Though thinking about this again the reasoning (avoiding brute-force attacks) seems somehow wrong as the /me/changePassword endpoint is only exposed to authenticated users (an only for changing their own password). I am open to revisit that decision.

maybe we should return 400 with "incorrect current password"?

Yeah. It might make sense. I'll open a PR for discussion.

in the comparison oc10 returns 200 and message:

That's an interesting behaviour. Seems at least weird to me.

@ScharfViktor
Copy link
Contributor Author

Yeah. It might make sense. I'll open a PR for discussion.

Thank you, then the web can process the response and give the user a message-reason "wrong current password" what is wrong

That's an interesting behaviour. Seems at least weird to me.

Me too. 200 is incorrect

@ScharfViktor
Copy link
Contributor Author

How do you prefer, change the test to 400 and move it to expected failures? or will you change the test when you fix the bug?

rhafer added a commit to rhafer/ocis that referenced this pull request Aug 31, 2022
Up to now the /me/changePassword endpoint return a 500 Status when
issue a password change with the old password set to the wrong password.
This changes the code to return 400 (Bad Request) with an additional
message that the old password is wrong. This does not seem to weaken the
security of /me/changePassword (i.e. for allowing easier brute-force
attacks) as the endpoint is only available to already authenticated
users (and only for changing their own passwords)

See owncloud#4480
@rhafer
Copy link
Contributor

rhafer commented Aug 31, 2022

or will you change the test when you fix the bug?

I'll update the test, with my PR.

Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

lgtm

@ScharfViktor ScharfViktor merged commit 3b548c9 into master Aug 31, 2022
@delete-merged-branch delete-merged-branch bot deleted the changeOwnPass branch August 31, 2022 11:02
rhafer added a commit to rhafer/ocis that referenced this pull request Aug 31, 2022
Up to now the /me/changePassword endpoint return a 500 Status when
issue a password change with the old password set to the wrong password.
This changes the code to return 400 (Bad Request) with an additional
message that the old password is wrong. This does not seem to weaken the
security of /me/changePassword (i.e. for allowing easier brute-force
attacks) as the endpoint is only available to already authenticated
users (and only for changing their own passwords)

See owncloud#4480
ownclouders pushed a commit that referenced this pull request Aug 31, 2022
Author: Viktor Scharf <scharf.vi@gmail.com>
Date:   Wed Aug 31 13:02:33 2022 +0200

    [test-only] apiTest. change own password (#4480)

    * apiTest. change own password

    * fix
rhafer added a commit to rhafer/ocis that referenced this pull request Aug 31, 2022
Up to now the /me/changePassword endpoint return a 500 Status when
issue a password change with the old password set to the wrong password.
This changes the code to return 400 (Bad Request) with an additional
message that the old password is wrong. This does not seem to weaken the
security of /me/changePassword (i.e. for allowing easier brute-force
attacks) as the endpoint is only available to already authenticated
users (and only for changing their own passwords)

See owncloud#4480
rhafer added a commit to rhafer/ocis that referenced this pull request Sep 1, 2022
Up to now the /me/changePassword endpoint return a 500 Status when
issue a password change with the old password set to the wrong password.
This changes the code to return 400 (Bad Request) with an additional
message that the old password is wrong. This does not seem to weaken the
security of /me/changePassword (i.e. for allowing easier brute-force
attacks) as the endpoint is only available to already authenticated
users (and only for changing their own passwords)

See owncloud#4480
rhafer added a commit that referenced this pull request Sep 5, 2022
Up to now the /me/changePassword endpoint return a 500 Status when
issue a password change with the old password set to the wrong password.
This changes the code to return 400 (Bad Request) with an additional
message that the old password is wrong. This does not seem to weaken the
security of /me/changePassword (i.e. for allowing easier brute-force
attacks) as the endpoint is only available to already authenticated
users (and only for changing their own passwords)

See #4480
ownclouders pushed a commit that referenced this pull request Sep 5, 2022
Author: Ralf Haferkamp <rhaferkamp@owncloud.com>
Date:   Wed Aug 31 12:46:03 2022 +0200

    graph: Fix Status code when updating the password

    Up to now the /me/changePassword endpoint return a 500 Status when
    issue a password change with the old password set to the wrong password.
    This changes the code to return 400 (Bad Request) with an additional
    message that the old password is wrong. This does not seem to weaken the
    security of /me/changePassword (i.e. for allowing easier brute-force
    attacks) as the endpoint is only available to already authenticated
    users (and only for changing their own passwords)

    See #4480
butonic added a commit that referenced this pull request Sep 8, 2022
* Add support for the jsoncs3 share manager

* WIP: point to the wip-reva with the jsoncs3 share manager

* migration fixes

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* update reva

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* use common config for jsoncs3 defaults

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* lint

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* add home space deletion on user delete

Signed-off-by: Christian Richter <crichter@owncloud.com>

* update reva

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* add delete home space permission to admin role

Signed-off-by: Christian Richter <crichter@owncloud.com>

* Bump github.com/tus/tusd from 1.9.0 to 1.9.1

Bumps [github.com/tus/tusd](https://github.com/tus/tusd) from 1.9.0 to 1.9.1.
- [Release notes](https://github.com/tus/tusd/releases)
- [Commits](tus/tusd@v1.9.0...v1.9.1)

---
updated-dependencies:
- dependency-name: github.com/tus/tusd
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/onsi/gomega from 1.20.0 to 1.20.1

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.20.0 to 1.20.1.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.20.0...v1.20.1)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* part1

* update reva

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

* [test-only] apiTest. change own password (#4480)

* apiTest. change own password

* fix

* Bump github.com/gookit/config/v2 from 2.1.2 to 2.1.4

Bumps [github.com/gookit/config/v2](https://github.com/gookit/config) from 2.1.2 to 2.1.4.
- [Release notes](https://github.com/gookit/config/releases)
- [Commits](gookit/config@v2.1.2...v2.1.4)

---
updated-dependencies:
- dependency-name: github.com/gookit/config/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* move graph api methods from Space context to GraphHelper

* fix after rebase

* Bump github.com/rs/zerolog from 1.27.0 to 1.28.0

Bumps [github.com/rs/zerolog](https://github.com/rs/zerolog) from 1.27.0 to 1.28.0.
- [Release notes](https://github.com/rs/zerolog/releases)
- [Commits](rs/zerolog@v1.27.0...v1.28.0)

---
updated-dependencies:
- dependency-name: github.com/rs/zerolog
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/onsi/ginkgo/v2 from 2.1.4 to 2.1.6

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.1.4 to 2.1.6.
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.1.4...v2.1.6)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/coreos/go-oidc/v3 from 3.2.0 to 3.3.0

Bumps [github.com/coreos/go-oidc/v3](https://github.com/coreos/go-oidc) from 3.2.0 to 3.3.0.
- [Release notes](https://github.com/coreos/go-oidc/releases)
- [Commits](coreos/go-oidc@v3.2.0...v3.3.0)

---
updated-dependencies:
- dependency-name: github.com/coreos/go-oidc/v3
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Added /Shares related tests for lock properties on ocis

* Update expected to failure for local api for copy

* Bump github.com/grpc-ecosystem/grpc-gateway/v2 from 2.11.2 to 2.11.3

Bumps [github.com/grpc-ecosystem/grpc-gateway/v2](https://github.com/grpc-ecosystem/grpc-gateway) from 2.11.2 to 2.11.3.
- [Release notes](https://github.com/grpc-ecosystem/grpc-gateway/releases)
- [Changelog](https://github.com/grpc-ecosystem/grpc-gateway/blob/master/.goreleaser.yml)
- [Commits](grpc-ecosystem/grpc-gateway@v2.11.2...v2.11.3)

---
updated-dependencies:
- dependency-name: github.com/grpc-ecosystem/grpc-gateway/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* refactor proxy code

I refactored the proxy so that we execute the routing before the
authentication middleware. This is necessary so that we can determine
which routes are considered unprotected i.e. which routes don't need
authentication.

* add unprotected flag to the proxy routes

I added an unprotected flag to the proxy routes which is evaluated by
the authentication middleware. This way we won't have to maintain a
hardcoded list of unprotected paths and path prefixes and we will
hopefully reduce the times we encounter the basic auth prompt by web
browsers.

* add missing unprotected flag and fix proxy test

* fix linting issues

* fix default policy and add changelog

* update tests

* Automated changelog update [skip ci]

* Bump google.golang.org/grpc from 1.48.0 to 1.49.0

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.48.0 to 1.49.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.48.0...v1.49.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/onsi/gomega from 1.20.1 to 1.20.2

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.20.1 to 1.20.2.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.20.1...v1.20.2)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/urfave/cli/v2 from 2.11.2 to 2.14.0

Bumps [github.com/urfave/cli/v2](https://github.com/urfave/cli) from 2.11.2 to 2.14.0.
- [Release notes](https://github.com/urfave/cli/releases)
- [Changelog](https://github.com/urfave/cli/blob/main/docs/CHANGELOG.md)
- [Commits](urfave/cli@v2.11.2...v2.14.0)

---
updated-dependencies:
- dependency-name: github.com/urfave/cli/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump github.com/owncloud/libre-graph-api-go from 0.16.0 to 0.17.0

Bumps [github.com/owncloud/libre-graph-api-go](https://github.com/owncloud/libre-graph-api-go) from 0.16.0 to 0.17.0.
- [Release notes](https://github.com/owncloud/libre-graph-api-go/releases)
- [Commits](owncloud/libre-graph-api-go@v0.16.0...v0.17.0)

---
updated-dependencies:
- dependency-name: github.com/owncloud/libre-graph-api-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Evaluate routing rules ordered by path-length

This is a quickfix for #4497. Before evaluating, we now sort the rules
of a specific type by the length of the endpoints and start evaluation
with the most specific endpoint first. There's obviously quite a bit
room for optimization here and this will only fix the issue for routes
of type `PrefixRoute`. But it should solve the immediate issue.

* Bump commit id for tests

* Improve login screen design

* Move background-size after the background css prop

* add returns after rendering errors and simplify loop condition

* Hardcode header in GraphHelper::deleteSpace

* Add 'Inter' font, change placeholder color to grey

* Use cv11 as font feature setting

* Automated changelog update [skip ci]

* adjust REPORT to PROPFIND endpoint

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* regenerate protogen

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* adjust expected failures

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* Automated changelog update [skip ci]

* Fix translations on login page

* Automated changelog update [skip ci]

* removed search test from expected failures

* Reuse code of code from core at most

* Add commit and branch related to this PR to work

* Reuse locking method from core

* fix search test

* Add api tests for tus upload when quota is set

* Add tests removed by core pr-40276

* graph: Fix Status code when updating the password

Up to now the /me/changePassword endpoint return a 500 Status when
issue a password change with the old password set to the wrong password.
This changes the code to return 400 (Bad Request) with an additional
message that the old password is wrong. This does not seem to weaken the
security of /me/changePassword (i.e. for allowing easier brute-force
attacks) as the endpoint is only available to already authenticated
users (and only for changing their own passwords)

See #4480

* Bump github.com/gookit/config/v2 from 2.1.4 to 2.1.5

Bumps [github.com/gookit/config/v2](https://github.com/gookit/config) from 2.1.4 to 2.1.5.
- [Release notes](https://github.com/gookit/config/releases)
- [Commits](gookit/config@v2.1.4...v2.1.5)

---
updated-dependencies:
- dependency-name: github.com/gookit/config/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* auto orientate pictures for thumbnails

* Automated changelog update [skip ci]

* update buf lock

* graph: purge home space when deleting a user

previously the homespace was just marked as trashed

Fixes: #4195

* proxy: Initialize logger for router

* proxy: Fix archiver for public links

Allows /archiver to be used the "public-token" auth middleware. The
archiver is a bit of a special case, because it can be uses in several
ways: using 'normal' authentication (basic, oidc), using signed-urls or
using sharetokens. As only the "sharetoken" part is handled by the
"PublicShareAuth" middleware, we needed to special-case it a bit.

* proxy: Clarify comment

* proxy: Avoid sorting endpoints for every single request

The endpoints are no longer hashed by path name in the directors map since
that made iterating over the endpoints unstable. They are now stored in a
slice in the order in which the are defined in the configuration.

Closes: #4497

* Automated changelog update [skip ci]

* update reva to latest edge

* remove personal spaces as admin in the graph test suite

* use id to delete project spaces

* Add context in behat.yml

* Fix home space deletion when deleting user by name

DELETE requess on /graph/v1.0/users also work when specifing a user by
name. For deleting the home space in that case we need to get the User's
id from the backend first.

Fixes: #4195

* Add new "delete-all-spaces" permission

This is assigned to the Admin role by default and allows to cleanup
orphaned spaces (e.g. where the owner as been deleted)

Fixes: #4196

* Automated changelog update [skip ci]

* Automated changelog update [skip ci]

* update reva to latest edge

* Automated changelog update [skip ci]

* delete users with their personal spaces

* update reva to latest edge

* delete method

* Remove tests from unrelated issue and add actual one

* REPORT: add permissions to personal space

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* changelog and bump reva

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* Automated changelog update [skip ci]

* update reva and jsoncs3 share manager config

* use experimental reva

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* fix search service after merge

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* praise the linting god

Signed-off-by: jkoberg <jkoberg@owncloud.com>

* use cs3 share manager

Signed-off-by: jkoberg <jkoberg@owncloud.com>

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Christian Richter <crichter@owncloud.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Co-authored-by: André Duffeck <andre.duffeck@firondu.de>
Co-authored-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Co-authored-by: Christian Richter <crichter@owncloud.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Viktor Scharf <scharf.vi@gmail.com>
Co-authored-by: David Christofas <dchristofas@owncloud.com>
Co-authored-by: sagargurung1001@gmail.com <sagargurung1001@gmail.com>
Co-authored-by: Ralf Haferkamp <rhaferkamp@owncloud.com>
Co-authored-by: Swikriti Tripathi <swikriti808@gmail.com>
Co-authored-by: Jannik Stehle <jannik.stehle@gmail.com>
Co-authored-by: Phil Davis <phil@jankaritech.com>
Co-authored-by: Swikriti Tripathi <41103328+SwikritiT@users.noreply.github.com>
Co-authored-by: Jannik Stehle <50302941+JammingBen@users.noreply.github.com>
Co-authored-by: Sagar Gurung <46086950+SagarGi@users.noreply.github.com>
Co-authored-by: Amrita <54478846+amrita-shrestha@users.noreply.github.com>
Co-authored-by: Michael Barz <mbarz@owncloud.com>
Co-authored-by: amrita <anukanxi05@gmail.com>
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.

3 participants