dommilosz / minecraft-auth

30 stars 3 forks source link

(Possible to?) Add support for PublicClientApplication structure for Microsoft Account authentication #17

Closed mica-alex closed 1 year ago

mica-alex commented 1 year ago

Would it be possible to add support for the public client structure for Microsoft Account authentication?

This falls in line with the Microsoft guidelines/recommendations for usage of their own MSAL library for Microsoft authentication in JS.

As a bonus, this would eliminate the need for a client secret to be specified, thus improving overall security of applications. They have more detailed documentation at this link: https://learn.microsoft.com/en-us/azure/active-directory/develop/msal-client-applications

I am only in the early stages of development for my new game launcher via Electron, so I still very much need to come up to speed on the language and this code base before I can offer useful assistance. ---- By all means though, please let me know if there is anything you'd like me to try and assist with, because I think this library is extremely valuable and certainly is much cleaner+simpler than a full blown MSAL library implementation such as https://learn.microsoft.com/en-us/azure/active-directory/develop/msal-js-initializing-client-applications

dommilosz commented 1 year ago

I tried to use it and I get an error saying that client secret is in fact needed.

import {PublicClientApplication} from "@azure/msal-node";

const msalConfig = {
    auth: {
        clientId: "app id",
        authority: "https://login.microsoftonline.com/common", 
        redirectUri: url
    }
};

const login = async () => {
    MicrosoftAuth.setup("app_id", "", url);
    console.log("http://localhost:8080/auth")
    let code = await MicrosoftAuth.listenForCode(8080)

    const authResult = await msalInstance.acquireTokenByCode({redirectUri: url, scopes: ["XboxLive.signin","offline_access"], code});
    //ServerError: invalid_client: 70002 - [2023-02-25 21:10:55Z]: AADSTS70002: The provided request must include a 'client_secret' input parameter.

    console.log("Access token:", authResult.accessToken);
};

Did you try to code something with it?

mica-alex commented 1 year ago

image I had not gotten as far as using the proper/necessary scopes, but I was able to successfully authenticate using that configuration. The only other changes I made were to enable my AAD 'Application' to be personal Microsoft accounts only.

The only major difference between what configuration you have and what I tested is that the authority you use includes /common while I used /consumers.

It may be worth a test, although I don't know the repercussions of limiting authentication to personal Microsoft accounts only. (I don't know if work/school accounts can even own Minecraft, and if so, how many people even use or rely on that)

My understanding is that /common is for personal Microsoft accounts and organizational (work/school) accounts alike, while /consumers is for personal Microsoft accounts only.

mica-alex commented 1 year ago

Here is a screenshot of the authentication configuration used with the above configuration on the Azure side:

image

mica-alex commented 1 year ago

I tested adding the scopes you mentioned to my basic MSAL AuthProvider and was able to get through login successfully.

I have pushed the working code to my repository for you to see: https://github.com/Mica-Technologies/minecraft-launcher/tree/electron-rebuild-work/src/main/api/auth

dommilosz commented 1 year ago

Thanks for the code I managed to implement the authentication. Are you able to refresh the token after few hours of inactivity? I don't see refresh token in the response and wondering if it can refresh itself, what data from authResponse you save to disc.

mica-alex commented 1 year ago

Unfortunately, I have not gotten that far. With MSAL, I believe it is supposed to have its own caching mechanism, and then you just specify a location or plugin. I tried with

cache: {
   cacheLocation: './data/cache.json'
}

but that has not worked for me.

I will keep experimenting and trying, and if I figure it out, will let you know.

mica-alex commented 1 year ago

I did a small test earlier today with promising results:

I copied the index.ts file for this plugin to my launcher code base temporarily., After removing all references to the app secret, I was able to successfully trigger authentication and get a code sent back to http://localhost:8080/token. The only modification I made on the Azure side was setting http://localhost:8080/token as an allowed redirect URI.

(edit: MSAL may not be required on your end at all. It may be as simple as removing the app secret entirely, then configuring the Azure AD application with the proper redirect URI under Mobile and desktop applications.

mica-alex commented 1 year ago

I pushed my current working source code for you to see, with a modified version of your index.ts file (named authLib.ts) to the previously mentioned repository and branch: https://github.com/Mica-Technologies/minecraft-launcher/tree/electron-rebuild-work/src/main/api/auth and https://github.com/Mica-Technologies/minecraft-launcher/blob/b0185e0b27372907a55e346c89977cf20ef8566f/src/main/ipc/ipcInit.ts#L47

Additionally, I was able to add working account serialization/deserialization on window open/close: https://github.com/Mica-Technologies/minecraft-launcher/blob/b0185e0b27372907a55e346c89977cf20ef8566f/src/main/main.ts#L145 and https://github.com/Mica-Technologies/minecraft-launcher/blob/b0185e0b27372907a55e346c89977cf20ef8566f/src/main/main.ts#L158

On a different note, do you think it is possible and/or a good idea to add support for allowing users to specify a 'final' redirect URL? For my testing, you can see I implemented this functionality on line 64: https://github.com/Mica-Technologies/minecraft-launcher/blob/b0185e0b27372907a55e346c89977cf20ef8566f/src/main/api/auth/authLib.ts#L64

dommilosz commented 1 year ago

I managed to reproduce your success. I didn't realise you used Mobile and desktop applications. Thanks

On a different note, do you think it is possible and/or a good idea to add support for allowing users to specify a 'final' redirect URL? For my testing, you can see I implemented this functionality on line 64: https://github.com/Mica-Technologies/minecraft-launcher/blob/b0185e0b27372907a55e346c89977cf20ef8566f/src/main/api/auth/authLib.ts#L64

It really doesn't matter that much if you stripped the token from the url.

dommilosz commented 1 year ago

Check out latest commit 4e9e72b. It also changed some function parameters and added the redirect after authentication option

mica-alex commented 1 year ago

Looks awesome! Thank you for moving forward with the suggestion and creating v2.0.0.

I look forward to using it and moving forward with my launcher implementation!

mica-alex commented 1 year ago

I've integrated v2.0.0 into my launcher and it is working great! I'll close this issue, because at this point, I think everything has been addressed.

If I run in to any issues in the future, I will open a new issue/let you know.

Thank you again!