Skip to content

added user email upn data in request session #364#365

Closed
ImamAbdullahKhan wants to merge 6 commits intosnok:mainfrom
ImamAbdullahKhan:email_feature
Closed

added user email upn data in request session #364#365
ImamAbdullahKhan wants to merge 6 commits intosnok:mainfrom
ImamAbdullahKhan:email_feature

Conversation

@ImamAbdullahKhan
Copy link
Copy Markdown

The OAuth2CallbackView has user as None if user is not registered in Django, so it can't provide user details but the AdfsAuthCodeBackend class authenticate function is called from the Django Backend authenticate function, so instead of extending OAuth2CallbackView, I have extended AdfsAuthCodeBackend class authenticate function to store username_claim in request session.
It can be used in login_failed as "user_email = request.session.get("username_claim")".

Please let me know if you need any further clarifications

@ImamAbdullahKhan
Copy link
Copy Markdown
Author

I think coverage / codecov (pull request) test failure can resolved by updating the token:
image

Copy link
Copy Markdown
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

This isn't something I'm willing to merge. I don't see the utility in the library capturing the user's email in the session. If there's a hook that can be added to make this easier for you to implement, that is more likely to be accepted.

@ImamAbdullahKhan
Copy link
Copy Markdown
Author

Thanks for the feedback! Instead of modifying the backend to store claims in the session, I’ve implemented a Django signal hook. This allows developers to listen for authentication events and handle claims as needed, also added sample code in signals.rst file. Please review it and let me know if still needs any changes.

access_token = adfs_response["access_token"]

# Extract claims before user lookup
claims = self.validate_access_token(access_token)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason you're not overriding validate_access_token in your code to achieve the logic you want?

Copy link
Copy Markdown
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

I'm against modifying the project for this purpose at this point. It should be possible to override validate_access_token in your own project to achieve what you're going for. That means the project is flexible enough as is and no changes are required.

Additionally, I suspect you may be feeding AI directly responses to us which isn't the most polite thing to do.

@ImamAbdullahKhan
Copy link
Copy Markdown
Author

ImamAbdullahKhan commented Apr 9, 2025

Ok Thanks for your Response, I thought Django unregistered email address display was a good feature to be add to the Login Failed Page.

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