search

divio / django-simple-sso

Other
307 stars 116 forks source link

Update user data #62

Closed Rastopapola closed 3 years ago

Rastopapola commented 3 years ago

Hello simple-sso team,

it's me again! Here is an implementation, coming with a test for issue #61 .

It would love to hear some feedback soon.

Best regards!

Rastopapola commented 3 years ago

Sorry, I forgot to mention some people: I summon @GaretJax or @kinkerl

Thanks!

GaretJax commented 3 years ago

Thanks for your PR @Rastopapola :-)

I left a comment.

Rastopapola commented 3 years ago

@GaretJax Sure thing! I really enjoy your project, which helps me a lot as well and I would be proud to contribute something practical :)

I see your concerns regarding the user_data. To decrease the amount of if-checks inside the loop I suggest building a 'user_data_tmp' dict beforehand, excluding the username. This way 'user_data' won't be touched and we don't have redundant checks in the loop.

'user_data' is the serialized 'User' model which is received by the sso client application for e.g. creating a new user. It holds only values that are provided by the sso server: https://github.com/divio/django-simple-sso/blob/master/simple_sso/sso_server/server.py#L153-L166 As you can see there is some 'extra_data' which can extend the serialized basic 'User' model data. As long as there are no critical extra-user-data, which has been added there by the developer inside the project, this critical data won't be received by any client.

I quickly update the code on this PR, so you can take another look ;)

Rastopapola commented 3 years ago

@GaretJax Your change request has been pushed :)

Rastopapola commented 3 years ago

@FinalAngel @GaretJax Ping! ;) Any news or thoughts on this?

GaretJax commented 3 years ago

Looks good (note that copy iterates over the dict, so we now have two loops, instead of a single loop with an if, but it should not matter performance-wise).

Rastopapola commented 3 years ago

Looks good (note that copy iterates over the dict, so we now have two loops, instead of a single loop with an if, but it should not matter performance-wise).

Oh, my humble apology on this. I was hoping for some high performance pointer C magic in the background. Thanks for the hint, I will remember this the next time.

And thanks for approving the pull request! Feeling pretty honored now, since that's my first approved open source contribution on another project. When can we expect the changes being available via pip? :)

Rastopapola commented 3 years ago

@FinalAngel @GaretJax Sorry for another disturbance, but I wanted to ask when the PR will be merged, so the feature will be available via pip?

GaretJax commented 3 years ago

This has been released as 1.1.0

Rastopapola commented 3 years ago

Thank you so much!