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

Feat: add screen_hint param in /authorize api for explicit signup redirection #420

Conversation

scaletech-milan
Copy link
Contributor

@scaletech-milan scaletech-milan commented Nov 20, 2023

What does this PR do?

Enable signup redirection via screen_hint param in /authorize API

Which issue(s) does this PR fix?

If this PR affects any API reference documentation, please share the updated endpoint references

authorizerdev/docs#50

- Introduce screen_hint param in /authorize for explicit signup redirection
Copy link
Contributor

@lakhansamani lakhansamani left a comment

Choose a reason for hiding this comment

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

This PR is missing change in signup page which should also include social media buttons

if err := memorystore.Provider.SetState(state, nonce); err != nil {
log.Debug("Error setting temp code", err)
}
}

loginURL := "/app?" + loginState
authURL := "/app?" + authState
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea is to not put ? & # in authURL use it or declare another variable which just holds path /appor /app/signup and use that variable in line 140, 144, 146

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the changes and added social media buttons on signup page

- Declare variable for base path and signup path
- Add social login on signup page
Copy link
Contributor

@lakhansamani lakhansamani left a comment

Choose a reason for hiding this comment

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

Just a minor suggestion overall looks good

@@ -16,4 +16,7 @@ const (
ResponseTypeToken = "token"
// For the Implicit grant of id_token, use response_type=id_token to include an identifier token.
ResponseTypeIDToken = "id_token"

// Constant indicating the "signup" screen hint for customizing authentication process and redirect to a signup page.
ScreenHint = "signup"
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets rename this const to ScreenHintSignUp
If there is something else available for screen hint then also we can accomodate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes True,
Updated!

- Update variable name for screen hint param
@lakhansamani lakhansamani merged commit de5c18b into authorizerdev:main Nov 21, 2023
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