Details

Reviewers

Group Reviewers

Commits

rDDEPba44a97373a2: deposit.auth: Adjust authentication error message

Summary

The error message in case of authentication failure is now aligned with the previous
basic authentication scheme. Prior to this, the keycloak error dict leaked into the
summary field which made the error unreadable.

before (basic auth):

$ curl -u test2:test http://localhost:5080/deposit/1/servicedocument/
<?xml version="1.0" encoding="utf-8"?>
<sword:error xmlns="http://www.w3.org/2005/Atom"
             xmlns:sword="http://purl.org/net/sword/terms/">
    <summary>Invalid username/password.</summary>
    <sword:treatment>processing failed</sword:treatment>

    <sword:verboseDescription>
        API is protected by basic authentication
    </sword:verboseDescription>

</sword:error>
$ swh deposit metadata-only --url http://localhost:5080/deposit  --username test \
  --password test2 \
  --metadata ../deposit-swh.update-metadata.xml
ERROR:swh.deposit.cli.client:Problem during parsing options: Service document retrieval: Invalid username/password.

before (keycloak):

$ curl -u test2:test http://localhost:5080/deposit/1/servicedocument/
<?xml version="1.0" encoding="utf-8"?>
<sword:error xmlns="http://www.w3.org/2005/Atom"
             xmlns:sword="http://purl.org/net/sword/terms/">
    <summary>{&quot;error&quot;:&quot;invalid_grant&quot;,&quot;error_description&quot;:&quot;Invalid user credentials&quot;}</summary>
    <sword:treatment>processing failed</sword:treatment>

    <sword:verboseDescription>
        API is protected by basic authentication
    </sword:verboseDescription>

</sword:error>
$ swh deposit metadata-only --url http://localhost:5080/deposit \
  --username test \
  --password test2 \
  --metadata ../deposit-swh.update-metadata.xml --format json
ERROR:swh.deposit.cli.client:Problem during parsing options: Service document retrieval: {"error":"invalid_grant","error_description":"Invalid user credentials"}

And now, with the following diff and keycloak authentication scheme:

$ curl -u test2:test http://localhost:5080/deposit/1/servicedocument/
<?xml version="1.0" encoding="utf-8"?>
<sword:error xmlns="http://www.w3.org/2005/Atom"
             xmlns:sword="http://purl.org/net/sword/terms/">
    <summary>invalid_grant: Invalid user credentials</summary>
    <sword:treatment>processing failed</sword:treatment>

    <sword:verboseDescription>
        API is protected by basic authentication
    </sword:verboseDescription>

</sword:error>
$  swh deposit metadata-only --url http://localhost:5080/deposit  --username test \
  --password test2 \
  --metadata ../deposit-swh.update-metadata.xml
ERROR:swh.deposit.cli.client:Problem during parsing options: Service document retrieval: invalid_grant: Invalid user credentials

Test Plan

tox

Diff Detail

Repository

rDDEP Push deposit

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Mar 22 2021, 6:53 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptMar 22 2021, 6:53 PM

Build has FAILED

Patch application report for D5302 (id=18989)

Could not rebase; Attempt merge onto 1c20fcc561...

Updating 1c20fcc5..a80b05f4
Fast-forward
 requirements-swh-server.txt               |  2 +-
 swh/deposit/auth.py                       |  6 ++---
 swh/deposit/cli/client.py                 |  3 ++-
 swh/deposit/client.py                     | 27 ++++++++++---------
 swh/deposit/tests/cli/test_client.py      | 45 +++++++++++++++++++++++++++++--
 swh/deposit/tests/data/atom/error-cli.xml |  9 +++++++
 6 files changed, 72 insertions(+), 20 deletions(-)
 create mode 100644 swh/deposit/tests/data/atom/error-cli.xml

Changes applied before test

commit a80b05f4690306b522dab91e3507496cdcbdd4e1
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 18:48:12 2021 +0100

    deposit.auth: Make the exception caught more specific
    
    This also allows to restrict the error message to the actual error message.

commit 84ed18de25a6ad9f3c37ef7710ec765ab0257992
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 17:45:11 2021 +0100

    deposit.cli: Fix service document error when failing to retrieve it

commit 026ca4ead098bd3ddb8b8440ca0a77f50651debb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 15:57:21 2021 +0100

    deposit.cli: Fix cli parsing issue when xml error is returned by server
    
    Prior to this commit, any error returned by the deposit server would end up a cascading
    KeyError.
    
    This commit fixes it.

Link to build: https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/589/
See console output for more information: https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/589/console

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 22 2021, 6:53 PM

Harbormaster failed remote builds in B20066: Diff 18989!

mmm, actually, this needs more thinking...
The previous message was semi-correct.

Expected result would be:

{"summary": "Invalid user credentials", "detail": "", "sword:verboseDescription": "API is protected by basic authentication", "swh:deposit_id": null, "swh:deposit_status": null, "status": 401}

(or something)

Build is green

Patch application report for D5302 (id=18989)

Could not rebase; Attempt merge onto 1c20fcc561...

Updating 1c20fcc5..a80b05f4
Fast-forward
 requirements-swh-server.txt               |  2 +-
 swh/deposit/auth.py                       |  6 ++---
 swh/deposit/cli/client.py                 |  3 ++-
 swh/deposit/client.py                     | 27 ++++++++++---------
 swh/deposit/tests/cli/test_client.py      | 45 +++++++++++++++++++++++++++++--
 swh/deposit/tests/data/atom/error-cli.xml |  9 +++++++
 6 files changed, 72 insertions(+), 20 deletions(-)
 create mode 100644 swh/deposit/tests/data/atom/error-cli.xml

Changes applied before test

commit a80b05f4690306b522dab91e3507496cdcbdd4e1
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 18:48:12 2021 +0100

    deposit.auth: Make the exception caught more specific
    
    This also allows to restrict the error message to the actual error message.

commit 84ed18de25a6ad9f3c37ef7710ec765ab0257992
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 17:45:11 2021 +0100

    deposit.cli: Fix service document error when failing to retrieve it

commit 026ca4ead098bd3ddb8b8440ca0a77f50651debb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 15:57:21 2021 +0100

    deposit.cli: Fix cli parsing issue when xml error is returned by server
    
    Prior to this commit, any error returned by the deposit server would end up a cascading
    KeyError.
    
    This commit fixes it.

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/590/ for more details.

Harbormaster completed remote builds in B20066: Diff 18989.Mar 23 2021, 9:42 AM

ardumont requested review of this revision.Mar 23 2021, 9:42 AM

mmm, actually, this needs more thinking...

This is actually something to be fixed in another diff.

ardumont added inline comments.Mar 23 2021, 10:17 AM

swh/deposit/auth.py
149	e.error_message is actually a dict so i need to rework this after all.

ardumont planned changes to this revision.Mar 23 2021, 10:17 AM

ardumont mentioned this in D5304: auth.pytest_plugin: Adjust error type according to reality.Mar 23 2021, 10:49 AM

Adapt according to my own remarks

ardumont retitled this revision from deposit.auth: Make the exception caught more specific to deposit.auth: Make the authentication exception caught more specific.Mar 23 2021, 10:58 AM

ardumont edited the summary of this revision. (Show Details)

Build is green

Patch application report for D5302 (id=19003)

Could not rebase; Attempt merge onto 1c20fcc561...

Updating 1c20fcc5..54cd94bc
Fast-forward
 requirements-swh-server.txt               |  2 +-
 swh/deposit/auth.py                       | 10 ++++---
 swh/deposit/cli/client.py                 |  3 ++-
 swh/deposit/client.py                     | 27 ++++++++++---------
 swh/deposit/tests/cli/test_client.py      | 45 +++++++++++++++++++++++++++++--
 swh/deposit/tests/conftest.py             |  9 +++++++
 swh/deposit/tests/data/atom/error-cli.xml |  9 +++++++
 7 files changed, 85 insertions(+), 20 deletions(-)
 create mode 100644 swh/deposit/tests/data/atom/error-cli.xml

Changes applied before test

commit 54cd94bc9cd9df7dfe608ca58ec05dd63d5582be
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 18:48:12 2021 +0100

    deposit.auth: Make the authentication exception caught more specific
    
    This also allows to restrict the error message to the actual error message. And to parse
    it appropriately so the rendering in the xml is not a dict.
    
    Related to T2858

commit 84ed18de25a6ad9f3c37ef7710ec765ab0257992
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 17:45:11 2021 +0100

    deposit.cli: Fix service document error when failing to retrieve it

commit 026ca4ead098bd3ddb8b8440ca0a77f50651debb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 15:57:21 2021 +0100

    deposit.cli: Fix cli parsing issue when xml error is returned by server
    
    Prior to this commit, any error returned by the deposit server would end up a cascading
    KeyError.
    
    This commit fixes it.

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/591/ for more details.

Harbormaster completed remote builds in B20080: Diff 19003.Mar 23 2021, 10:58 AM

ardumont added inline comments.Mar 23 2021, 10:59 AM

swh/deposit/tests/conftest.py
325 ↗	(On Diff #19003)	This shall go away if D5304 lands and is released in a new swh.auth version.

ardumont retitled this revision from deposit.auth: Make the authentication exception caught more specific to deposit.auth: Adjust authentication error message.Mar 23 2021, 11:09 AM

ardumont edited the summary of this revision. (Show Details)

Rework commit message (align according to diff description)

ardumont edited the test plan for this revision. (Show Details)Mar 23 2021, 11:11 AM

Build is green

Patch application report for D5302 (id=19004)

Could not rebase; Attempt merge onto 1c20fcc561...

Updating 1c20fcc5..a65d0065
Fast-forward
 requirements-swh-server.txt               |  2 +-
 swh/deposit/auth.py                       | 10 ++++---
 swh/deposit/cli/client.py                 |  3 ++-
 swh/deposit/client.py                     | 27 ++++++++++---------
 swh/deposit/tests/cli/test_client.py      | 45 +++++++++++++++++++++++++++++--
 swh/deposit/tests/conftest.py             |  9 +++++++
 swh/deposit/tests/data/atom/error-cli.xml |  9 +++++++
 7 files changed, 85 insertions(+), 20 deletions(-)
 create mode 100644 swh/deposit/tests/data/atom/error-cli.xml

Changes applied before test

commit a65d00651af26554aa11a3a77ae804519a1f65b9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 18:48:12 2021 +0100

    deposit.auth: Adjust authentication error message
    
    The error message in case of authentication failure is now aligned with the previous
    basic authentication scheme. Prior to this, the keycloak error dict leaked into the
    summary field which made the error unreadable.
    
    Related to T2858

commit 84ed18de25a6ad9f3c37ef7710ec765ab0257992
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 17:45:11 2021 +0100

    deposit.cli: Fix service document error when failing to retrieve it

commit 026ca4ead098bd3ddb8b8440ca0a77f50651debb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 15:57:21 2021 +0100

    deposit.cli: Fix cli parsing issue when xml error is returned by server
    
    Prior to this commit, any error returned by the deposit server would end up a cascading
    KeyError.
    
    This commit fixes it.

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/592/ for more details.

Harbormaster completed remote builds in B20081: Diff 19004.Mar 23 2021, 11:13 AM

anlambert accepted this revision.Mar 23 2021, 11:20 AM

This revision is now accepted and ready to land.Mar 23 2021, 11:20 AM

Now swh.auth v0.3.6 got released, so dropping unnecessary deposit test definition
regarding failing test setup.

Build is green

Patch application report for D5302 (id=19010)

Could not rebase; Attempt merge onto 026ca4ead0...

Updating 026ca4ea..ba44a973
Fast-forward
 requirements-swh-server.txt          |  2 +-
 swh/deposit/auth.py                  | 10 +++++++---
 swh/deposit/cli/client.py            |  3 ++-
 swh/deposit/client.py                |  6 +++++-
 swh/deposit/tests/cli/test_client.py | 15 +++++++++++++++
 5 files changed, 30 insertions(+), 6 deletions(-)

Changes applied before test

commit ba44a97373a235f7a9a9db4ab18250617b814032
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 18:48:12 2021 +0100

    deposit.auth: Adjust authentication error message
    
    The error message in case of authentication failure is now aligned with the previous
    basic authentication scheme. Prior to this, the keycloak error dict leaked into the
    summary field which made the error unreadable.
    
    Related to T2858

commit 84ed18de25a6ad9f3c37ef7710ec765ab0257992
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 22 17:45:11 2021 +0100

    deposit.cli: Fix service document error when failing to retrieve it

See https://jenkins.softwareheritage.org/job/DDEP/job/tests-on-diff/593/ for more details.

Harbormaster completed remote builds in B20086: Diff 19010.Mar 23 2021, 11:32 AM

Closed by commit rDDEPba44a97373a2: deposit.auth: Adjust authentication error message (authored by ardumont). · Explain WhyMar 23 2021, 11:34 AM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDDEPba44a97373a2: deposit.auth: Adjust authentication error message.

deposit.auth: Adjust authentication error message
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D5302 (id=18989)

Changes applied before test

Patch application report for D5302 (id=18989)

Changes applied before test

Patch application report for D5302 (id=19003)

Changes applied before test

Patch application report for D5302 (id=19004)

Changes applied before test

Patch application report for D5302 (id=19010)

Changes applied before test

Revision Contents
Changeset List

Diff 19012

requirements-swh-server.txt

swh/deposit/auth.py

deposit.auth: Adjust authentication error messageClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D5302 (id=18989)

Changes applied before test

Patch application report for D5302 (id=18989)

Changes applied before test

Patch application report for D5302 (id=19003)

Changes applied before test

Patch application report for D5302 (id=19004)

Changes applied before test

Patch application report for D5302 (id=19010)

Changes applied before test

Revision ContentsChangeset List

Diff 19012

requirements-swh-server.txt

swh/deposit/auth.py

deposit.auth: Adjust authentication error message
ClosedPublic
Actions

Revision Contents
Changeset List