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

Config to save requested claims #12

Closed
wants to merge 1 commit into from

Conversation

brianmay
Copy link

@brianmay brianmay commented Nov 7, 2021

Fixes #7

@brianmay
Copy link
Author

brianmay commented Nov 7, 2021

Note this will break existing sessions. Not sure how much that matters...

@brianmay
Copy link
Author

brianmay commented Nov 7, 2021

dialyzer passes. Which is somewhat amazing.

I can't test act stuff, but hopefully I didn't break it (except for existing sessions). Everything else looks good to me.

@tanguilp
Copy link
Owner

tanguilp commented Nov 7, 2021

The problem with this approach is that cookies have size limit, and authentication cookies can hold authentication info of several sessions. This could be a problem when using the COOKIE backend (data stored in the cookie and not in a backend).

This would be nice if we could configure which claims are saved, and which ones are not.

@brianmay
Copy link
Author

brianmay commented Nov 7, 2021

This PR does allow specifying which claims you do/don't want to keep. Not sure I would ever need any more then this?

But yes, if it supported a function as well as a list, I guess it could be more generic.

@tanguilp
Copy link
Owner

tanguilp commented Nov 7, 2021

There's also the alternative which consist in giving the opportunity to the user to to whatever he wants with the claims (and tokens) and the Plug.Conn.t, for example saving them in its own session. See #7

@brianmay
Copy link
Author

brianmay commented Nov 7, 2021

Feel free to take this PR and modify more in line of what you want.

@tanguilp
Copy link
Owner

tanguilp commented Nov 7, 2021

I've taken the second approach: in the conn_callback branch the token callback takes the conn as a parameter and returns it. It should be sufficient to set whatever you want in the session.

https://github.com/tanguilp/plugoid/tree/conn_callback

Feel free to test it and give me feedback.

@brianmay
Copy link
Author

brianmay commented Nov 7, 2021

Thanks. Will test ASAP.

I imagine this means the callback can put data in the default phoenix session, which would solve one of the problems we were predicting with the live view not having access to the required session details.

Wondering if the special case code for subject is still required though. We might want to include it in the default session also, so that live views have access. Which would mean we get two copies.

@brianmay
Copy link
Author

brianmay commented Nov 7, 2021

I am going to put this URL here so I don't have to keep finding it. Seems the appropriate place anyway:

https://github.com/tanguilp/plugoid/compare/conn_callback

@brianmay
Copy link
Author

brianmay commented Nov 7, 2021

So far, your branch seems to work fine.

@brianmay brianmay mentioned this pull request Nov 7, 2021
@brianmay
Copy link
Author

brianmay commented Nov 8, 2021

If you want to merge your branch, we can then close this PR.

@brianmay
Copy link
Author

Will close and summarise in #7.

@brianmay brianmay closed this Nov 17, 2021
@brianmay brianmay deleted the brianmay/issue7 branch November 17, 2021 21:22
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.

Allow setting more claims from the ID token to the Plug.Conn.t()
2 participants