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

Add 401 Unauthenticated to conjure error types #367

Merged
merged 2 commits into from
Oct 14, 2019

Conversation

amrav
Copy link
Contributor

@amrav amrav commented Aug 22, 2019

Palantir's authentication service uses 401 in the expected way, but currently clients have to manually propagate these. This PR makes 401 part of the conjure spec, which will let us define it as an ErrorType in conjure-java-runtime-api and propagate by default in conjure-java-runtime, similar to 403s.

HTTP names this 401 Unauthorized, however 401 Unauthenticated seems like a clearer name given the way Palantir uses 401s, and to avoid confusion with 403 PermissionDenied.

@amrav amrav requested a review from a team as a code owner August 22, 2019 13:47
@changelog-app
Copy link

changelog-app bot commented Aug 22, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Add 401 Unauthenticated

Palantir's authentication service uses 401 in the expected way, but currently clients have to manually propagate these. This PR makes 401 part of the conjure spec, which will let us define it as an ErrorType in conjure-java-runtime-api and propagate by default in conjure-java-runtime, similar to 403s.

HTTP names this 401 Unauthorized, however 401 Unauthenticated seems like a clearer name given the way Palantir uses 401s, and to avoid confusion with 403 PermissionDenied.

Check the box to generate changelog(s)

  • Generate changelog entry

@iamdanfox
Copy link
Contributor

This seems pretty reasonable to me. It seems valuable to be able to distinguish in server logs:

  • server doesn't know who you are (unauthenticated 401), and
  • server knows who you are and doesn't want to let you in (permission_denied 403)

@carterkozak
Copy link
Contributor

Agreed, I've found it helpful to differentiate auth-n failures from auth-z in other systems. I have been hesitant to throw 403 forbidden for missing or malformed credential material, but 401 would make sense.

@uschi2000
Copy link
Contributor

In what way would clients respond differently depending on receiving a 401 or 403?

@carterkozak
Copy link
Contributor

After receiving a 403 a client may attempt other operations, however a 401 suggests that there's no reason to make requests with the current credentials until they have been replaced.

@a-k-g
Copy link

a-k-g commented Sep 30, 2019

Looks like the comments on this PR were addressed. Could we get this merged in that case?

@uschi2000
Copy link
Contributor

We had an internal discussion (on Slack) about this a while that, to the best of my knowledge, was not resolved?

@pkoenig10
Copy link
Member

+1, would like to get this merged. We have had numerous FRs for our internal authentication service to return error responses when tokens have expired and are no longer valid.

@JacekLach
Copy link

+1 as per slack discussion, lack of OOTB handling of 401s in the server framework (at least in part a consequence of 401s not being part of the conjure api) is a headache

@dansanduleac
Copy link
Contributor

👍

@dansanduleac dansanduleac changed the title Add 401 Unauthenticated Add 401 Unauthenticated to conjure error types Oct 14, 2019
@dansanduleac dansanduleac merged commit aad1a81 into palantir:master Oct 14, 2019
- INVALID_ARGUMENT
- UNAUTHENTICATED
- PERMISSION_DENIED
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a change to the IR - I think we now need to figure out our IR versioning story pronto before we cut any releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants