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

Apple signin for allowing multiple client ids #6523

Merged

Conversation

UnderratedDev
Copy link
Contributor

@UnderratedDev UnderratedDev commented Mar 21, 2020

I ruined my other branch #6394, & attempted to fix it by rewriting git history at 3am 🤦‍♂ which let's just say not work very well so I created this branch. It is solving the same problem as before. Here is a recap:

Added multiple client ids since if you sign in with apple from the watch it uses a different client ID than if you signed in from the website. I could not find any way to make the watch sign in with apple match the client id of the website. I looked online & seems like this is the expected behaviour so I updated parse server to be able to use multiple client ids & verify against them. I also fixed an issue that the jwt claims verify was throwing a error so the server would throw that exception 10 times, I made it throw a parse error which fixed the issue.

@dplewis

@UnderratedDev
Copy link
Contributor Author

UnderratedDev commented Mar 21, 2020

@dplewis I used your suggestion of using the jwt.verify function for the client ids. It works!!! After reading the docs, I also found you can use it for validating the subject, & issuer so I modified to do all of that in there as well! There is a problem however, that if you want to use the verify, you must pass in a token that can be decoded via one of apple's keys so it is really annoying to write tests & I tried to make it testable however I had to use my own apple client id, generated id's & tokens so I have placeholder text in the tests that have to be replaced if developers want to test apple sign in. If there is a better way, please let me know. Any advice is appreciated on this.

jwtClaims = jwt.verify(token, signingKey, { algorithms: algorithm, audience: clientId, issuer: TOKEN_ISSUER, subject: id, });

JWT Documentation

@codecov
Copy link

codecov bot commented Mar 21, 2020

Codecov Report

Merging #6523 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6523      +/-   ##
==========================================
- Coverage   94.02%   93.99%   -0.03%     
==========================================
  Files         169      169              
  Lines       11825    11826       +1     
==========================================
- Hits        11118    11116       -2     
- Misses        707      710       +3     
Impacted Files Coverage Δ
src/Adapters/Auth/apple.js 100.00% <100.00%> (ø)
src/RestWrite.js 93.48% <0.00%> (-0.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5977f3...44f57e9. Read the comment docs.

@dplewis
Copy link
Member

dplewis commented Mar 21, 2020

LGTM! Waiting for travis to run

@dplewis dplewis closed this Mar 21, 2020
@dplewis dplewis reopened this Mar 21, 2020
@dplewis
Copy link
Member

dplewis commented Mar 21, 2020

@UnderratedDev The travis build is failing. I don't know why it doesn't show in the PR.

https://travis-ci.org/github/parse-community/parse-server/builds/665207590

Just one quick test to fix.

@UnderratedDev
Copy link
Contributor Author

@dplewis for those tests, you need actual tokens signed by apple which is why they are failing, I tested it locally with my own tokens & they pass, & I don't want to leave my token in the test cases. Unless there is a way to fake sign an apple token, I'm not sure of a way to make tests for this. Using spy on JWT verify with fake claims as done previously bypasses the options in the verify function so I wasn't able to use that.

@dplewis
Copy link
Member

dplewis commented Mar 21, 2020

I've been thinking about that for a while now. For real test we can create our own token by using a key and signing it for testing.

const privateKey = fs.readFileSync("./parseKey.p8");
const token = jwt.sign({}, privateKey, {
 algorithm: "ES256",
 expiresIn: "60 days",
 audience: "https://appleid.apple.com",
 issuer: "TEAM_ID",
 subject: "com.mycustomdomain.webapp",
 keyid: "KEY_ID"
});

console.log("The token is:", token);

@dplewis
Copy link
Member

dplewis commented Mar 21, 2020

@UnderratedDev You can use xit instead of fit for the failing test and add // TODO: ... . I can look at it later.

@UnderratedDev
Copy link
Contributor Author

@UnderratedDev You can use xit instead of fit for the failing test and add // TODO: ... . I can look at it later.

Sure, I will update that. & yes, having our own key & way to generate tokens would be super useful!

…o comment to them stating what we need to do to fix them
@UnderratedDev
Copy link
Contributor Author

@dplewis I have updated the tests, adding a todo, & using xit

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Thanks for getting this in!

@dplewis dplewis merged commit dd2b64e into parse-community:master Mar 22, 2020
@dplewis dplewis changed the title updated 2 files for allowing multiple client ids Apple signin for allowing multiple client ids Mar 22, 2020
@TomWFox
Copy link
Contributor

TomWFox commented Apr 7, 2020

Hi @UnderratedDev (and possibly @andrewking0207),

Thanks for the improvements to Sign in with Apple!

A question on the community forum brought to my attention that there might be a update needed to the Parse Server guide here.

I've had a brief look through this PR and #6416 but to be honest I'm not sure if there are changes needed to guide, if you could let me know that would be great!

@UnderratedDev
Copy link
Contributor Author

UnderratedDev commented Apr 10, 2020

Hey @TomWFox, if you are sending tokens generated from apple devices (iPad, iPhone, Watch, etc) you can send them directly to parse, & configure the parse server (with an array of client id's if using different bundle identifiers). If however you are using web authentication then you do require a p8 file, & I have a server in between parse server & the client (web client) so the token that is generated from apple servers goes to my backend where I request a token from apple using the token from the user (request token), & then validate it using my p8 file on my backend, & then I use the parse method to authenticate with authdata.

I use this as my provider (this part can be done without a backend, it's just the parse JS SDK)

    authenticate: () => Promise.resolve(),
    restoreAuthentication() {
      return true;
    },

    getAuthType() {
      return "apple";
    }
};

parse.User._registerAuthenticationProvider(provider);

And then for the signing up & login I use this:

const user = new parse.User();
return user._linkWith(provider.getAuthType(), 
    {
        authData: authData
    }
)

I do wish there was a way for Parse to detect if there exists a user with the given authdata, as currently if the user has attempted to log in & then gone back, I cannot get the email from the user, as Apple only provides the email & name on the first token request therefore I check if parse has created a user & then delete it if there is no email & the user has been created within the last 5 minutes or so (not a great solution)

I can update the documentation if you'd like. I also wrote an authentication provider for apple sign in on iOS, so I can update the apple docs also or make a PR for that. And a side note, I can also implement the token request to apple with validation using a p8 if that's something people want. With that being said, it's only required for the web token so it's not for all devices :/.

Here's the base code for using a p8 file to generate a token to use with parse:

    return new Promise(
        (resolve, reject) => {
            // make it expire within 6 months
            const exp = Math.floor(Date.now() / 1000) + (86400 * 180);
            const claims = {
                iss: config.team_id,
                iat: Math.floor(Date.now() / 1000),
                exp,
                aud: "https://appleid.apple.com",
                sub: config.client_id
            };

            jwt.sign(claims, privateKey,
                {
                    algorithm: "ES256",
                    keyid: config.key_id
                },
                (error, token) => {
                    if (error) {
                        reject(`AppleAuth Error - Error occured while signing: ${ error }`);
                        return;
                    }

                    resolve(token);
                }
            );
        }
    );
}

I hope this is useful 👍

@TomWFox
Copy link
Contributor

TomWFox commented Apr 15, 2020

Thanks for that info 🙏 Sorry for my delayed response!

@TomWFox
Copy link
Contributor

TomWFox commented Apr 15, 2020

I can update the documentation if you'd like. I also wrote an authentication provider for apple sign in on iOS, so I can update the apple docs also or make a PR for that.

Yes a PR to the docs would be much appreciated and a PR for the iOS SDK would be fab too - I think someone got started on some helper methods like we have with Facebook & Twitter utils but I don't think they got very far.

And a side note, I can also implement the token request to apple with validation using a p8 if that's something people want. With that being said, it's only required for the web token so it's not for all devices :/.

I haven't seen the many people requesting anything for .p8 files for web auth but it sounds like it would make the Sign in with Apple implementation more complete so if you have the time it would be great.

@UnderratedDev
Copy link
Contributor Author

Yes a PR to the docs would be much appreciated and a PR for the iOS SDK would be fab too - I
think someone got started on some helper methods like we have with Facebook & Twitter utils
but I don't think they got very far.

Do you have the branch name or a link to see what work they have already done?

When I have some free time, I will update the iOS SDK (with possible utils code) & make a PR, update the documentation wherever needed (iOS, backend, JS SDK), I will update the documentation or update the code for Parse Backend to use .p8 files.

I've been thinking about that for a while now. For real test we can create our own token by using a key and signing it for testing.

const privateKey = fs.readFileSync("./parseKey.p8");
const token = jwt.sign({}, privateKey, {
 algorithm: "ES256",
 expiresIn: "60 days",
 audience: "https://appleid.apple.com",
 issuer: "TEAM_ID",
 subject: "com.mycustomdomain.webapp",
 keyid: "KEY_ID"
});

console.log("The token is:", token);

On that note, is it possible to get some resources for development, like a .p8 file, client id, etc, so that I don't have to test with my own identifiers (for all platforms, & I won't be able to make tests universal if I have to use my own). @dplewis & I had a brief discussion on this before ^^.

@TomWFox
Copy link
Contributor

TomWFox commented Apr 16, 2020

  1. See Adds Sign in with Apple Parse-SDK-iOS-OSX#1475 for a partial ParseUI implementation and see Apple Sign In Parse-SDK-iOS-OSX#1440 for some suggested Utils code & discussion. It might be good to link up with @drdaz on this - perhaps opening a new issue on the iOS SDK e.g. “Parse UI and helper methods for Sign In with Apple” would help to get the discussion going.

  2. I see the testing issue, if you can point me in the right direction I could probably generate the required resources on my dev account. It’s not an ideal solution but it might be better as I’m on the core team.

@UnderratedDev
Copy link
Contributor Author

@TomWFox Tutorial Thanks Bud :)

@TomWFox
Copy link
Contributor

TomWFox commented Apr 16, 2020

@UnderratedDev got it, are you planning to work on this now? If not perhaps you could ping me when you are and I'll get all the keys etc?

@UnderratedDev
Copy link
Contributor Author

@TomWFox I most likely will work on it this weekend, if not then, some time during the following week. So whenever you can get them, I can use my own p8 file for now, worst case

@UnderratedDev UnderratedDev mentioned this pull request Apr 26, 2020
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