search

wrobins / cordova-plugin-msal

Use the newest Microsoft MSAL library in your Cordova-based project!
Apache License 2.0
23 stars 63 forks source link

Allow client and tenant IDs to be specified on msal init at runtime #102

Closed peitschie closed 1 year ago

peitschie commented 1 year ago

Hi @wrobins ! Here's another tweak I've made recently. I have an app that required the ability to switch between different tenant & client IDs at runtime based on which region the app discovered it was in.

These are all fully backwards compatible, so existing uses are unimpacted by the change, and no migration work is required. Simply the tenant and client IDs can be overridden during the msalInit call 🙂

wrobins commented 1 year ago

@peitschie Nice work! I'm starting to wonder if we need these specified in the Cordova variables moving forward. I know when I first wrote this plugin, the native Android library needed them at build-time, but I think that's changed. It would make implementation a lot more straightforward in future releases if we streamlined those variables to be runtime-only, but I'll need to go through and make sure nothing else needs them at build-time. Thoughts?

marcelarmz commented 1 year ago

@peitschie @wrobins Tested this change for iOS. Seems like CLIENT_ID and TENANT_ID are still required in package.json. Broker login doesn't work if I don't have these declared in package.json.

This has so much potential. Good job @peitschie !

peitschie commented 1 year ago

@wrobins I'm inclined to agree! Given the plugin needs to be specifically init'd via msalInit anyways, it seems easy enough to supply the IDs in there rather than having two mechanisms. If you like, I'm happy to raise a PR to fix this?

@marcelarmz interesting 🤔 What error are you getting with the updated plugin when those fields are missing? I have an app going locally without any of these arguments in package.json, so interested to know what's different with your environment 😅

Perhaps let's move that to an issue to hammer out (presuming @wrobins doesn't mind!)?

peitschie commented 1 year ago

cc: @fordkilleen in here too, as they've done the heavy lifting putting the patch together! ❤️

wrobins commented 1 year ago

@peitschie I'll go ahead and take care of that this afternoon and tomorrow. I've been trying to get rid of having those variables declared in the global install variables so that I can stop doing hooks and other sketchy stuff to get it to work in the browser, in the OutSystems web preview (and reactive applications), as well as making Electron implementation easier. I'll bump this plugin a minor version and mark those two install variables as deprecated.