Skip to content

Revert "Merge pull request #4901 from xihai01/devise-jwt"#4934

Merged
compwron merged 3 commits intomainfrom
revert-xihai01-devise-jwt
Jul 1, 2023
Merged

Revert "Merge pull request #4901 from xihai01/devise-jwt"#4934
compwron merged 3 commits intomainfrom
revert-xihai01-devise-jwt

Conversation

@compwron
Copy link
Copy Markdown
Collaborator

@compwron compwron commented Jul 1, 2023

This reverts commit fce43a9, reversing changes made to 79a6caf.

reverts #4901

as per https://rubyforgood.slack.com/archives/G01143RQ7DG/p1688235741466369?thread_ts=1688179662.135939&cid=G01143RQ7DG

Sean (He/Him)
:ruby: 3 hours ago
I took at look at that PR and it feels like it is being build wrong. I’m more than a little worried about their cors.rb initializer as it is pretty much opening up the app and is a pretty substantial security issue. (edited)

Sean (He/Him)
:ruby: 3 hours ago
It is pretty much opens up the app by negating all the built in protection that rails gives against CSRF attacks.
New

Sean (He/Him)
:ruby: 3 hours ago
Honestly, if that has been deployed I think I might revert that PR and deploy again. Obviously, I don’t think people are out there actively trying to attack this app but given the vulnerable population it serves…

This reverts commit fce43a9, reversing
changes made to 79a6caf.
@github-actions github-actions Bot added dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code Tests! 🎉💖👏 labels Jul 1, 2023
@compwron compwron requested a review from seanmarcia July 1, 2023 21:19
@compwron compwron merged commit a6e247c into main Jul 1, 2023
@compwron compwron deleted the revert-xihai01-devise-jwt branch July 1, 2023 22:36
@xihai01
Copy link
Copy Markdown
Collaborator

xihai01 commented Jul 4, 2023

Hey @seanmarcia!
I'm having trouble opening the slack link - can you let me know the issue we are having with this PR so I can take a look into it? Thanks 😄

@seanmarcia
Copy link
Copy Markdown
Member

Hey @xihai01 the slack link on the Ruby for Good site should be working, you may be using an old one, you can find an up to date link on this page: https://rubyforgood.org/join-us

Linda posted above the major concern I had with the cors.rb file that was added which introduced a major security vulnerability to the application. My other comment about how it "feels" like this is being built wrong is from an initial look at the code that is being introduced and how it doesn't appear to be encapsulating the code for a mobile application. This is just a "feel" as I have not been following what you are doing. It could be that you have an excellent plan and I am just over the shoulder commenting here -- do you have any design docs for how you are building things that you can point me at?

@xihai01
Copy link
Copy Markdown
Collaborator

xihai01 commented Jul 4, 2023

@seanmarcia
When I was implementing the authentication for the app, I didn't really create any design docs as I am still fairly new to mobile dev. I followed this article here -> https://medium.com/ruby-daily/a-devise-jwt-tutorial-for-authenticating-users-in-ruby-on-rails-ca214898318e
So on the server side, I used the devise-jwt gem. As for the cors file, I guess I used "*" to make testing easier. In production, it would be replaced with the URL to casa?

We also have a public repo hosting the frontend written in react native. Here is the link to it -> https://github.com/ctc-casa-ios/ios-app
On the homepage, we have an up to date list of API routes implemented. Then on the wiki page there are some files hosting figma designs, navigation structure and state management.

On the frontend, the authentication is handled using a simple implementation of redux with useContext + useReducer hooks.

@seanmarcia
Copy link
Copy Markdown
Member

In essence, you are creating API for a mobile app to consume.

You should be isolating everything to the API and the routes for that API.

The convention is for your controllers/routes/etc to all be in an /api route. Since this is the first mobile api the convention should be /api/v1/* and then when we deprecate this api and launch a new one, it will be /api/v2/* and so on. In general if we are altering existing controllers/files/concerns for an API that is usually a sign that it may not being built correctly.

Then your cors file should look something like:

    resource '/api/v1/*', # adjust this to match your API endpoints
      headers: :any, 
      methods: [methods you are going to use],
      expose: %w[Authorization]

Hopefully this makes sense. :)

@xihai01
Copy link
Copy Markdown
Collaborator

xihai01 commented Jul 6, 2023

@seanmarcia ok, that makes sense. thanks! I'll work on this

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

Labels

dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code Tests! 🎉💖👏

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants