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

feat: easier to extend the payload returning the token without having… #5726

Closed
wants to merge 1 commit into from

Conversation

woile
Copy link
Contributor

@woile woile commented Jan 5, 2018

Problem

Usually we want to extend the information here, reusing the token, not reimplementing the whole method.

I find it counterintuitive having to copy the whole function again, like suggested here it's not what you'd expect at first, and I start wondering if I might be doing something wrong.

Another solution is calling super and having to scrap the response for the token, like suggested here usually if you modify this function, you want something related to the user, and you will have to request it again (or the token), because it was already requested in the serializer inside the super.

Use cases

  • Returning the groups a user belongs to (that's what I intend to use it for)
  • Returning a user id

Similar PR

#4385
#2978
#2151

Tickets related

#2414

Hope it helps!

Regards!

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @woile.

I'm not sure about this. I see what you're doing, no problem there...

We have, though, deliberately, long-chosen to keep the auth token implementation as simple as possible. We explicitly do want people to re-implement if the packaged solution doesn't meet needs.

(The reasoning here is just fixing the scope of the project — once we allow some extensions, we then allow others and all of a sudden we're maintaining an overly complex view/model/...)

@rpkilby
Copy link
Member

rpkilby commented Jan 8, 2018

Agreed w/ @carltongibson

I'd suggest following the solution from #2414.

@woile
Copy link
Contributor Author

woile commented Jan 8, 2018

Yeah, I understand. Keep in mind that there were already 3 PR for this feature, so it looks like a common problem/choice people face. I think mostly because it's a bit counter intuitive (not wrong at all) doing that.

Even if we don't merge this PR, I think we could improve the docs, like showing a whole example of how to reimplement the ObtainAuthToken, and explain why the choice to do that. What do you think?

@carltongibson
Copy link
Collaborator

Hey @woile — sorry for the slow follow-up.

... I think we could improve the docs...

Ultimately I think that's the way I want to go here. An example towards the end of the TokenAuthentication docs would be great.

Fancy taking that on? 😉

@woile
Copy link
Contributor Author

woile commented Jan 19, 2018

I'll check it out after work!

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.

3 participants