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

allow overriding default allowedRoles #60

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tobiaskohlbau
Copy link

@tobiaskohlbau tobiaskohlbau commented Aug 23, 2022

If an app needs login a requirement the current adapter could not support such scenarios. This PR address this by allowing the user to override the rules. By default nothing changes and everything is allowed by using the anonymous role.

@tobiaskohlbau tobiaskohlbau force-pushed the main branch 6 times, most recently from b30f7b9 to e0481cb Compare August 23, 2022 16:15
If for e.g. the complete app should be guarded by authentication it's necessary to
apply an authenticated role to every route.

Signed-off-by: Tobias Kohlbau <[email protected]>
Copy link
Owner

@geoffrich geoffrich 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 the PR! Left a few comments. Please also merge in the latest changes from main to resolve merge conflicts.

### allowedRoles

An array of strings containing the roles allowed to access the app. For e.g. if the app needs to be restrictied it's possible to use a [custom role](https://docs.microsoft.com/en-us/azure/static-web-apps/authentication-authorization). *Notice* this only supports securing the complete app. If specific routes should be secured it's best to implement custom authentication within the app itself.
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if it would be better to link docs on role configuration instead of the general auth docs:

Suggested change
An array of strings containing the roles allowed to access the app. For e.g. if the app needs to be restrictied it's possible to use a [custom role](https://docs.microsoft.com/en-us/azure/static-web-apps/authentication-authorization). *Notice* this only supports securing the complete app. If specific routes should be secured it's best to implement custom authentication within the app itself.
An array of strings containing the roles allowed to access the app. For example, if the app needs to be restricted, it's possible to use a [custom role](https://docs.microsoft.com/en-us/azure/static-web-apps/configuration#securing-routes-with-roles). **Note:** this only supports securing the complete app. If specific routes should be secured, you should implement custom authentication within the app itself.

@@ -28,7 +28,8 @@ function validateCustomConfig(config) {
export default function ({
debug = false,
customStaticWebAppConfig = {},
esbuildOptions = {}
esbuildOptions = {},
allowedRoles = ['anonymous']
Copy link
Owner

Choose a reason for hiding this comment

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

I think the default should be undefined:

Suggested change
allowedRoles = ['anonymous']
allowedRoles

allowedRoles: allowedRoles
}
);

Copy link
Owner

Choose a reason for hiding this comment

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

Instead of remembering to set allowedRoles on each route, can we apply them all at once? e.g.

swaConfig.routes = swaConfig.routes.map((r) => ({ ...r, allowedRoles }));

Comment on lines +151 to +200
routes: [
{
route: "/.auth/login/facebook",
statusCode: 404
},
{
route: "/.auth/login/github",
statusCode: 404
},
{
route: "/.auth/login/google",
statusCode: 404
},
{
route: "/.auth/login/twitter",
statusCode: 404
},
{
route: "/.auth/*",
allowedRoles: [
"anonymous"
]
},
{
route: '/login',
allowedRoles: [
"anonymous"
],
rewrite: "/.auth/login/aad",
}
],
responseOverrides: {
'401': {
'redirect': '/login',
'statusCode': 302
}
},
auth: {
identityProviders: {
azureActiveDirectory: {
registration: {
openIdIssuer: "AAD_ISSUER",
clientIdSettingName: "AAD_CLIENT_ID",
clientSecretSettingName: "AAD_CLIENT_SECRET"
}
}
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this entire config in the docs? Can we just show setting allowedRoles?

@geoffrich
Copy link
Owner

This PR may be superseded by #66, which will allow for greater customization of the config.

@tobiaskohlbau
Copy link
Author

Sorry for the late response, I've had a look into #66. Is the plan to superseed this or would you rather merge this now and have the details of #66 change it later on? If we would like to get this merged now I'm happy to address the requested changes.

@geoffrich
Copy link
Owner

Hey - yeah, I think #66 will be the more robust solution, since it will allow applying allowedRoles to individual routes, and it would be weird to have a separate config option for a global allowedRoles. It fell off my radar a few months ago, and I'm hoping to get back to it soon (definitely before v1 of this adapter is released).

I'll leave this PR open until #66 is merged in case I change my mind, but let's hold off on making any more changes for now. Thanks!

@tobiaskohlbau
Copy link
Author

Thaks for the response, looking forward to the changes of #66. Let me know if I can help with anything to get that PR merged.

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.

2 participants