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

Remove bearer authentication errors, re-set unauthorized responses #1555

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

roshii
Copy link
Contributor

@roshii roshii commented Sep 20, 2023

See #1552

Copy link
Member

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

utACK ef29982

@kristapsk
Copy link
Member

@theborakompanioni Looks ok to you?

@theborakompanioni
Copy link
Contributor

@theborakompanioni Looks ok to you?

Hmmm.. I am not sure.. shouldn't all endpoints with

  security:
    - bearerAuth: []

potentially answer with a 401 response? @roshii

  '401':
    $ref: '#/components/responses/401-Unauthorized'

@roshii
Copy link
Contributor Author

roshii commented Sep 20, 2023

@theborakompanioni Looks ok to you?

Hmmm.. I am not sure.. shouldn't all endpoints with

  security:
    - bearerAuth: []

potentially answer with a 401 response? @roshii

  '401':
    $ref: '#/components/responses/401-Unauthorized'

See #1552 (comment)

After a few reads and as far as I understand, 401-AuthenticationError does not need to be defined in wallet RPC API since it is an implication of bearer token authentication, defined by the security scheme object. Its error codes are in turn defined by https://datatracker.ietf.org/doc/html/rfc6750#section-3.1.

Bottom line, 401-AuthenticationError response definition should be removed, along with 403-Forbidden

@theborakompanioni
Copy link
Contributor

See #1552 (comment)

After a few reads and as far as I understand, 401-AuthenticationError does not need to be defined in wallet RPC API since it is an implication of bearer token authentication, defined by the security scheme object. Its error codes are in turn defined by https://datatracker.ietf.org/doc/html/rfc6750#section-3.1.
Bottom line, 401-AuthenticationError response definition should be removed, along with 403-Forbidden

Okay, thanks for the clarification. However, what use is 401-Unauthorized then, what does it effectively signal and when is it returned?

To be more specific, e.g. why does /wallet/{walletname}/taker/stop include 401 but /wallet/{walletname}/configset does not?

@roshii
Copy link
Contributor Author

roshii commented Sep 20, 2023

Okay, thanks for the clarification. However, what use is 401-Unauthorized then, what does it effectively signal and when is it returned?

401-Unauthorized will be issued for errors such as providing an invalid wallet password, starting/stopping a started/stopped service or unlocking an unlocked wallet, in other words: any other authorization error than API authentication/authorization related.

To be more specific, e.g. why does /wallet/{walletname}/taker/stop include 401 but /wallet/{walletname}/configset does not?

In this example:

  • /wallet/{walletname}/taker/stop will return a 401 if service is already stopped
  • /wallet/{walletname}/configset has not authorization logic per se, if you have a valid JWT it will never return a 401.

@theborakompanioni
Copy link
Contributor

401-Unauthorized will be issued for errors such as providing an invalid wallet password, starting/stopping a started/stopped service or unlocking an unlocked wallet, in other words: any other authorization error than API authentication/authorization related.

Ok, thanks for taking the time to explain it. 🙏
utACK!

@kristapsk kristapsk merged commit 131fe09 into JoinMarket-Org:master Sep 21, 2023
20 checks passed
@roshii roshii deleted the bearer-errors branch September 22, 2023 11:52
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