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

Fix 'WSGIRequest' object does not support item assignment error #152

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

Conversation

iamlordaubrey
Copy link

@iamlordaubrey iamlordaubrey commented Jul 22, 2018

  • Delete request arguments in functions. It is redundant as the first argument is usually the request object
  • Reference the session object on the request (web_app_session.session). This fixes self.session[self.csrf_token_session_key] = csrf_token error

- Delete request arguments in functions. It is redundant as the first argument is usually the request object
- Call the session method on the request object (web_app_session.session). This fixes `self.session[self.csrf_token_session_key] = csrf_token` error
@greg-db
Copy link
Contributor

greg-db commented Jul 22, 2018

Thanks!

@greg-db greg-db requested a review from starforever July 22, 2018 23:46
@iamlordaubrey
Copy link
Author

@starforever might I also suggest changing web_app_session to request, as they are basically the same thing and the name request is more intuitive.

Copy link
Contributor

@starforever starforever left a comment

Choose a reason for hiding this comment

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

I think the error happens when you pass in an object for web_app_session which should actually be a dict.

I think we should keep these parameters simple, rather than making them some objects with fields like session or query_params without explicitly specifying how they are constructed. This makes it easier for others to understand the example code regardless of what libraries they use for building requests.

try:
oauth_result = \\
get_dropbox_auth_flow(web_app_session).finish(
get_dropbox_auth_flow(web_app_session.session).finish(
request.query_params)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove request you need to find proper replacement for all its usages within the function. Otherwise the code will break.

@iamlordaubrey
Copy link
Author

Ok, I think I understand.

DropboxOAuth2Flow class needs to take in web_app_session. However, the documentation would be wrong for Django's implementation because the URL handlers for /dropbox-auth-start and /dropbox-auth-finish (that is, the functions dropbox_auth_start and dropbox_auth_finish) take in request as the first (and in function-based views, the only) argument.

It is that request's session (request.session) that needs to be passed to DropboxOAuth2Flow __init__ method.

My point is, web_app_session needs to be passed to DropboxOAuthFlow class, but in Django, that web_app_session is found in the request object as request.session. Also, request is the first parameter the URL handler takes in;

Therefore for Django:

def get_dropbox_auth_flow(web_app_session):
    redirect_uri = "https://my-web-server.org/dropbox-auth-finish"
    return DropboxOAuth2Flow(
        APP_KEY, APP_SECRET, redirect_uri, web_app_session,
        "dropbox-auth-csrf-token")

# URL handler for /dropbox-auth-start
def dropbox_auth_start(request):
    authorize_url = get_dropbox_auth_flow(request.session).start()
    redirect_to(authorize_url)

# URL handler for /dropbox-auth-finish
def dropbox_auth_finish(request):
    try:
        oauth_result = \
                get_dropbox_auth_flow(request.session).finish(
                    request.query_params)
    except BadRequestException, e:
        http_status(400)
    ...

For Flask, session is global. We would just need to access it directly:

from flask import session, request

def get_dropbox_auth_flow(web_app_session):
    redirect_uri = "https://my-web-server.org/dropbox-auth-finish"
    return DropboxOAuth2Flow(
        APP_KEY, APP_SECRET, redirect_uri, web_app_session,
        "dropbox-auth-csrf-token")

# URL handler for /dropbox-auth-start
@app.route('/dropbox-auth-start')
def dropbox_auth_start():
    authorize_url = get_dropbox_auth_flow(session).start()
    redirect_to(authorize_url)

# URL handler for /dropbox-auth-finish
@app.route('/dropbox-auth-finish')
def dropbox_auth_finish():
    try:
        oauth_result = \
                get_dropbox_auth_flow(session).finish(
                    request.query_params)
    except BadRequestException, e:
        http_status(400)
    ...

The documentation specified two arguments, web_app_session and request, and that's what tripped me originally. With django, only one is needed which is request. With Flask, none is needed.

I think with Django's implementation, implementing for Flask would be clear and straightforward to a newbie.

@rogebrd rogebrd self-requested a review March 30, 2020 17:29
@rogebrd
Copy link
Contributor

rogebrd commented Mar 30, 2020

Hello, is this something you are still interested in working on?

@iamlordaubrey
Copy link
Author

Hello, is this something you are still interested in working on?

Sure @rogebrd. I'll take a look at the failing test

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

None yet

5 participants