-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Feature/simple oauth #164
Feature/simple oauth #164
Conversation
# Conflicts: # src/dymaptic.GeoBlazor.Core/Model/LogicComponent.cs # src/dymaptic.GeoBlazor.Core/Scripts/arcGisJsInterop.ts # src/dymaptic.GeoBlazor.Core/dymaptic.GeoBlazor.Core.csproj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty great tim! Just a couple questions/thoughts for you.
@@ -2062,6 +2094,7 @@ protected override async Task OnInitializedAsync() | |||
protected override async Task OnAfterRenderAsync(bool firstRender) | |||
{ | |||
ApiKey = Configuration["ArcGISApiKey"]; | |||
AppId = Configuration["ArcGISAppId"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe if either or both are not specified in the configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the IConfiguration
indexer is defined to return a nullable string if the value isn't found.
/// </returns> | ||
public async Task<string> GetCurrentToken() | ||
{ | ||
return await InvokeAsync<string>("getToken"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was kind of hoping that we could centralize the "token" management such that the dev could just call "getCurrentToken" and get back the api key if that's the current token, or the oauth token if that's the current token. That way there is a central point to extract the current token if you need to pass it to a custom rest call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was good feedback and pushed me to do a bit more refactoring. I added an ApiKeyAuthentication
class, moved all the APIKey logic out of MapView
and into this LogicComponent
, and then created a new AuthenticationManager
class that checks for both api keys and app ids, initializes the correct logic component, and exposes GetCurrentToken
as you suggested.
agency that already has ArcGIS accounts. | ||
OAuth is a more secure way to authenticate your users with ArcGIS. It requires that your users have an ArcGIS account. | ||
You generate a new OAuth `Application` in the [ArcGIS Developer Dashboard](https://developers.arcgis.com/applications/), | ||
and then store the `ClientId` in your application configuration as `ArcGISAppId`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a bit here about setting the Portal URL as well to support custom enterprise portal logins?
@morehavoc I did another serious refactor, moving everything into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, I really like having this centralized this way!
{ | ||
if (string.IsNullOrWhiteSpace(_portalUrl)) | ||
{ | ||
_portalUrl = _configuration["ArcGISPortalUrl"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a default set for this somewhere? What I mean is I know we don't need it if it's just arcgis online, but maybe we should still have it set so that it's consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be consistent for every use of AuthenticationManager
, but across ArcGIS JS and GeoBlazor, it is more common to leave optional parameters as null
.
@morehavoc I have created a new PR so that you can review my changes to the OAuth flow.
I attempted to implement some of the ideas we discussed previously. What I discovered, however, is that
JSRuntime
is essentially never ready for calls until after a view has been initialized and rendered. Since we are using the JSIdentityManager
, this means we simply can't "pre-load" authentication.I added code in
MapView
that will trigger oauth initialization, and optionally trigger a login on first view render. However, you can also still call the methods you created inOAuthAuthentication
directly if you are doing other work and don't want to show a map.I created a Server OAuth sample to verify that the current pattern works for both Wasm and Server. The
AddScoped
method in DI works well for both, because there is only one effective scope in blazor WASM.Unfortunately, I hit some roadblocks with a MAUI implementation. dotnet/maui#2702 documents how OAuth is essentially broken for MAUI Windows/WinUI. Interestingly Morton from Esri commented on this with a workaround, probably what the ArcGIS .NET SDK team is using. I think it might make sense to have a MAUI-specific implementation in the future that uses the ArcGIS solution. But in the meantime, I'm comfortable with adding OAuth support minus MAUI as long as we document it as such.