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

Replace deprecated SecKeychain API with SecItem #33

Merged
merged 7 commits into from
Aug 26, 2023

Conversation

Nevon
Copy link
Contributor

@Nevon Nevon commented Jul 5, 2023

Consider this a bit of a draft, as I'm definitely not a C++ developer and have never developed against the Mac APIs before, so there may be consequences of this change that I'm not aware of. The tests are passing locally, so it seems to work.

@hrantzsch
Copy link
Owner

This is great, thanks a lot! I'll try to review as soon as possible, but it'll be a few days at least.

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch coverage: 34.56% and project coverage change: -14.52% ⚠️

Comparison is base (c593cce) 63.03% compared to head (4582b65) 48.51%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #33       +/-   ##
===========================================
- Coverage   63.03%   48.51%   -14.52%     
===========================================
  Files           3        3               
  Lines         165      202       +37     
  Branches       51       77       +26     
===========================================
- Hits          104       98        -6     
- Misses         22       45       +23     
- Partials       39       59       +20     
Flag Coverage Δ
unittests 48.51% <34.56%> (-14.52%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/keychain_mac.cpp 44.52% <34.56%> (-20.62%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hrantzsch
Copy link
Owner

The failing build and test job is unrelated to your changes, I'll look into it.

Copy link
Owner

@hrantzsch hrantzsch left a comment

Choose a reason for hiding this comment

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

I finally got around to taking a closer look at this. Again, thanks a lot, this is great (and really needed since that API has been deprecated for a while...)

I left some comments, my main worry is that CF*Create functions could fail.

Besides that, there are two things I'd like improved, but I don't mind merging the PR without them and doing it afterwards:
The first would be getting rid of the CFRelease tirades. For example, we could use RAII to ensure the CFRef is released when it goes out of scope.
The other would be reduce the code duplication of creating those query dicts. I'm not sure if this pays out in the end, because they keys are always different. But then again class, account, service; and converting the account and service strings, is always the same.

Let me know if you want to address these refactorings in this PR.

src/keychain_mac.cpp Outdated Show resolved Hide resolved
src/keychain_mac.cpp Outdated Show resolved Hide resolved
src/keychain_mac.cpp Outdated Show resolved Hide resolved
@Nevon
Copy link
Contributor Author

Nevon commented Jul 26, 2023

Gave it a shot to address your feedback. Most of it was fairly straightforward, but figuring out how to use RAII was not easy considering I've written more lines of C++ today than in the rest of my life, so let me know if I did something super weird. 😅 I would recommend looking at the individual commits first, as I tried to address each piece of feedback individually.

I tried to make the AutoCF*Ref class generic, to avoid duplication, but there were a lot of small differences that made it seem not worth it.

@hrantzsch
Copy link
Owner

Amazing! I like it.

I want to think about the AutoRefs a bit more and play around with it. I found some examples how others deal with the problem, like various implementations of CFUniquePtr. I'll get back to you 😅

@hrantzsch
Copy link
Owner

I tried some things and I've come up with something that I hope should work. Admittedly, I haven't tested it in the real code. But could we do something like this? https://godbolt.org/z/cranneTnP
It should also work for all the CFRef types.

@hrantzsch
Copy link
Owner

Hi @Nevon, I suggest I merge all except for the last commit (up until d0fa42a) and do the RAII refactoring later. Is that fine with you?

@Nevon
Copy link
Contributor Author

Nevon commented Aug 17, 2023

That's perfectly fine by me. I ended up using Rust for this instead, so I didn't get around to finishing this up.

@hrantzsch
Copy link
Owner

Good choice ;) Thanks for taking the initiative to address this!

@hrantzsch hrantzsch merged commit ec80ca8 into hrantzsch:master Aug 26, 2023
22 of 24 checks passed
@barracuda156
Copy link

@hrantzsch @Nevon It would be awesome to have a fallback code, otherwise this breaks the build, unfortunately.
An earlier version built fine with one trivial fix: #38

P. S. Sorry, I realize that I am a bit late with this, but I came to know about this software just today, since it turned out to be a dependency for a port I want to build.

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