-
Notifications
You must be signed in to change notification settings - Fork 33
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
base: main
Are you sure you want to change the base?
Conversation
b30f7b9
to
e0481cb
Compare
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]>
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.
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. |
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 wonder if it would be better to link docs on role configuration instead of the general auth docs:
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'] |
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 think the default should be undefined:
allowedRoles = ['anonymous'] | |
allowedRoles |
allowedRoles: allowedRoles | ||
} | ||
); | ||
|
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.
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 }));
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" | ||
} | ||
} | ||
} | ||
} | ||
} |
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 need this entire config in the docs? Can we just show setting allowedRoles
?
This PR may be superseded by #66, which will allow for greater customization of the config. |
Hey - yeah, I think #66 will be the more robust solution, since it will allow applying 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! |
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. |
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.