Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve permissions of .togglrc file #337

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

joe-niland
Copy link
Contributor

What

  • add note about pip dev mode in contributing guide
  • set permissions each time config file is persisted

Why

  • this file has creds or an API token so it should be restricted to reading by the owner only

Refs

- chore: add note about pip dev mode in contributing guide
Copy link
Owner

@AuHau AuHau left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the PR 🙏

@AuHau AuHau merged commit d952f1e into AuHau:master Mar 5, 2025
0 of 5 checks passed
@AuHau
Copy link
Owner

AuHau commented Mar 5, 2025

Actually, I might have been a bit too rushed with merging. Now, when I think about it, I am not sure if updating the access rights on every update is the way to go. IMHO, this should be done only when the file is being created by toggl-cli eq. when it bootstraps the configuration file. Then we should leave it up to the user if he wants to modify it for whatever use-case he needs it for and not enforce this set of rights even though yes it is the optimal one.

WDYT @joe-niland ?

@joe-niland
Copy link
Contributor Author

I am not sure if updating the access rights on every update is the way to go. IMHO, this should be done only when the file is being created by toggl-cli eq. when it bootstraps the configuration file.

I had this thought, but it seemed like that would have taken more refactoring to perform this only during bootstrap. I only spent a short time looking at the code so it's quite possible I missed a good place to make the change in this way.

Then we should leave it up to the user if he wants to modify it for whatever use-case he needs it for and not enforce this set of rights even though yes it is the optimal one.

I also thought about this because forcing and resetting permissions can be confusing, and we can't predict every possible scenario. However, I figured that in the general case most people should have it this way.

Happy to spend some more time on it if you'd like to do things differently.

@AuHau
Copy link
Owner

AuHau commented Mar 5, 2025

Alright! No worries, I have actually good idea about how to do it, so I will patch it in ;-) Just wanted to ask for your input.

@joe-niland
Copy link
Contributor Author

Tbh I didn't expect that this change would be ideal - it was more of a starting point.

Thanks for asking. Looking forward to seeing your solution!

@AuHau
Copy link
Owner

AuHau commented Mar 5, 2025

Here it is #337

@joe-niland
Copy link
Contributor Author

Here it is #337

Nice! That is more efficient.

@AuHau
Copy link
Owner

AuHau commented Mar 5, 2025

lol, linked your old PR 😅 But I guess you found my follow-up one ;-)
#339

Anyway, thanks for your contribution 🙏

@joe-niland
Copy link
Contributor Author

Yep found it!

No worries! I'm just starting to use this so maybe I'll have some others.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.togglrc permissions should be more restrictive
2 participants