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

[ADD] Download user iCal with res.users.apikeys in URL. #53986

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amh-mw
Copy link
Contributor

@amh-mw amh-mw commented Jul 2, 2020

Description of the issue/feature this PR addresses:

There is no way for a user to synchronize their calendar directly from Odoo.

Current behavior before PR:

Two-way CalDAV synchronization was retired in OpenERP 7?

Desired behavior after PR is merged:

One-way read-only iCalendar synchronization from Odoo using a specially crafted URL containing a res.users.apikeys value. Calendar key setup has been added to account security tab in user preferences.

Screenshot 2023-04-20 at 2 23 13 PM

Screenshot 2023-04-20 at 2 23 43 PM


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

addons/calendar/models/ir_http.py Outdated Show resolved Hide resolved
addons/calendar/controllers/main.py Outdated Show resolved Hide resolved
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Jul 2, 2020
@tivisse
Copy link
Contributor

tivisse commented Jul 9, 2020

@amh-mw Thanks for the feedback.

Well, I wouldn't approve this PR at the first sight for the following reason:

  • I'm not fond of adding a new authentication mechanism (see: https://github.com/odoo/odoo/pull/53986/files#diff-e6bce60073e5b2c72561523cfefbc79dR34). It seems fishy to introduce a new mechanism without any justifications. The commit message lack a real purpose, a complete specification, and an argumentation about why this is impossible to use the standard auth mechanism. Starting from that, I wouldn't merge it even if it was correct.

  • If I get it correctly, you refer the basic authentication method in http (See: https://en.wikipedia.org/wiki/Basic_access_authentication). It means that the request from the outside contains your Odoo password in the headers, which seems alors very fishy. I didn't take time to imagine some use cases, but there is a risk of introducing a security flaw over here.

I'm still open to the discussion ;)

Cc @odony maybe you have another point of view to bring here.

Have a nice day,

Yannick.

@amh-mw
Copy link
Contributor Author

amh-mw commented Jul 9, 2020

@tivisse I am currently migrating to Odoo from another CRM and one of the parity features currently lacking is the ability of sales/service employees to synchronize their work calendar directly to their phones. I'd be happy to use (and implement) any authentication mechanism that is broadly supported across iOS and Android calendar applications (research into this topic TBD). We are HTTPS-only, so the HTTP Basic headers are encrypted at that layer at least, but given that Odoo supports running in non-HTTPS mode, I agree with your concern. Comparing this to the XML-RPC External API, it seems that password is sent on every call?

https://www.odoo.com/documentation/13.0/webservices/odoo.html#list-records:
models.execute_kw(db, uid, password, ...)

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Jul 9, 2020
@amh-mw
Copy link
Contributor Author

amh-mw commented Jul 9, 2020

I read up on RFC 3744, RFC 7616, Passlib and Werkzeug for a bit today. The modern pbkdf2_sha512 hashing of our passwords does not appear to be an algorithm supported by HTTP Digest, so that seems like a dead end for authentication.

I notice that Odoo uses access tokens for various behaviors (calendar invites, document signing), perhaps a per-user access token could be appropriate here for read-only calendar access? It could still be SHA-512 hashed with a nonce on the wire via HTTP Digest.

@tivisse
Copy link
Contributor

tivisse commented Jul 10, 2020

Hi @amh-mw

Thanks for your answer.

You're right. Indeed https encrypts the headers. And indeed Odoo allows http. This could be a leak, as for xml-rpc, or even simple login. But we don't want to add a new authentication mechanism if it's not the only solution that works.

I was looking at the way Google Calendar works to share easily a calendar, and I found that you may generate a private iCal address with a token from which you can retrieve all your events.

Something like
https://calendar.google.com/calendar/ical/bruce.wayne%40gmail.com/private-my-super-token/basic.ics

This would imply to store my-super-token on the res.users table for example, while handling correctly the privacy of this information (groups on the field), and maybe a button to reset the token.

See:
image

What do you think ?

cc @mart-e

Have a nice day,

Yannick.

@amh-mw
Copy link
Contributor Author

amh-mw commented Jul 10, 2020

@tivisse That generally seems reasonable. Let me work it up.

@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Jul 10, 2020
addons/calendar/models/res_users.py Outdated Show resolved Hide resolved
addons/calendar/models/ir_http.py Outdated Show resolved Hide resolved
addons/calendar/models/ir_http.py Outdated Show resolved Hide resolved
@amh-mw
Copy link
Contributor Author

amh-mw commented Jul 10, 2020

I'm not in love with the latest prototype, especially that the token is dumped into the logs on every call. I also prototyped this with an URL signature of /calendar/basic.ics?login=LOGIN&token=TOKEN and that is also dumped into the logs.

What about HTTP Basic or Digest against the access token instead of the password?

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Jul 10, 2020
@amh-mw
Copy link
Contributor Author

amh-mw commented Jul 10, 2020

I read up on RFC 2617 for a bit and implemented HTTP Digest for qop=auth, algorithm=SHA-512. I feel pretty good about this one, testing in iOS Calendar now.

@amh-mw amh-mw force-pushed the ics branch 3 times, most recently from 82ded4d to d559848 Compare July 10, 2020 20:17
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Jul 10, 2020
@tivisse
Copy link
Contributor

tivisse commented Jul 14, 2020

Hi @xmo-odoo

After a small discussion with @odony it appear that this whole new feature is overlapping (at least partly) your work on API keys.

We should check with @antonylesuisse to decide if reject this or try to integrate it into your work.

What do you think ?

Thanks !

@xmo-odoo
Copy link
Collaborator

xmo-odoo commented Jul 14, 2020

After a small discussion with @odony it appear that this whole new feature is overlapping (at least partly) your work on API keys.

The access_token seems to be overlapping with the "api keys" idea, especially with the addition of scopes to keys by @Florimond

Though the biggest blocker so far is obviously that the entire thing is still in limbo waiting for @odony's seal of approval.

@amh-mw
Copy link
Contributor Author

amh-mw commented Jul 14, 2020

Any chance I could get a link to the api keys work? I didn't find anything relevant searching for is:open is:pr author:xmo-odoo archived:false. I would absolutely make use of scoped keys; as a workalike right now I have created "api users" with specific permissions to simulate this. I'm coding full time right now on nothing but Odoo, so let me know if there is any work along these lines that I could contribute to.

@amh-mw
Copy link
Contributor Author

amh-mw commented Jul 14, 2020

Returning to the original thread, my testing with Apple Calendar in the iOS Simulator results:

  • HTTP Basic
  • HTTP Digest algorithm=MD5
  • HTTP Digest algorithm=MD5-sess
  • HTTP Digest algorithm=SHA-256
    Initial 401, then calendar takes no subsequent action.
  • HTTP Digest algorithm=SHA-512
    Typo on my part, SHA-512 isn't on the IANA Hash Algorithms for HTTP Digest Authentication list.
  • HTTP Digest algorithm=SHA-512-256
    Initial 401, then calendar takes no subsequent action.

@xmo-odoo
Copy link
Collaborator

xmo-odoo commented Jul 14, 2020

Any chance I could get a link to the api keys work? I didn't find anything relevant searching for is:open is:pr author:xmo-odoo archived:false. I would absolutely make use of scoped keys; as a workalike right now I have created "api users" with specific permissions to simulate this. I'm coding full time right now on nothing but Odoo, so let me know if there is any work along these lines that I could contribute to.

@amh-mw it's part of the 2FA/TOTP work as (obviously) once totp is enabled we can't allow using passwords for RPC as that would defeat the purpose, so API keys of sort are introduced as part of that. I couldn't tell you where @Florimond is working on his extension and scope system.

@amh-mw

This comment was marked as outdated.

@C3POdoo C3POdoo requested a review from a team January 5, 2023 20:04
@Julien00859 Julien00859 removed request for Julien00859 and a team January 16, 2023 12:17
@amh-mw amh-mw force-pushed the ics branch 9 times, most recently from 4017ea6 to 95ea87a Compare April 19, 2023 17:43
@amh-mw

This comment was marked as resolved.

@amh-mw amh-mw marked this pull request as draft April 19, 2023 17:58
@amh-mw amh-mw force-pushed the ics branch 5 times, most recently from 0d102ed to 68ecf4d Compare April 20, 2023 17:36
@amh-mw amh-mw marked this pull request as ready for review April 20, 2023 18:12
As a user, I would like to be able to subscribe to my work
calendar from my phone.
@C3POdoo C3POdoo requested review from a team and Julien00859 and removed request for a team April 20, 2023 18:13
Copy link
Member

@Julien00859 Julien00859 left a comment

Choose a reason for hiding this comment

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

Review for addons/calendar_ical/models/ir_http.py only, LGTM. I don't know for the feature itself though

@rdeodoo rdeodoo removed the request for review from a team April 28, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses CLA Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants