Skip to content

Conversation

smitthhyy
Copy link
Contributor

Fixes: Issue # 20: Unfriendly text when authenticate success, but not have permission to run app.

Copy link
Contributor

@danieltjewett danieltjewett left a comment

Choose a reason for hiding this comment

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

Hi @smitthhyy , thanks for contributing!

My initial thought was this should go into the fail method, to prevent the azurecallback method from being cluttered with potentially many conditions from Microsoft's servers. I recognize this is a condition we haven't thought through yet though and have a couple of questions.

Could we get the error_description, or something simliar, in our validating token response, a few lines down? Or does this check / response from Microsoft only happen after the user authenticates in AD? If we can get the error_description, or something simliar, later, I'd recommend moving this code in the fail method. If not, I understand the check, and would probably want to add another function to keep the azurecallback method clean of numerous conditions.

@smitthhyy smitthhyy changed the title Signed-off-by: trevor <[email protected]> Resolve issue where user authenticates but does not have permission to run application displaying unfriendly error Dec 18, 2019
@danieltjewett danieltjewett self-requested a review December 18, 2019 17:43
Copy link
Contributor

@danieltjewett danieltjewett left a comment

Choose a reason for hiding this comment

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

I like where this is going. I think we need to refactor the fail method to handle better error messaging, but I don't think this PR needs to worry about that. Thanks for contributing!

@danieltjewett danieltjewett merged commit bb26d8b into rootinc:master Dec 20, 2019
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