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 support for iOS Local Network permission #687

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

geraintwhite
Copy link
Contributor

@geraintwhite geraintwhite commented May 19, 2022

Summary

Fix #509

Test Plan

Tested in my app by calling the following and verifying that the permission prompt is displayed:

Permissions.check(PERMISSIONS.IOS.LOCAL_NETWORK_PRIVACY)
Permissions.request(PERMISSIONS.IOS.LOCAL_NETWORK_PRIVACY)

Compatibility

OS Implemented
iOS
Android

Checklist

  • I have tested this on a device and a simulator (local network privacy not available on simulator)
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

@geraintwhite geraintwhite force-pushed the local-network-permission branch 2 times, most recently from 50491d8 to 80ee792 Compare May 19, 2022 15:03
README.md Outdated Show resolved Hide resolved
Comment on lines +14 to +15
s.ios.deployment_target = "10.0"
s.tvos.deployment_target = "11.0"

Choose a reason for hiding this comment

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

I have not personally checked, but have you checked all APIs are actually available on ios and tvos? out of curiosity, are there any restrictions for ipados / macCatalyst or macos? (just thinking cross-platform, though it's not a strict requirement or anything)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't checked yet, just based the podspec on one of the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the docs of NetService it is available on all platforms - but it has been deprecated with no obvious replacement.

Choose a reason for hiding this comment

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

Oh that's unfortunate. And you're right, no real guidance. You probably found this as well but the only thing I found was the lower level stuff here https://developer.apple.com/library/archive/samplecode/DNSSDObjects/Introduction/Intro.html#//apple_ref/doc/uid/DTS40011371-Intro-DontLinkElementID_2 - everything links to it

Copy link

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I think this is neat so I'm leaving comments with the intention of being helpful but I haven't personally investigated them (apologies) or pulled this and tried it (apologies again), and @zoontek is the repo boss, so it's all up to him anyway. Just wanted to see if I could help move it along. Cheers

@zoontek
Copy link
Owner

zoontek commented May 20, 2022

Thanks a lot for this one! However, is it possible for you to migrate the module to ObjC? There's already too much languages on this repository (ObjC, Java, TypeScript, C#…).

@geraintwhite
Copy link
Contributor Author

geraintwhite commented May 20, 2022

Thanks a lot for this one! However, is it possible for you to migrate the module to ObjC? There's already too much languages on this repository (ObjC, Java, TypeScript, C#…).

Converted to objective c and tested it still works d2d598f

@geraintwhite geraintwhite marked this pull request as ready for review May 20, 2022 12:02
@geraintwhite
Copy link
Contributor Author

It's occurred to me that checking the permission will only return an accurate result after requesting it, as the only way to check it is by requesting it first and storing the result (https://stackoverflow.com/questions/63940427/ios-14-how-to-trigger-local-network-dialog-and-check-user-answer/64242745#64242745).

I can think of three possible solutions:

  • leave it as is so the check result will be inaccurate until it is requested
  • disable Permissions.check for this permission
  • persist the result somewhere other than a class property.

@zoontek
Copy link
Owner

zoontek commented May 26, 2022

@grit96 The library has a builtin escape hatch for these cases:

+ (void)flagAsRequested:(NSString * _Nonnull)handlerId {

Get isFlaggedAsRequested value on check. If it's false, return "NotDetermined". If it's true, you can perform a request to get the status (don't forget to call flagAsRequested on request).

You can have an usage example in the FaceID handler: https://github.com/zoontek/react-native-permissions/blob/2d645ff4f3dfc6db9686bc2fd2877bd049b8043e/ios/FaceID/RNPermissionHandlerFaceID.m

@geraintwhite
Copy link
Contributor Author

@zoontek something like this?

#import "RNPermissionHandlerLocalNetworkPrivacy.h"
#import "LocalNetworkPrivacy.h"

@implementation RNPermissionHandlerLocalNetworkPrivacy

+ (NSArray<NSString *> *_Nonnull)usageDescriptionKeys {
  return @[ @"NSLocalNetworkUsageDescription" ];
}

+ (NSString *_Nonnull)handlerUniqueId {
  return @"ios.permission.LOCAL_NETWORK_PRIVACY";
}

- (void)checkWithResolver:(void (^_Nonnull)(RNPermissionStatus))resolve
                 rejecter:(void(__unused ^ _Nonnull)(NSError *_Nonnull))reject {
  if (![RNPermissions isFlaggedAsRequested:[[self class] handlerUniqueId]]) {
    return resolve(RNPermissionStatusNotDetermined);
  }

  LocalNetworkPrivacy *local = [LocalNetworkPrivacy new];
  [local checkAccessState:^(BOOL granted) {
    resolve(granted ? RNPermissionStatusAuthorized : RNPermissionStatusDenied);
  }];
}

- (void)requestWithResolver:(void (^_Nonnull)(RNPermissionStatus))resolve
                   rejecter:(void (^_Nonnull)(NSError *_Nonnull))reject {
  [RNPermissions flagAsRequested:[[self class] handlerUniqueId]];
  [self checkWithResolver:resolve rejecter:reject];
}

@end

@zoontek
Copy link
Owner

zoontek commented May 26, 2022

@grit96 Exactly. With your current request code:

- (void)requestWithResolver:(void (^ _Nonnull)(RNPermissionStatus))resolve
                   rejecter:(void (^ _Nonnull)(NSError * _Nonnull))reject {
  LocalNetworkPrivacy *local = [LocalNetworkPrivacy new];
  [local checkAccessState:^(BOOL granted) {
      [RNPermissions flagAsRequested:[[self class] handlerUniqueId]];
      [self checkWithResolver:resolve rejecter:reject];
  }];
}

@geraintwhite
Copy link
Contributor Author

geraintwhite commented May 26, 2022

I've moved the network request from requestWithResolver into checkWithResolver so it doesn't get called twice.

@zoontek
Copy link
Owner

zoontek commented May 26, 2022

I recommend the opposite : perform a request in check (for consistency)

@nilrezaee88

This comment was marked as off-topic.

@bintoll
Copy link

bintoll commented Jun 2, 2022

Hello! Thanks for this PR, I've tested this and find out that straight after permission request it immediately returns BLOCKED as a response while user request alert is still on screen and user did not allowed or denied it, so it just do not wait untill user will process with any action

@geraintwhite
Copy link
Contributor Author

@bintoll hmm you are correct - it's happening because there is no way to detect when the user presses decline, so we have a timer which automatically returns denied after 2s. There does seem to be an alternative using NWBrowser but I can't find any objc equivalent API (https://stackoverflow.com/a/67758105/2468126).

@bintoll
Copy link

bintoll commented Jun 15, 2022

@grit96 Got it, that generally is fine, it will not lead to the best UX for user (in case if we would like to alert user after declining permission and allow him to navigate to device settings), but requesting permission itself works nice.
I would recommend to mention this behavior somewhere in readme so the flow would be clear and developers would not create issues regarding this functionality.
Thank you again for implementing this!

@mikehardy
Copy link

after a decline you can re-check (but not re-request!) in order to differentiate perhaps? I have not looked that closely though, if that idea doesn't fit at all, apologies

@bintoll
Copy link

bintoll commented Jun 15, 2022

@mikehardy It seems that it could work, it will be needed to setup an interval to check status consistently until the status will be changed (when user finally will press allow or decline). Will check it al little bit later.
But anyway, as long as this flow breaks the common behavior of requesting permissions in this lib, it's better to mention this.

@nilaydigiwhiz
Copy link

Why this PR is not merged yet?

@zoontek
Copy link
Owner

zoontek commented Jul 15, 2022

@nilaydigiwhiz https://jacobtomlinson.dev/posts/2022/dont-be-that-open-source-user-dont-be-me/

I do almost free, volunteer work on this library (and others) and have a full time job, that's why.

Also, every time someone say things like "Why is this not done yet?", "My company need this!" (for free, obviously) or else, my motivation doesn't go up, isn't it weird? 😄

Anyway, I'm currently on holidays ☀️ and will probably take some time from it to review it / test it on several devices and simulators / release it (and later, maintain it and reply to issues - just in case you though that being an open source maintainer is just pressing "merge" buttons)

@vitalyiegorov
Copy link

@zoontek Thanks for your effort and for this great library! Did you have time to review/test this PR? Maybe I can help in someway with this?

@zoontek
Copy link
Owner

zoontek commented Sep 8, 2022

@vitalyiegorov Not yet.

FC04FED5-4F72-4651-AB62-6D5786A53B14

Sure, you can try and confirm if it works well.

@zoontek
Copy link
Owner

zoontek commented Sep 11, 2022

I took a check and finally try the @grit96 solution. IMHO this is not enough (and I'm afraid it can't be better)
The permission must be requested using the library or it will fails on check (ex: here react native in dev mode will prompt for permission at app start)

Also, this indeed use deprecated APIs so it's not future proof. I'd rather not merge this as it will create confusions and issues.

RPReplay_Final1662917452.MP4

[self.service stop];
}

- (void)checkAccessState:(void (^)(BOOL))completion {

Choose a reason for hiding this comment

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

Is this as reliable as:
(It seems you altered the code from: https://stackoverflow.com/a/65800221)

    func checkAccessState(completion: @escaping (Bool) -> Void) {
        self.completion = completion
        
        timer = .scheduledTimer(withTimeInterval: 2, repeats: true, block: { timer in
            guard UIApplication.shared.applicationState == .active else {
                return
            }
            
            if self.publishing {
                self.timer?.invalidate()
                self.completion?(false)
            }
            else {
                self.publishing = true
                self.service.delegate = self
                self.service.publish()
                
            }
        })
    }

@Audrey-Ann
Copy link

Any updates here?

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.

iOS14 - Local Network permission support
9 participants