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

added disable_cookies as default for api actions #535

Merged
merged 3 commits into from
Aug 10, 2020

Conversation

confact
Copy link
Contributor

@confact confact commented Jul 18, 2020

Added disable cookies as default for API actions

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

I'm not sure about this one. It would make sense if you have an API only app where let's say the client is a mobile app or something, right? You wouldn't need cookies or sessions or anything because that's all handled on the mobile app side. But what if you have a full lucky app where your front-end is basically a react app, and it calls back to an API for everything? I'm thinking in that case you would want access to cookies and sessions. Though, I guess some people might build their react app using say CRA, and have the API separate which then goes back to the first example....

I'm wondering which is the more common use case here 🤔 I guess it is easier to remove this line than to dig through docs figuring out how to add it... so maybe this is good. 😅 I'll defer to Paul for this

@@ -1,5 +1,6 @@
# Include modules and add methods that are for all API requests
abstract class ApiAction < Lucky::Action
disable_cookies
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a little comment on what this is removing? It removes sessions too, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment @jwoertink - feel free to change it to something better :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching this and adding it. It makes sense to disable cookies by default IMO! Is this right that "Cookie header will still be sent"? I thought the header wouldn't be sent if cookies are disabled?

Copy link
Contributor Author

@confact confact Aug 4, 2020

Choose a reason for hiding this comment

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

@paulcsmith you are right. I must have been confused of the request headers. I changed the comment now.

Feel free to merge it if it looks good :)

@confact
Copy link
Contributor Author

confact commented Jul 19, 2020

@jwoertink Now API action's current user is using JWT as default and should not need cookies and sessions. If they want to use API action with cookies and sessions they would need to use the module as in the Browser action anyway.

Like in API Action we include:

include Api::Auth::Helpers
include Api::Auth::RequireAuthToken

But in browser action we include:

include Auth::RequireSignIn

I can add comments for it though to know why it is there and can be removed if they want to use session and cookies (they would need to change Auth::Helpers and RequireAuthToken as well)

@confact confact requested a review from jwoertink July 19, 2020 15:55
@jwoertink
Copy link
Member

Ah, that makes sense. 👍

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Thanks @confact! Looks great

@paulcsmith paulcsmith merged commit 4e5b652 into luckyframework:master Aug 10, 2020
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