openedx / open-edx-proposals

Proposals for Open edX architecture, best practices and processes
http://open-edx-proposals.readthedocs.io/
Other
44 stars 32 forks source link

OEP-42 and XBlock authentication #489

Open robrap opened 1 year ago

robrap commented 1 year ago

To Do item, post discussion:

As per the comment https://github.com/openedx/open-edx-proposals/issues/489#issuecomment-2123103869,

This may be another case where we just note in the OEP that XBlock authentication is not under development (or however you want to put it), and point to this ticket for more details.

So whomever is picking up this task, please add this note appropriately, point to this ticket, and update the OEP changelog. Thanks!

Original ticket description

Minor context: I just learned (or re-learned) about XBlock secure tokens used for authentication, implemented in the XBlock utils here. Note: this is old enough that for all I know I took part in reviews and just don't remember.

Here are some assorted thoughts related to this, that may or may not result in different tickets:

robrap commented 1 year ago

FYI: @bradenmacdonald

bradenmacdonald commented 1 year ago

@robrap Sorry for not writing an ADR about it. I did try to write an extensive docstring though as you can see. I think that if we decide to keep this, a retroactive ADR makes a lot of sense.

One important clarification is that that implementation and auth mechanism were only used by the Blockstore XBlock runtime which was only used for LabXchange, though may soon be used for Content Libraries V2 as well. So in some sense it is an experimental approach, to test stronger XBlock sandboxing. I think it has been shown to work well, but there is certainly room for a larger discussion around it before we decide if it's a useful auth mechanism to move forward with for sandboxed XBlocks, or if we should use something else.

There are no shared secret keys in this approach, and keys can be rotated whenever you want. The "secret" lives on the server and is used to generate a user-specific hashed token (asymmetrically hashed) which will work until it expires or the server's secret is rotated.

@kenclary has also been looking into this a lot lately and may have additional input about next steps.

robrap commented 1 year ago

Thanks @bradenmacdonald. No worries.

  1. The docstrings are a huge help.
  2. I’m not entirely clear on the mechanism planned for sandboxed XBlocks, but I assume the endpoint returning the secure token first relies on typical auth, and then this secure token is somehow passed to an iframe to be used for further calls?
  3. It sounded like this implementation was being taken as the de facto way of doing this, vs something to be considered and discussed with a new ADR. Maybe I got the wrong impression though?
bradenmacdonald commented 1 year ago

I’m not entirely clear on the mechanism planned for sandboxed XBlocks, but I assume the endpoint returning the secure token first relies on typical auth, and then this secure token is somehow passed to an iframe to be used for further calls?

Yes, exactly. The JavaScript code running within the iframe has no way to make authenticated calls to the LMS other than via this token, and this token only works for calls to the XBlock runtime API that are scoped to the particular block in the iframe. So it cannot access any other LMS APIs nor user data, which is the key thing.

I certainly assumed there would be some discussion on the merits of this approach and relevant alternatives before it is more widely adopted or blessed.

sarina commented 4 months ago

Closing this for no action in a year. Please reopen with more detail about current work to be done if you are picking it up or are asking someone else to do so.

robrap commented 4 months ago

@bradenmacdonald: What is the current status of this? I don't think we need to reopen unless someone makes progress on this, but at least it is somewhat documented somewhere by having this issue.

bradenmacdonald commented 4 months ago

@robrap As far as I know, there's no related activity happening now, though at some point in the next six months we're going to need to figure out a better solution for rendering XBlocks in MFEs, and then this may be relevant to the discussion.

robrap commented 4 months ago

@sarina:

  1. This may be another case where we just note in the OEP that XBlock authentication is not under development (or however you want to put it), and point to this ticket for more details.
  2. I'm not sure who needs to be aware of this ticket. Maybe @arbrandes? Others?
sarina commented 4 months ago

OK, re-opening as an issue to add that note.

sarina commented 2 months ago

In #612, I marked this OEP as deferred. Please discuss over there!