#242 Add custom logout function
Merged 2 months ago by jflory7. Opened 3 months ago by shraddhaag.
fedora-commops/ shraddhaag/fedora-happiness-packets custom-logout  into  master

@@ -5,6 +5,10 @@ 

  with open("config.yml", 'r') as ymlfile:

      cfg = yaml.full_load(ymlfile)

  

+ def provider_logout(request):

+     redirect_url = 'https://iddev.fedorainfracloud.org/logout'

+     return redirect_url

+ 

  class OIDC(OIDCAuthenticationBackend):

      def update_user(self, user, claims):

          if user.username in cfg['auth']['admins']:

@@ -155,6 +155,7 @@ 

  LOGIN_REDIRECT_URL_FAILURE = '/error'

  LOGIN_URL = '/oidc/authenticate/'

  OIDC_RP_SCOPES = 'openid profile email'

+ OIDC_OP_LOGOUT_URL_METHOD = 'happinesspackets.messaging.auth.provider_logout'

  

  LOGGING = {

      'version': 1,

This commit adds a custom logout function as described in Mozilla Django OIDC docs.

Metadata Update from @jflory7:
- Pull-request tagged with: PASSED, needs info, new change, type - backend, type - summer coding

3 months ago

@shraddhaag From our conversation earlier, I understand this PR to be a work-in-progress because the logout doesn't take on the authentication endpoint. Let us know when you are ready for review.

BTW, it would be a good idea to figure out which authentication endpoint we should hard-code in (or alternatively, convert it to an environment variable if the endpoints need to be different for local development vs. production). This might be a possible explanation for your blocker. See below:

$ grep -rnw . -e 'iddev.fedorainfracloud.org'
./generate_client_secrets.sh:15:        https://iddev.fedorainfracloud.org/openidc/Registration \
./happinesspackets/settings/base.py:150:OIDC_OP_AUTHORIZATION_ENDPOINT = "https://iddev.fedorainfracloud.org/openidc/Authorization"
./happinesspackets/settings/base.py:151:OIDC_OP_TOKEN_ENDPOINT = "https://iddev.fedorainfracloud.org/openidc/Token"
./happinesspackets/settings/base.py:152:OIDC_OP_USER_ENDPOINT = "https://iddev.fedorainfracloud.org/openidc/UserInfo"

Metadata Update from @jflory7:
- Pull-request untagged with: needs info
- Pull-request tagged with: needs changes
- Request assigned

3 months ago

rebased onto 797e169d223bc468f44d8b7e54c6a63665ab4465

3 months ago

@jflory7 I finally got the custom logout function working!
Turns out what we had been using the logout endpoint for production instance for id.fedoraproject.org while we were logging in using development instance of iddev.fedorainfracloud.org.

Although this is functioning properly now, what I was trying to add further was redirecting from the logout page of Ipsilon to the home page of FHP. I used the following code:

def provider_logout(request):
    base_url = 'https://iddev.fedorainfracloud.org/logout'
    params = urlencode({
        "next": f"{request.scheme}://{request.get_host()}{reverse('messaging:start')}",
    })
    redirect_url = f"{base_url}?{params}"
    return redirect_url

But, this doesn't seem to work. I was wondering if the logout endpoint, https://iddev.fedorainfracloud.org/logout takes next or redirectTo parameters?

rebased onto f8dad34

3 months ago

@jflory7 I finally got the custom logout function working!

Awesome! :smile: Nice work on figuring this one out.

But, this doesn't seem to work. I was wondering if the logout endpoint, https://iddev.fedorainfracloud.org/logout takes next or redirectTo parameters?

Good question. Ipsilon authentication is outside of my knowledge. I suggest pinging Patrick (@puiterwijk) or Clement (@cverna) in #fedora-apps on Freenode IRC. One of them should be able to point you in the right direction if this is possible.

@shraddhaag Oops, I forgot to follow up here. :see_no_evil: Did you have a chance to figure this one out? If there not obvious docs about it, I think we can merge this as it is today. The important part was to get the logout bits implemented, which you already found the bug for and fixed. :smile:

What do you think?

Hey @shraddhaag, let's move ahead with finishing this PR. We can revisit the OpenID redirect endpoint later. This PR is open for a while and the answer on redirect is still unclear.

Could you please rebase this on master and we will go ahead and merge it?

rebased onto 20d44f4

2 months ago

Hi Justin. I've rebased the commit. This should be good to go now. Hopefully we'll be able to get some information about redirect parameter to make this a better UX.

Thanks @shraddhaag! :raised_hands: I opened #257 to keep track of the logout redirect for later. I'm going to go ahead and merge. Nice job on figuring this out in the first place!

Merging! :ocean:

Metadata Update from @jflory7:
- Pull-request untagged with: needs changes

2 months ago

Pull-Request has been merged by jflory7

2 months ago