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

Cookie authentication not properly working #525

Closed
joepio opened this issue Oct 27, 2022 · 7 comments · Fixed by #528
Closed

Cookie authentication not properly working #525

joepio opened this issue Oct 27, 2022 · 7 comments · Fixed by #528
Assignees
Labels
bug Something isn't working server atomic-server
Milestone

Comments

@joepio
Copy link
Member

joepio commented Oct 27, 2022

In the current release, I get a 401 after opening my drive. The error from the server shows that it has not found an Agent, which should imply there is no cookie set. However, there is in fact a cookie sent.

I'm not sure why this isn't working, but I suspect that one of the functions in the cookie parse logic is throwing an error which is thrown away with .ok(), which makes it impossible to see what is going on. So even though I'm not sure what is causing the issue, a good first step would be to refactor get_auth_from_cookie and make sure it gets rid of the .ok calls.

The reason I think the parsing fails, is because in production, we have multiple cookies. Google analytics is probably responsible for setting the multiple cookies. Our implementation should deal with this.

Screenshot 2022-10-27 at 08 59 15

Using actix_web::cookie might be a good idea.

However, even if we update the back-end, we might still have an issue with atomic-data-browser. When it sets the cookies, it may remove others (e.g. google analaytics) <- That is false.

@rescribet
Copy link
Contributor

The cookie was set with a fixed path so there could only be one atomic_session cookie, other cookies don't matter.

The only edge case which I'm seeing is that you've added the Domain property, which wasn't accounted for in the implementation. That case should only be hit when dealing with subdomains.

it may remove others

No it may not, assigning to document.cookie should adhere to the set-cookie parsing logic, which does not remove other cookies

rescribet pushed a commit that referenced this issue Oct 27, 2022
rescribet pushed a commit that referenced this issue Oct 27, 2022
@joepio
Copy link
Member Author

joepio commented Oct 27, 2022

Your last commit is called allow origin authentication, does that mean that the client should be able to set a cookie for a domain / origin different from the main URL? Because that was what I tried to do in atomic-data-browser, but I never got the cookie to actually send there.

See also: atomicdata-dev/atomic-data-browser#253

@rescribet
Copy link
Contributor

Since cookies don't change every request, the subject in question is a constant containing the server url. From what I recall is that the solution we came up with was that the root subject should be valid for all the children as well.

@joepio
Copy link
Member Author

joepio commented Feb 13, 2023

Looks like there's still something wrong with the cookie parser, it's still signing in as public even if a proper cookie is set, when combined with another cooke (google analytics in my case).

Also: why is there a _ga cookie here? I'm not setting them. It isn't set on initialize. Could be one of the images. Haven't checked origin, though.

@joepio joepio reopened this Feb 13, 2023
@joepio joepio assigned joepio and unassigned rescribet Feb 13, 2023
@joepio joepio added this to the v1.0.0 milestone Feb 13, 2023
@joepio joepio added bug Something isn't working server atomic-server labels Feb 13, 2023
joepio added a commit that referenced this issue Feb 13, 2023
@joepio
Copy link
Member Author

joepio commented Feb 13, 2023

I've added a test (single cookie value at the moment) and improved error handling. Now I still need to add a failing test for the google analytics cookies, but then I first need to actually set these here.

@joepio
Copy link
Member Author

joepio commented Feb 13, 2023

Another case we should handle: what if the user has multiple atomic_session cookies?

  • Sign in on atomicdata.dev
  • Sign in on staging.atomicdata.dev
  • Open a resource on staging.atomicdata.dev

Two domains, two cookies.

Now the server will receive two cookies, and will parse the first one, leading to an error:

Wrong requested subject, expected https://staging.atomicdata.dev was https://atomicdata.dev

Screenshot 2023-02-13 at 11 07 28

So how should we deal with this?

Server handles multiple cookies

If there is a subject mismatch, check if there is another cookie. This means we update the session_cookie_from_handler to return multiple cookies, instead of just one.

Client removes existing cookie

We set cookies on top level domain only

joepio added a commit that referenced this issue Feb 13, 2023
@joepio
Copy link
Member Author

joepio commented Feb 13, 2023

I've made the server try all the cookiesatomic_session cookies, until it finds one it can work with. Test is added too.

@joepio joepio closed this as completed Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server atomic-server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants